ARROW-7216: [Java] Improve the performance of setting/clearing individual bits by liyafan82 · Pull Request #5872 · apache/arrow

liyafan82 · 2019-11-20T10:42:39Z

Setting/clearing individual bits are key operations for Arrow. In this issue, we improve the performance these operations by:

replacing arithmetic operations with bit-wise operations
remove unnecessary casts between int/byte
provide new API to remove the if branch

Benchmark results show that for clearing a bit, the performance improve by 11%, and for general set/clear operation, the performance improve by 4.7%:

before:
BitVectorHelperBenchmarks.setValidityBitBenchmark avgt 5 4.524 ± 0.015 us/op

after:
BitVectorHelperBenchmarks.setValidityBitBenchmark avgt 5 4.313 ± 0.011 us/op
BitVectorHelperBenchmarks.setValidityBitToZeroBenchmark avgt 5 4.020 ± 0.016 us/op

github-actions · 2019-11-20T10:45:44Z

https://issues.apache.org/jira/browse/ARROW-7216

…dual bits

emkornfield · 2019-11-21T05:20:46Z

java/vector/src/main/java/org/apache/arrow/vector/BitVectorHelper.java

+   * @param index index to be set
+   */
+  public static void setValidityBitToZero(ArrowBuf validityBuffer, int index) {
+    final int byteIndex = byteIndex(index);


can this delegate to setValidityBit instead? I think it is likely JIT will inline and do dead-code elimination?

I am sorry I don't understand this comment. You mean this method should be called from setValidityBit?

sorry I thought I replied here, the performance benchmarks show the JIT will not eliminate the branches. we should comment on why code duplication is necessary instead of calling setValidityBit below.

I see. Thanks for the clarification.
I have added comments explicitly to discuss this.

emkornfield · 2019-11-21T05:22:24Z

java/vector/src/main/java/org/apache/arrow/vector/BitVectorHelper.java

I know this mimics the existing setValidityBitToOne, but I think unsetBit would be a better name (we can consider adding an alias for setValidityBitToOne as "setBit" as well?

Sounds good to me. Will provide the alias later.

Alias provided. Please take a look. Thank you.

emkornfield · 2019-11-21T05:23:58Z

java/vector/src/main/java/org/apache/arrow/vector/BitVectorHelper.java

    final int bitIndex = bitIndex(index);
-    byte currentByte = validityBuffer.getByte(byteIndex);
-    final byte bitMask = (byte) (1L << bitIndex);
+    int currentByte = validityBuffer.getByte(byteIndex);


please provide a comment on why you are assigning a byte to an int.

Good point. Comment added.

emkornfield · 2019-11-21T05:24:17Z

java/vector/src/main/java/org/apache/arrow/vector/BitVectorHelper.java

    final int bitIndex = bitIndex(index);
-    byte currentByte = validityBuffer.getByte(byteIndex);
-    final byte bitMask = (byte) (1L << bitIndex);
+    int currentByte = validityBuffer.getByte(byteIndex);


please comment on the assignment.

Comment added. Thank you.

emkornfield · 2019-11-21T05:24:31Z

java/vector/src/main/java/org/apache/arrow/vector/BitVectorHelper.java

@@ -74,12 +89,12 @@ public static void setValidityBitToOne(ArrowBuf validityBuffer, int index) {
  public static void setValidityBit(ArrowBuf validityBuffer, int index, int value) {


is this still used? can it be marked as deprecated?

I am afraid not. This will be used in scenarios where the bit to set is unknown a priori.
For example, in BaseVariableWidthVector#public void set(int index, int isSet, int start, int end, ArrowBuf buffer).

emkornfield

Thanks a few comments/suggestions.

emkornfield · 2019-11-22T05:44:54Z

+1, thank you.

…dual bits Setting/clearing individual bits are key operations for Arrow. In this issue, we improve the performance these operations by: 1. replacing arithmetic operations with bit-wise operations 2. remove unnecessary casts between int/byte 3. provide new API to remove the if branch Benchmark results show that for clearing a bit, the performance improve by 11%, and for general set/clear operation, the performance improve by 4.7%: before: BitVectorHelperBenchmarks.setValidityBitBenchmark avgt 5 4.524 ± 0.015 us/op after: BitVectorHelperBenchmarks.setValidityBitBenchmark avgt 5 4.313 ± 0.011 us/op BitVectorHelperBenchmarks.setValidityBitToZeroBenchmark avgt 5 4.020 ± 0.016 us/op Closes apache#5872 from liyafan82/fly_1120_clear and squashes the following commits: cb745b5 <liyafan82> Discuss the reason for duplicate logic aec3b04 <liyafan82> Use better method names 58b9735 <liyafan82> Improve the performance of setting/clearing individual bits Authored-by: liyafan82 <fan_li_ya@foxmail.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>

[ARROW-7216][Java] Improve the performance of setting/clearing indivi…

58b9735

…dual bits

liyafan82 force-pushed the fly_1120_clear branch from 20182ef to 58b9735 Compare November 21, 2019 04:44

emkornfield added the Component: Java label Nov 21, 2019

emkornfield reviewed Nov 21, 2019

View reviewed changes

emkornfield requested changes Nov 21, 2019

View reviewed changes

liyafan82 added 2 commits November 21, 2019 13:56

[ARROW-7216][Java] Use better method names

aec3b04

[ARROW-7216][Java] Discuss the reason for duplicate logic

cb745b5

emkornfield closed this in 95119a5 Nov 22, 2019

asfimport mentioned this pull request Nov 22, 2019

[Java] Improve the performance of setting/clearing individual bits #23512

Closed

		@@ -74,12 +89,12 @@ public static void setValidityBitToOne(ArrowBuf validityBuffer, int index) {
		public static void setValidityBit(ArrowBuf validityBuffer, int index, int value) {

Conversation

liyafan82 commented Nov 20, 2019

Uh oh!

github-actions bot commented Nov 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emkornfield left a comment

Choose a reason for hiding this comment

Uh oh!

emkornfield commented Nov 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments