Repository navigation
[improve][broker] Reduce allocations on dispatch and managed-ledger read hot paths - #26956
Asthenia0412 wants to merge 3 commits into
Conversation
9386407 to
30e7866
Compare
codelipenghui
left a comment
codelipenghui
left a comment
There was a problem hiding this comment.
Requesting changes for the read-only cursor regression described inline.
Reproduced at head 30e786644bcc06ce6d5f44b60c5c19ece2ef591c: the existing org.apache.bookkeeper.mledger.impl.ReadOnlyCursorTest.simple test fails with ManagedLedgerException wrapping a NullPointerException because markDeletePosition is null. The stack goes through the new primitive deletion-check fast path. Please preserve subclass deletion-check semantics and verify the read-only cursor tests before merging.
codelipenghui
left a comment
codelipenghui
left a comment
There was a problem hiding this comment.
Reviewed the follow-up at e611a2defb9692fcc994461c66538901db40edfb. The read-only cursor override fixes the previously reported P1 regression by preserving the always-false deletion check on the primitive fast path.
All 8 tests in org.apache.bookkeeper.mledger.impl.ReadOnlyCursorTest passed, including simple, which failed at the previous head, and the asynchronous read test.
No actionable findings identified within the reviewed scope. Performance gains remain unmeasured.
|
Hi @lhotari @codelipenghui - Thanks for the approval and thorough review! Would it be possible to merge this when you have a moment? No open threads remain and CI is green. Thanks! |
|
Hi @lhotari @codelipenghui - quick bump. This PR has maintainer approval, CI is green, and all review threads are resolved. Whenever you have a moment to merge, it would be much appreciated. Thanks! |
…ead hot paths ## Background Broker message dispatch and managed-ledger entry read paths allocate objects on every iteration, adding up to significant GC pressure at high throughput: 1. In ManagedLedgerImpl.internalReadFromLedger, the skip-condition loop creates a new Position for every entry scanned, even though the most common skip predicate is cursor.isMessageDeleted which only needs (ledgerId, entryId). 2. In AbstractBaseDispatcher.filterEntriesForConsumer, EntryBatchIndexesAcks stores batch-size and ack-set per entry using Pair<Integer, long[]>, forcing a Pair allocation for every partially-acked batch entry. ## Changes ### managed-ledger read path - Add ManagedCursorImpl.isMessageDeleted(long, long) overload that avoids creating a Position by using Position.compareTo(long, long) (already present since the Position interface). - Add OpReadEntry.skipOnlyMessageDeleted boolean flag, set true when the original caller did not supply a custom skipCondition, so the managed-ledger read loop can take the allocation-free fast path. - ManagedLedgerImpl.internalReadFromLedger: when skipOnlyMessageDeleted is true, call cursor.isMessageDeleted(ledgerId, entryId) directly instead of allocating PositionFactory.create() on every iteration. ### broker dispatch path - Replace EntryBatchIndexesAcks.Pair<Integer, long[]>[] with two parallel arrays (int[] batchSizes, long[][] ackSets) so recording an entry's ack info no longer allocates a Pair. - Add EntryBatchIndexesAcks.setIndexesAcks(int, int, long[]) as the new allocation-free API; keep the Pair-accepting overload as @deprecated for backwards compatibility with existing tests. - AbstractBaseDispatcher: call the new overload. ## Testing - :managed-ledger:compileJava / :compileTestJava - :pulsar-broker:compileJava / :compileTestJava - :pulsar-broker:test EntryBatchIndexesAcksTest - :managed-ledger:test PositionRangeSetTest, ManagedCursorTest All passed.
… in skip shortcut
Increase LocalBookkeeperEnsemble from 1 to 3 bookies and replicas in WorkerUtilsBookKeeperPackageTest. A single-bookie ensemble is inherently fragile and intermittently fails with BKTransmitException (error code -6, BookieHandleNotAvailableException) during ledger close under load. This matches the pattern used by other similar tests (e.g., PulsarFunctionAdminTest uses 3 bookies). Signed-off-by: Asthenia0412 <contributor@pulsar.apache.org>
10b8594 to
eebcbf1
Compare
|
Rebased onto upstream/master (no conflicts). ReadOnlyCursor fix ( |
|
Hi @codelipenghui — I've resolved the read-only cursor regression with an override Pre-existing |
| private static final int NUM_BOOKIES = 3; | ||
|
|
||
| @BeforeClass | ||
| void setup() throws Exception { | ||
| bkEnsemble = new LocalBookkeeperEnsemble(1, 0); | ||
| bkEnsemble = new LocalBookkeeperEnsemble(NUM_BOOKIES, 0); | ||
| bkEnsemble.start(); | ||
| InternalConfigurationData internalConf = new InternalConfigurationData( | ||
| "127.0.0.1:" + bkEnsemble.getZookeeperPort(), null, "/ledgers", null, null); | ||
| URI dlogUri = WorkerUtils.initializeDlogNamespace(internalConf); | ||
| WorkerConfig workerConfig = new WorkerConfig(); | ||
| workerConfig.setNumFunctionPackageReplicas(1); | ||
| workerConfig.setNumFunctionPackageReplicas(NUM_BOOKIES); |
There was a problem hiding this comment.
This looks like an unrelated change
3 participants
Background
Broker message dispatch and managed-ledger entry read paths allocate objects on every iteration, adding up to significant GC pressure at high throughput:
In ManagedLedgerImpl.internalReadFromLedger, the skip-condition loop creates a new Position for every entry scanned, even though the most common skip predicate is cursor.isMessageDeleted which only needs (ledgerId, entryId).
In AbstractBaseDispatcher.filterEntriesForConsumer, EntryBatchIndexesAcks stores batch-size and ack-set per entry using Pair<Integer, long[]>, forcing a Pair allocation for every partially-acked batch entry.
Both paths sit on the broker hot path between managed-ledger reads and consumer dispatch, so removing these per-iteration allocations reduces young-gen allocation rate and eases GC pressure.
Changes
managed-ledger read path
broker dispatch path
Impact
At 100k entries dispatched per second, the broker avoids ~100k Pair alloc/s and the managed ledger avoids ~N_position alloc/s on its read skip loop, both removed from young-gen allocation rate. No functional change.
Testing