Denovo1998 commented on code in PR #26128:
URL: https://github.com/apache/pulsar/pull/26128#discussion_r3565836031
##########
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:
Since this call only needs an emptiness check, computing the full
cardinality scans and popcounts every word even after a non-zero word has been
found. This is on the dispatcher path and the purpose of the PR is to reduce
ack-set overhead. Could LongArrayAckSets expose an isEmpty(long[]) helper that
returns false at the first non-zero word, and use that here?
##########
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:
This result can be computed with one scan instead of scanning cursorAckSet
twice. For every bit, C - (C & A) is equivalent to C & ~A, so:
```
cardinality(cursor) - cardinality(cursor & ack)
= cardinality(cursor & ~ack)
```
Here, cursor & ~ack directly represents indexes that were unacked in the
cursor and become acked by this acknowledgment. For unequal array lengths,
words beyond ackSets.length must still contribute their full cursor
cardinality, because the missing ack words are treated as zero by the previous
BitSet.and implementation.
Could this be implemented as a single-pass helper such as
cardinalityOfDifference(cursorAckSet, ackSets)?
```java
return LongArrayAckSets.cardinalityOfDifference(cursorAckSet, ackSets);
public static int cardinalityOfDifference(long[] ackSet1, long[] ackSet2) {
int sum = 0;
int commonLength = Math.min(ackSet1.length, ackSet2.length);
for (int i = 0; i < commonLength; i++) {
sum += Long.bitCount(ackSet1[i] & ~ackSet2[i]);
}
for (int i = commonLength; i < ackSet1.length; i++) {
sum += Long.bitCount(ackSet1[i]);
}
return sum;
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
