Hadoop Common
  1. Hadoop Common
  2. HADOOP-7445

Implement bulk checksum verification using efficient native code

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.23.0
    • Component/s: native, util
    • Labels:
      None
    • Hadoop Flags:
      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.

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

        Issue Links

          Activity

          Hide
          Todd Lipcon added a comment -

          Patch on top of HADOOP-7444 and HADOOP-7443

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

          Updated patch using tables generated by TestPureJavaCrc32$Table

          Show
          Todd Lipcon added a comment - Updated patch using tables generated by TestPureJavaCrc32$Table
          Hide
          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.

          Show
          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.
          Hide
          Kihwal Lee added a comment -

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

          Show
          Kihwal Lee added a comment - Is there any specific reason why the checksum update functions are in the headers?
          Hide
          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.

          Show
          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.
          Hide
          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?

          Show
          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?
          Hide
          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.

          Show
          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.
          Hide
          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.

          Show
          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.
          Hide
          Todd Lipcon added a comment -

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

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

          Updated patch based on above suggestion.

          Show
          Todd Lipcon added a comment - Updated patch based on above suggestion.
          Hide
          Kihwal Lee added a comment -

          +1 The patch looks good.

          Show
          Kihwal Lee added a comment - +1 The patch looks good.
          Hide
          Tsz Wo Nicholas 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.

          Show
          Tsz Wo Nicholas 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.
          Hide
          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)
          Show
          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)
          Hide
          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.

          Show
          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.
          Hide
          Tsz Wo Nicholas 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?
          Show
          Tsz Wo Nicholas 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?
          Hide
          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?

          Show
          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?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 Patch looks good.

          It makes a lot of sense. Thanks.

          Show
          Tsz Wo Nicholas Sze added a comment - +1 Patch looks good. It makes a lot of sense. Thanks.
          Hide
          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.

          Show
          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

            • Assignee:
              Todd Lipcon
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development