Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Duplicate
    • Affects Version/s: None
    • Fix Version/s: 1.6
    • Component/s: None
    • Labels:
      None

      Description

      It's possible to speed up the regular checking of isEOFRecord by creating a buffer that is all zeros then using the Java primitive Arrays.equals function. My bench-marking puts it at roughly 1/3 the time per check. No unit tests were harmed in the production of these modifications.

        Issue Links

          Activity

          Hide
          Stefan Bodewig added a comment -

          not strictly a duplicate but replaced by it

          Show
          Stefan Bodewig added a comment - not strictly a duplicate but replaced by it
          Hide
          BELUGA BEHR added a comment -

          Sebb, Stefan,

          I have re-wrote a chunk of the TAR archiver. Please review. I ultimately fell back to something similar to what was there before (checking with a for-loop), but I took the code from IOUtils and made an external call from isEOFRecord(). So now there's an external, easy-to-read method, and a check without creating a buffer.

          Please see:

          https://issues.apache.org/jira/browse/COMPRESS-234

          Show
          BELUGA BEHR added a comment - Sebb, Stefan, I have re-wrote a chunk of the TAR archiver. Please review. I ultimately fell back to something similar to what was there before (checking with a for-loop), but I took the code from IOUtils and made an external call from isEOFRecord(). So now there's an external, easy-to-read method, and a check without creating a buffer. Please see: https://issues.apache.org/jira/browse/COMPRESS-234
          Hide
          Sebb added a comment -

          The method isEofRecord is certainly simpler.

          However the constructor is more complicated, and the setup code is not documented.

          It is vital that isEofRecord is called with a buffer that is exactly the same size as eofRecord otherwise the method will return false instead of true.

          This assumption needs to be very carefully documented.

          Is it guaranteed that the buffer will always be the correct size?

          A subtle code change could break the code; i.e. the code is now less robust.

          Show
          Sebb added a comment - The method isEofRecord is certainly simpler. However the constructor is more complicated, and the setup code is not documented. It is vital that isEofRecord is called with a buffer that is exactly the same size as eofRecord otherwise the method will return false instead of true. This assumption needs to be very carefully documented. Is it guaranteed that the buffer will always be the correct size? A subtle code change could break the code; i.e. the code is now less robust.
          Hide
          Stefan Bodewig added a comment -

          To me it is more about readability than about performance.

          Worst case we are wasting the size of on record (512 bytes by default) and not gaining performance.

          Show
          Stefan Bodewig added a comment - To me it is more about readability than about performance. Worst case we are wasting the size of on record (512 bytes by default) and not gaining performance.
          Hide
          Sebb added a comment -

          svn revision 1500788

          Are we sure that the Arrays.equals check really is faster?

          And is it worth trading the additional memory usage for the speed up?

          How much difference does it make overall to reading a file?
          I would have thought any increase in speed is likely to be dwarfed by the other costs.

          Show
          Sebb added a comment - svn revision 1500788 Are we sure that the Arrays.equals check really is faster? And is it worth trading the additional memory usage for the speed up? How much difference does it make overall to reading a file? I would have thought any increase in speed is likely to be dwarfed by the other costs.
          Hide
          Sebb added a comment -

          the if-statement inside the loop will perform a bounds check on each access to record, this may be costly

          Arrays.equals has to do the same check against 2 arrays unless the compiler can somehow work out that the index will never go out of bounds.

          If that is the case, then maybe there is a way to hint to the compiler that this is true in Compress.

          - and Arrays.equals has several ways to get optimized by the compiler/runtime.

          I suppose it is more likely to be used, but otherwise the loops are very similar.

          Show
          Sebb added a comment - the if-statement inside the loop will perform a bounds check on each access to record, this may be costly Arrays.equals has to do the same check against 2 arrays unless the compiler can somehow work out that the index will never go out of bounds. If that is the case, then maybe there is a way to hint to the compiler that this is true in Compress. - and Arrays.equals has several ways to get optimized by the compiler/runtime. I suppose it is more likely to be used, but otherwise the loops are very similar.
          Hide
          Stefan Bodewig added a comment -

          svn revision 1500788

          Show
          Stefan Bodewig added a comment - svn revision 1500788
          Hide
          Stefan Bodewig added a comment -

          with your second patch (removing the null check) isEOFRecord will now return false if record is null. Trivial to fix but it seems we are missing a unit test if you got away with it

          Show
          Stefan Bodewig added a comment - with your second patch (removing the null check) isEOFRecord will now return false if record is null. Trivial to fix but it seems we are missing a unit test if you got away with it
          Hide
          Stefan Bodewig added a comment -

          the if-statement inside the loop will perform a bounds check on each access to record, this may be costly - and Arrays.equals has several ways to get optimized by the compiler/runtime.

          I've already replaced some loops in other places in commons with Arrays.equals - if only for clarity.

          Show
          Stefan Bodewig added a comment - the if-statement inside the loop will perform a bounds check on each access to record, this may be costly - and Arrays.equals has several ways to get optimized by the compiler/runtime. I've already replaced some loops in other places in commons with Arrays.equals - if only for clarity.
          Hide
          BELUGA BEHR added a comment -

          Sebb,

          Thanks for the feedback. I'll investigate further. I updated the patch to make it even more simple. The Arrays.equals() does null checking already, so I removed that.

          My test did not take into the account of creating the initial array. That is a one-time cost though v.s. navigating the entries each time. I'll look at the effects of that.

          As far as assuming the initial array is all zeros, it's in the Java Spec.

          Each class variable, instance variable, or array component is initialized with a default value when it is created (§15.9, §15.10):
          
          For type byte, the default value is zero, that is, the value of (byte)0.
          
          For type short, the default value is zero, that is, the value of (short)0.
          
          For type int, the default value is zero, that is, 0.
          
          For type long, the default value is zero, that is, 0L.
          
          For type float, the default value is positive zero, that is, 0.0f.
          
          For type double, the default value is positive zero, that is, 0.0d.
          
          For type char, the default value is the null character, that is, '\u0000'.
          
          For type boolean, the default value is false.
          
          For all reference types (§4.3), the default value is null.
          
          Show
          BELUGA BEHR added a comment - Sebb, Thanks for the feedback. I'll investigate further. I updated the patch to make it even more simple. The Arrays.equals() does null checking already, so I removed that. My test did not take into the account of creating the initial array. That is a one-time cost though v.s. navigating the entries each time. I'll look at the effects of that. As far as assuming the initial array is all zeros, it's in the Java Spec. Each class variable, instance variable, or array component is initialized with a default value when it is created (§15.9, §15.10): For type byte , the default value is zero, that is, the value of ( byte )0. For type short , the default value is zero, that is, the value of ( short )0. For type int , the default value is zero, that is, 0. For type long , the default value is zero, that is, 0L. For type float , the default value is positive zero, that is, 0.0f. For type double , the default value is positive zero, that is, 0.0d. For type char , the default value is the null character, that is, '\u0000'. For type boolean , the default value is false . For all reference types (§4.3), the default value is null .
          Hide
          Sebb added a comment -

          That is odd, because Arrays.equals has to do exactly the same - in fact it does more, because it has to check the sizes too.

          Maybe the issue is in the for loop header - why does it call the overrideable method getRecordSize()?

          Perhaps using this.recordSize would be faster?

          Also does your benchmark take into account the cost of creating the empty array?
          What about ensuring that the array contains zeros initially?

          Maybe the compiler/runtime has optimised the Arrays code and not the Compress code.
          Remember to "warm-up" any code before doing timing comparisons.

          Show
          Sebb added a comment - That is odd, because Arrays.equals has to do exactly the same - in fact it does more, because it has to check the sizes too. Maybe the issue is in the for loop header - why does it call the overrideable method getRecordSize()? Perhaps using this.recordSize would be faster? Also does your benchmark take into account the cost of creating the empty array? What about ensuring that the array contains zeros initially? Maybe the compiler/runtime has optimised the Arrays code and not the Compress code. Remember to "warm-up" any code before doing timing comparisons.

            People

            • Assignee:
              Unassigned
              Reporter:
              BELUGA BEHR
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development