Re: [PR] [improve][misc] Add LongArrayAckSets for allocation-free ack set operations [pulsar]

["Shawyeok (via GitHub)" <gi...@apache.org>]

Shawyeok commented on code in PR #26128:
URL: https://github.com/apache/pulsar/pull/26128#discussion_r3984602119


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/PositionAckSetUtil.java:
##########
@@ -54,20 +55,11 @@ public static void andAckSet(Position currentPosition, Position otherPosition) {
 
     //This method is do `and` operation for ack set
     public static long[] andAckSet(long[] firstAckSet, long[] secondAckSet) {
-        BitSetRecyclable thisAckSet = BitSetRecyclable.valueOf(firstAckSet);
-        BitSetRecyclable otherAckSet = BitSetRecyclable.valueOf(secondAckSet);
-        thisAckSet.and(otherAckSet);
-        long[] ackSet = thisAckSet.toLongArray();
-        thisAckSet.recycle();
-        otherAckSet.recycle();
-        return ackSet;
+        return LongArrayAckSets.intersect(firstAckSet, secondAckSet);
     }
 
     public static boolean isAckSetEmpty(long[] ackSet) {
-        BitSetRecyclable bitSet =  BitSetRecyclable.create().resetWords(ackSet);
-        boolean isEmpty = bitSet.isEmpty();
-        bitSet.recycle();
-        return isEmpty;
+        return LongArrayAckSets.cardinality(ackSet) == 0;

Review Comment:
   Sorry for the late reply. Good catch, fixed. Added `LongArrayAckSets.isEmpty(long[])`, which returns `false` as soon as it hits a non-zero word, and switched `isAckSetEmpty` to use it instead of `cardinality(ackSet) == 0`.



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java:
##########
@@ -804,23 +803,15 @@ private long computeAckedCount(MessageIdData msgId, Position position, Consumer
         }
         long[] cursorAckSet = getCursorAckSet(position);
         if (cursorAckSet == null) {
-            return batchSize - BitSet.valueOf(ackSets).cardinality();
+            return batchSize - LongArrayAckSets.cardinality(ackSets);
         }
-        BitSetRecyclable cursorBitSet = BitSetRecyclable.create().resetWords(cursorAckSet);
-        int lastCardinality = cursorBitSet.cardinality();
-        BitSetRecyclable givenBitSet = BitSetRecyclable.create().resetWords(ackSets);
-        cursorBitSet.and(givenBitSet);
-        givenBitSet.recycle();
-        int currentCardinality = cursorBitSet.cardinality();
-        cursorBitSet.recycle();
+        int lastCardinality = LongArrayAckSets.cardinality(cursorAckSet);
+        int currentCardinality = LongArrayAckSets.cardinalityOfIntersection(cursorAckSet, ackSets);

Review Comment:
   Sorry for the late reply. Added `LongArrayAckSets.cardinalityOfDifference(long[], long[])` exactly as you described — single pass over `ackSet1[i] & ~ackSet2[i]`, with words beyond the shorter array's length treated as zero — and switched `computeAckedCount` to use it instead of the two-scan `cardinality(cursor) - cardinalityOfIntersection(cursor, ack)`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org