lhotari commented on PR #26010:
URL: https://github.com/apache/pulsar/pull/26010#issuecomment-4696644063
I performed a local review with Claude Code and these were the findings:
1. **[QUALITY] Dead writes in `add()`**
The three `array.writeLong(arrayIdx, …)` calls before
`siftUp(tuplesCount, n1, n2, n3)` are redundant. `siftUp` never reads the slot
at `tupleIdx` (it only reads parents) and unconditionally writes `(n1, n2, n3)`
at the final hole position — which equals `arrayIdx` when no movement occurs.
Dropping them saves 3 `writeLong` calls per `add()`, exactly the kind of saving
this PR is after. (`SegmentedLongArray.writeLong` is a plain `setLong` with no
size/writer-index tracking, so nothing depends on the pre-write.)
2. **[QUALITY] Single-element `pop()` does wasted I/O**
`pop()` reads the last (= only) tuple and `siftDown` writes it back to
slot 0, which is now beyond `tuplesCount`. Harmless (the old code's `swap(0,
0)` was worse), but an early return when `tuplesCount == 0` after the decrement
would skip 3 reads + 3 writes. Very minor.
3. **[QUALITY] No test changes for a heap-algorithm rewrite**
Existing coverage (`testLargeQueue`, `testCompareWithSamePrefix`) does
exercise ordering, but for a from-scratch sift rewrite a randomized
differential test against `java.util.PriorityQueue<long[]>` (interleaved
add/pop, duplicates, same-prefix tuples) would be cheap insurance. Worth
considering, not blocking.
--
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]
