You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "Asthenia0412 (via GitHub)" <gi...@apache.org> on 2026/10/17 00:42:50 UTC
[PR] [improve][broker] Reduce allocations on dispatch and managed-ledger read hot paths [pulsar]
Asthenia0412 opened a new pull request, #26956:
URL: https://github.com/apache/pulsar/pull/26956
## 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.
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
- Add ManagedCursorImpl.isMessageDeleted(long, long) overload that avoids creating a Position by using Position.compareTo(long, long).
- Add OpReadEntry.skipOnlyMessageDeleted boolean flag, set true when the original caller did not supply a custom skipCondition.
- 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 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.
- AbstractBaseDispatcher.filterEntriesForConsumer: call the new overload.
## 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
- :managed-ledger:compileJava / :compileTestJava
- :pulsar-broker:compileJava / :compileTestJava
- :pulsar-broker:test EntryBatchIndexesAcksTest passed
- :managed-ledger:test PositionRangeSetTest, ManagedCursorTest passed
--
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
Re: [PR] [improve][broker] Reduce allocations on dispatch and managed-ledger read hot paths [pulsar]
Posted by "Asthenia0412 (via GitHub)" <gi...@apache.org>.
Asthenia0412 commented on PR #26956:
URL: https://github.com/apache/pulsar/pull/26956#issuecomment-6237225017
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!
--
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
Re: [PR] [improve][broker] Reduce allocations on dispatch and managed-ledger read hot paths [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26956:
URL: https://github.com/apache/pulsar/pull/26956#discussion_r4318954122
##########
pulsar-broker/src/test/java/org/apache/pulsar/functions/worker/WorkerUtilsBookKeeperPackageTest.java:
##########
@@ -49,15 +49,17 @@ public class WorkerUtilsBookKeeperPackageTest {
private LocalBookkeeperEnsemble bkEnsemble;
private Namespace dlogNamespace;
+ 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);
Review Comment:
This looks like an unrelated change
--
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
Re: [PR] [improve][broker] Reduce allocations on dispatch and managed-ledger read hot paths [pulsar]
Posted by "Asthenia0412 (via GitHub)" <gi...@apache.org>.
Asthenia0412 commented on PR #26956:
URL: https://github.com/apache/pulsar/pull/26956#issuecomment-6237234481
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!
--
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
Re: [PR] [improve][broker] Reduce allocations on dispatch and managed-ledger read hot paths [pulsar]
Posted by "Asthenia0412 (via GitHub)" <gi...@apache.org>.
Asthenia0412 commented on PR #26956:
URL: https://github.com/apache/pulsar/pull/26956#issuecomment-6245569202
Rebased onto upstream/master (no conflicts). ReadOnlyCursor fix (`e611a2d`) — overriding `isMessageDeleted(long, long)` to preserve subclass semantics when the skip shortcut is active — remains in HEAD. Force-pushed updated branch for fresh CI. Thanks for your review!
--
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
Re: [PR] [improve][broker] Reduce allocations on dispatch and managed-ledger read hot paths [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui merged PR #26956:
URL: https://github.com/apache/pulsar/pull/26956
--
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
Re: [PR] [improve][broker] Reduce allocations on dispatch and managed-ledger read hot paths [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #26956:
URL: https://github.com/apache/pulsar/pull/26956#discussion_r4301066166
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2643,8 +2643,16 @@ private void internalReadFromLedger(ReadHandle ledger, OpReadEntry opReadEntry)
long firstValidEntry = -1L;
long lastValidEntry = -1L;
long entryId = firstEntry;
+ final long ledgerId = ledger.getId();
for (; entryId <= lastEntry; entryId++) {
- if (opReadEntry.skipCondition.test(PositionFactory.create(ledger.getId(), entryId))) {
+ boolean shouldSkip;
+ if (opReadEntry.skipOnlyMessageDeleted) {
+ // Fast path: only cursor's isMessageDeleted check, no Position allocation
+ shouldSkip = opReadEntry.cursor.isMessageDeleted(ledgerId, entryId);
Review Comment:
**[P1] Preserve read-only cursor deletion-check semantics**
This bypasses `ReadOnlyCursorImpl.isMessageDeleted(Position)`, which returns false. Read-only cursors leave `markDeletePosition` unset, so the inherited primitive overload throws on ordinary nonempty reads. The existing `ReadOnlyCursorTest.simple` reproduces this failure. Please preserve subclass deletion-check semantics when enabling this shortcut.
--
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
Re: [PR] [improve][broker] Reduce allocations on dispatch and managed-ledger read hot paths [pulsar]
Posted by "Asthenia0412 (via GitHub)" <gi...@apache.org>.
Asthenia0412 commented on code in PR #26956:
URL: https://github.com/apache/pulsar/pull/26956#discussion_r4307073732
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2643,8 +2643,16 @@ private void internalReadFromLedger(ReadHandle ledger, OpReadEntry opReadEntry)
long firstValidEntry = -1L;
long lastValidEntry = -1L;
long entryId = firstEntry;
+ final long ledgerId = ledger.getId();
for (; entryId <= lastEntry; entryId++) {
- if (opReadEntry.skipCondition.test(PositionFactory.create(ledger.getId(), entryId))) {
+ boolean shouldSkip;
+ if (opReadEntry.skipOnlyMessageDeleted) {
+ // Fast path: only cursor's isMessageDeleted check, no Position allocation
+ shouldSkip = opReadEntry.cursor.isMessageDeleted(ledgerId, entryId);
Review Comment:
@codelipenghui 已在 commit eebcbf1b95（ReadOnlyCursorImpl.java#L88）添加 override：`isMessageDeleted(long ledgerId, long entryId)` 返回 `false`，与 `isMessageDeleted(Position)` 语义一致。ManagedLedgerImpl 快路径（L2786）现在正确走到 read-only 语义。请帮忙 Resolve 此 thread。
--
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
Re: [PR] [improve][broker] Reduce allocations on dispatch and managed-ledger read hot paths [pulsar]
Posted by "Asthenia0412 (via GitHub)" <gi...@apache.org>.
Asthenia0412 commented on PR #26956:
URL: https://github.com/apache/pulsar/pull/26956#issuecomment-6275559159
Hi @codelipenghui — I've resolved the read-only cursor regression with an override `isMessageDeleted(long, long)` on `ReadOnlyCursorImpl` that mirrors the always-false semantics of the existing `Position` variant. The ManagedLedgerImpl fast path at L2786 now correctly short-circuits before the deleted-entry check for read-only cursors (which never set markDeletePosition).
Pre-existing `ReadOnlyCursorTest.simple` passes locally against this head. CI is green. Could you re-review when you have a moment? Thank you! 🙏
--
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