Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-7445

Implement bulk checksum verification using efficient native code

Details

    • Improvement
    • Status: Closed
    • Major
    • Resolution: Fixed
    • None
    • 0.23.0
    • native, util
    • None
    • Reviewed

    Description

      Once HADOOP-7444 is implemented ("bulk" API for checksums), good performance gains can be had by implementing bulk checksum operations using JNI. This JIRA is to add checksum support to the native libraries. Of course if native libs are not available, it will still fall back to the pure-Java implementations.

      Attachments

        1. hadoop-7445.txt
          76 kB
          Todd Lipcon
        2. hadoop-7445.txt
          77 kB
          Todd Lipcon
        3. hadoop-7445.txt
          77 kB
          Todd Lipcon
        4. hadoop-7445.txt
          79 kB
          Todd Lipcon
        5. hadoop-7445.txt
          73 kB
          Todd Lipcon
        6. hadoop-7445.txt
          71 kB
          Todd Lipcon

        Issue Links

          Activity

            tlipcon Todd Lipcon added a comment -

            Patch on top of HADOOP-7444 and HADOOP-7443

            tlipcon Todd Lipcon added a comment - Patch on top of HADOOP-7444 and HADOOP-7443
            tlipcon Todd Lipcon added a comment -

            Updated patch using tables generated by TestPureJavaCrc32$Table

            tlipcon Todd Lipcon added a comment - Updated patch using tables generated by TestPureJavaCrc32$Table
            hadoopqa Hadoop QA added a comment -

            +1 overall. Here are the results of testing the latest attachment
            http://issues.apache.org/jira/secure/attachment/12486257/hadoop-7445.txt
            against trunk revision 1146300.

            +1 @author. The patch does not contain any @author tags.

            +1 tests included. The patch appears to include 3 new or modified tests.

            +1 javadoc. The javadoc tool did not generate any warning messages.

            +1 javac. The applied patch does not increase the total number of javac compiler warnings.

            +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

            +1 release audit. The applied patch does not increase the total number of release audit warnings.

            +1 core tests. The patch passed core unit tests.

            +1 system test framework. The patch passed system test framework compile.

            Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/726//testReport/
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/726//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
            Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/726//console

            This message is automatically generated.

            hadoopqa Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12486257/hadoop-7445.txt against trunk revision 1146300. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/726//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/726//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/726//console This message is automatically generated.
            kihwal Kihwal Lee added a comment -

            Is there any specific reason why the checksum update functions are in the headers?

            kihwal Kihwal Lee added a comment - Is there any specific reason why the checksum update functions are in the headers?
            tlipcon Todd Lipcon added a comment -

            I wanted to put them in a different file, but also wanted to make sure they got inlined. Though they're only used by this bit of code, I could imagine in the future wanting to be able to re-use them from other code (eg to implement a native block reader as part of libhdfs)

            If you don't think a separate file is necessary, happy to just inline the contents of those files into the main code.

            tlipcon Todd Lipcon added a comment - I wanted to put them in a different file, but also wanted to make sure they got inlined. Though they're only used by this bit of code, I could imagine in the future wanting to be able to re-use them from other code (eg to implement a native block reader as part of libhdfs) If you don't think a separate file is necessary, happy to just inline the contents of those files into the main code.
            kihwal Kihwal Lee added a comment -

            Sorry, probably I wasn't very clear. I didn't mean to suggest jamming everything into one. I was thinking about exposing a set of pure native functions at multi-chunk checksum processing level in addition to single chunk processing. I imagine this level of abstraction is good enough for the native client code. If we can separate JNI-wrapper bits from these, the native functions can go to their own C file(s) and function prototypes in header(s). The rest is up to the build system. The build system can make it part of libhadoop or make it an independent lib or be part of native executables or whatever. As for inlining/optimization, we can optimize within the multi-chunk functions and that should be good enough for most use cases. Does it make sense?

            kihwal Kihwal Lee added a comment - Sorry, probably I wasn't very clear. I didn't mean to suggest jamming everything into one. I was thinking about exposing a set of pure native functions at multi-chunk checksum processing level in addition to single chunk processing. I imagine this level of abstraction is good enough for the native client code. If we can separate JNI-wrapper bits from these, the native functions can go to their own C file(s) and function prototypes in header(s). The rest is up to the build system. The build system can make it part of libhadoop or make it an independent lib or be part of native executables or whatever. As for inlining/optimization, we can optimize within the multi-chunk functions and that should be good enough for most use cases. Does it make sense?
            tlipcon Todd Lipcon added a comment -

            Finally snuck some time to update this based on your feedback. How does this look? I split up the code into the following files:

            • NativeCrc32c: just JNI wrappers
            • bulk_crc32. {c,h}

              : a "C style" API for bulk checksum verification

            • crc32.h: the actual implementations (to be inlined)
            • gcc_optimizations.h: the macros for builtin_expect, now used from multiple spots

            I ran TestDataChecksum with native code built, and with -Xcheck:jni enabled, and it passes.

            tlipcon Todd Lipcon added a comment - Finally snuck some time to update this based on your feedback. How does this look? I split up the code into the following files: NativeCrc32c: just JNI wrappers bulk_crc32. {c,h} : a "C style" API for bulk checksum verification crc32.h: the actual implementations (to be inlined) gcc_optimizations.h: the macros for builtin_expect, now used from multiple spots I ran TestDataChecksum with native code built, and with -Xcheck:jni enabled, and it passes.
            kihwal Kihwal Lee added a comment -

            +1

            This is a minor point. Do you think we need to expose crc32c_sb8() and crc32c_zlib_sb8()? If not, why don't we move these to bulk_crc32.c and get rid of crc32.h? I thought the bulk verification API is all we need to make available, but I may be wrong. If you believe it is better as is, that's fine too.

            kihwal Kihwal Lee added a comment - +1 This is a minor point. Do you think we need to expose crc32c_sb8() and crc32c_zlib_sb8()? If not, why don't we move these to bulk_crc32.c and get rid of crc32.h? I thought the bulk verification API is all we need to make available, but I may be wrong. If you believe it is better as is, that's fine too.
            tlipcon Todd Lipcon added a comment -

            sure, I'll do that - I agree it makes sense.

            tlipcon Todd Lipcon added a comment - sure, I'll do that - I agree it makes sense.
            tlipcon Todd Lipcon added a comment -

            Updated patch based on above suggestion.

            tlipcon Todd Lipcon added a comment - Updated patch based on above suggestion.
            kihwal Kihwal Lee added a comment -

            +1 The patch looks good.

            kihwal Kihwal Lee added a comment - +1 The patch looks good.
            szetszwo Tsz-wo Sze added a comment -

            Two quick questions:

            • Do we have to include the BSD license if we generate the tables ourselves?
            • Have you tested the correctness and performance with our Java implementations?

            I will check the patch in more details.

            szetszwo Tsz-wo Sze added a comment - Two quick questions: Do we have to include the BSD license if we generate the tables ourselves? Have you tested the correctness and performance with our Java implementations? I will check the patch in more details.
            tlipcon Todd Lipcon added a comment -

            Good point that we don't need the special license on the tables, since we generated them using your Table class. But, the actual "slicing-by-8" implementation is from a project with BSD license. So, I moved that special license header to bulk_crc32.c.

            This new revision also rebases on the mavenized common.

            As for testing performance and correctness against the existing implementation:

            • Performance wise, we don't currently have a canned benchmark for testing performance of checksum verification. This patch doesn't currently add native checksum computation anywhere, since the umbrella JIRA HDFS-2080 is focusing on the read path. I was able to run benchmarks of "hadoop fs -cat /dev/shm/128M /dev/shm/128M /dev/shm/128M [repeated 50 times]" using a ChecksumFileSystem, and saw ~60% speed improvement. This is a measurement of CPU overhead, since it's reading from a file in a RAM disk.
            • Correctness wise, the new test cases in TestDataChecksum verify both the native and non-native code, since they test with direct buffers as well as heap buffers that wrap a byte[]. If the native and non-native code disagreed, then this test would fail for one of the two cases (since the computed checksums are always computed by the java code)
            tlipcon Todd Lipcon added a comment - Good point that we don't need the special license on the tables, since we generated them using your Table class. But, the actual "slicing-by-8" implementation is from a project with BSD license. So, I moved that special license header to bulk_crc32.c. This new revision also rebases on the mavenized common. As for testing performance and correctness against the existing implementation: Performance wise, we don't currently have a canned benchmark for testing performance of checksum verification . This patch doesn't currently add native checksum computation anywhere, since the umbrella JIRA HDFS-2080 is focusing on the read path. I was able to run benchmarks of "hadoop fs -cat /dev/shm/128M /dev/shm/128M /dev/shm/128M [repeated 50 times] " using a ChecksumFileSystem, and saw ~60% speed improvement. This is a measurement of CPU overhead, since it's reading from a file in a RAM disk. Correctness wise, the new test cases in TestDataChecksum verify both the native and non-native code, since they test with direct buffers as well as heap buffers that wrap a byte[]. If the native and non-native code disagreed, then this test would fail for one of the two cases (since the computed checksums are always computed by the java code)
            tlipcon Todd Lipcon added a comment -

            btw, just to "test the tests", I tried flipping a single bit in one of the tables in the native code, and the tests failed immediately as a result.

            tlipcon Todd Lipcon added a comment - btw, just to "test the tests", I tried flipping a single bit in one of the tables in the native code, and the tests failed immediately as a result.
            szetszwo Tsz-wo Sze added a comment -

            Todd, nice works. Some comments:

            • It is better to simply use org_apache_hadoop_util_NativeCrc32_CHECKSUM_CRC32 and org_apache_hadoop_util_NativeCrc32_CHECKSUM_CRC32C, instead of defining two more constants, CRC32_ZLIB_POLYNOMIAL and CRC32C_POLYNOMIAL.
            • How about moving everything in gcc_optimizations.h to org_apache_hadoop.h?
            szetszwo Tsz-wo Sze added a comment - Todd, nice works. Some comments: It is better to simply use org_apache_hadoop_util_NativeCrc32_CHECKSUM_CRC32 and org_apache_hadoop_util_NativeCrc32_CHECKSUM_CRC32C, instead of defining two more constants, CRC32_ZLIB_POLYNOMIAL and CRC32C_POLYNOMIAL. How about moving everything in gcc_optimizations.h to org_apache_hadoop.h?
            tlipcon Todd Lipcon added a comment -

            Hi Nicholas. Thanks for reviewing.

            • The reason I opted to use a second set of constants was because Kihwal had asked me to split the code into two distinct parts: one that does the JNI wrapping, and one that is a straight C library. Since the org_apache_hadoop_* constants are generated by javah, they aren't usable from the C library portion. Does that make sense?
            • Same goes for gcc_optimizations.h – since org_apace_hadoop.h has JNI-specific stuff, I didn't want to include it from within bulk_crc32.c, the "non-JNI" section of the code.

            Does that make sense?

            tlipcon Todd Lipcon added a comment - Hi Nicholas. Thanks for reviewing. The reason I opted to use a second set of constants was because Kihwal had asked me to split the code into two distinct parts: one that does the JNI wrapping, and one that is a straight C library. Since the org_apache_hadoop_* constants are generated by javah, they aren't usable from the C library portion. Does that make sense? Same goes for gcc_optimizations.h – since org_apace_hadoop.h has JNI-specific stuff, I didn't want to include it from within bulk_crc32.c, the "non-JNI" section of the code. Does that make sense?
            szetszwo Tsz-wo Sze added a comment -

            +1 Patch looks good.

            It makes a lot of sense. Thanks.

            szetszwo Tsz-wo Sze added a comment - +1 Patch looks good. It makes a lot of sense. Thanks.
            tlipcon Todd Lipcon added a comment -

            Attaching final patch for commit - same as the above minus the change to the surefire settings in pom.xml, since those were covered by another fix that went into common yesterday.

            I ran test-patch, and, modulo some issues with the test-patch script that Tom is looking into, it checked out OK.

            I'll commit this momentarily.

            tlipcon Todd Lipcon added a comment - Attaching final patch for commit - same as the above minus the change to the surefire settings in pom.xml, since those were covered by another fix that went into common yesterday. I ran test-patch, and, modulo some issues with the test-patch script that Tom is looking into, it checked out OK. I'll commit this momentarily.

            People

              tlipcon Todd Lipcon
              tlipcon Todd Lipcon
              Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: