ARROW-5909: [Java] Optimize ByteFunctionHelpers equals & compare logic #4852
Conversation
| int leftInt = PlatformDependent.getInt(lPos); | ||
| int rightInt = PlatformDependent.getInt(rPos); | ||
| if (leftInt != rightInt) { | ||
| return unsignedIntCompare(Integer.reverseBytes(leftInt), Integer.reverseBytes(rightInt)); |
There was a problem hiding this comment.
maybe i am missing something :) but why are we reversing the bytes here..should not PlatformDependent already return the interpreted value according to endianess..
There was a problem hiding this comment.
Thanks for your comments. Seems you are right, I just make it similar with Long value logic(https://github.com/apache/arrow/blob/master/java/vector/src/main/java/org/apache/arrow/vector/util/ByteFunctionHelpers.java#L129). Looks like a legacy issue.
I would like to remove these reverse operations and add UT for this class, what do you think?
There was a problem hiding this comment.
I updated this PR, would you mind taking a look again? thanks!
|
@emkornfield Could you please take a look if we could get this merged? thanks! |
|
|
||
| buffer1.setByte(50, 10); | ||
|
|
||
| assertEquals(-1, ByteFunctionHelpers.compare(buffer1, 0, SIZE - 1, |
There was a problem hiding this comment.
can you add another test case for the + case.
In an ideal world we would explicitly test the three cases for different lengths >8, >3 and <=3
Codecov Report
@@ Coverage Diff @@
## master #4852 +/- ##
==========================================
+ Coverage 87.42% 89.58% +2.15%
==========================================
Files 996 661 -335
Lines 139830 96464 -43366
Branches 1418 0 -1418
==========================================
- Hits 122246 86415 -35831
+ Misses 17222 10049 -7173
+ Partials 362 0 -362Continue to review full report at Codecov.
|
|
+1, LGTM. Thank you for the additional tests. |
emkornfield
changed the title
Related to [ARROW-5909](https://issues.apache.org/jira/browse/ARROW-5909). Now it first compare Long values and then if length < 8 then it compares Byte values. Add the logic to compare Int values when 4 < length < 8. Buffer length far greater than 8 have similar performance in equals, in case buffer length < 8 performances are as below: ByteFunctionHelpersBenchmarks#equals Before: avgt 5 7.718 ± 0.401 ns/op After: avgt 5 5.005 ± 0.830 ns/op Author: tianchen <niki.lj@alibaba-inc.com> Closes #4852 from tianchen92/ARROW-5909 and squashes the following commits: 18db45f <tianchen> add more UT dbd0923 <tianchen> remove reverse in ByteFunctionHelpers 41030c4 <tianchen> fix e6ab2ed <tianchen> ARROW-5909: Optimize ByteFunctionHelpers equals & compare logic
Related to ARROW-5909.
Now it first compare Long values and then if length < 8 then it compares Byte values.
Add the logic to compare Int values when 4 < length < 8.
Buffer length far greater than 8 have similar performance in equals, in case buffer length < 8 performances are as below:
ByteFunctionHelpersBenchmarks#equals
Before: avgt 5 7.718 ± 0.401 ns/op
After: avgt 5 5.005 ± 0.830 ns/op