Skip to content

[improve][broker] Track in-progress request frame memory - #27110

Open
poorbarcode wants to merge 13 commits into
apache:masterfrom
poorbarcode:improve_maxMessagePublishBufferSizeInMB
Open

poorbarcode wants to merge 13 commits into
apache:masterfrom
poorbarcode:improve_maxMessagePublishBufferSizeInMB

Conversation

@poorbarcode

@poorbarcode poorbarcode commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Inbound publish requests are decoded from Netty pooled direct buffers. A small retained frame can pin a much larger cumulation allocation, while the existing publish-buffer limiter accounts for logical payload bytes rather than retained root-buffer capacity. This can leave substantial direct memory retained by in-progress requests before the broker applies backpressure.

Modifications

This change tracks the capacity of direct-memory root buffers retained by inbound request frames, including the extra capacity pinned by small slices. It adds configurable broker-wide backpressure with three modes: metrics only (DISABLED), automatic memory-pressure protection (AUTO, the default), and forced enforcement (ENABLED). Separate retained-memory and active-throttling metrics make both memory use and the limiter's current state observable.

  • Always install InProgressRequestFrameTrackingDecoder, preserving accounting across slices, duplicates, cumulation replacement/expansion, and final release on any thread.
  • Share one request-frame limiter across all connections. Dynamic updates of maxMessagePublishBufferSizeInMB re-evaluate existing and future connections against the latest broker-wide threshold; the existing payload-byte limiter keeps its original connection-time configuration behavior.
  • Make the mode dynamically configurable. DISABLED maintains accounting but skips policy evaluation; AUTO checks direct-memory pressure only when activation or recovery is possible; ENABLED applies the retained-capacity threshold regardless of direct-memory pressure. Boolean aliases remain accepted: false maps to AUTO, and true to ENABLED.
  • Keep atomic capacity accounting and validated optimistic state reads on the steady path. Concentrate write locking on state transitions and registration, invoke listeners outside that lock, and coalesce connection notifications into one pending task per EventLoop.
  • Separate socket-read backpressure from delivery of already-decoded frames in PulsarFlowControlHandler. Request-frame memory throttling stops new socket reads while allowing queued frames to complete and release their roots; other throttle reasons continue to pause queue delivery. Recheck those reasons after each frame and permit bounded continuation reads for an incomplete frame when memory throttling is the only reason for the pause.
  • Export pulsar.broker.publish.in.progress.buffer.memory.usage (retained root capacity in bytes) and pulsar.broker.publish.in.progress.buffer.memory.throttled (1 while this limiter is throttling, otherwise 0). The latter describes active request-frame throttling, not whether its policy is enabled and not other broker throttling reasons.
  • Add decoder, mode-transition, dynamic-threshold, lifecycle, notification, flow-control, and metric tests. Keep dynamic managedLedgerCacheCopyEntries implementation and its managed-ledger tests in the independent PR [improve][broker] Make entry cache copying dynamically configurable #27148.

Performance comparison: no PR vs. the complete PR

The comparison uses 17166c820bf011ce7dd8b2b6795cc9c62e734af7 (without this PR, its common ancestor) and ef2a4475722d94c0d673a317f9c0d82993c56251 (the complete PR, including the three modes, shared dynamic threshold, flow-control fix, and throttling metric). The harness, clients and Dockerfile are identical across these revisions. Both images were built before measurement, and each report records its exact commit and a clean checkout. These numbers replace the previous performance table.

All runs used one native Linux x86_64 GitHub-hosted VM: AMD EPYC 7763, 4 vCPUs, approximately 16 GiB RAM, Ubuntu 24.04.5 and Docker 28.0.4. Images used Alpine 3.24.3, Amazon Corretto 25.0.5 and the Pulsar Dockerfile's mimalloc/gcompat preload. Both sides enabled the OpenTelemetry Prometheus exporter with the same 1-second scrapes. No emulation or profiling was used. The earlier experiment used a different host and telemetry configuration; its numbers are not used as this experiment's baseline.

The IoT workload used 10,000 devices, 10 gateway clients, 8 topics, and one application with one Key_Shared consumer client. One broker, one bookie (E/W/A=1), and ZooKeeper shared the host. Broker and bookie each had 512 MiB heap and 512 MiB direct-memory limits. Batching was disabled; broker IO threads=4; maxMessagePublishBufferSizeInMB=256; managedLedgerCacheCopyEntries=false. The bookie journal used the framework's 2 GiB tmpfs overlay with journal sync disabled. This is a test topology, not a production durability configuration.

There are 12 completed small-message runs: 128 B payloads at 20,000 msg/s, with a global outstanding-send window of 1,024, 20 seconds of warmup and 60 seconds of measurement. Each variant has three runs, ordered no-PR/DISABLED/AUTO/ENABLED; ENABLED/AUTO/DISABLED/no-PR; AUTO/no-PR/ENABLED/DISABLED. Resolved configurations match apart from the PR-only mode and generated service URL.

The experiment also completed seven 128 KiB runs at 500 msg/s (window=128; same warmup and measurement duration). Their repetition counts are unequal, so they are reported separately as exploratory observations. Two short metric-collection checks are excluded from performance results. The next large-message baseline run was interrupted by cancellation and has no complete measurement report; it is excluded. Neither unpaced scenario was started.

128 B: three runs per variant

Values are median [minimum–maximum] across runs. Publish p99 is the median of per-run p99s, not a pooled percentile. Broker CPU comes from cgroup measurements normalized per million measured messages. Throughput is higher-is-better; latency and CPU per message are lower-is-better.

The manual-enable and throttle columns refer only to request-frame memory limiting; the legacy payload-byte limiter remains enabled throughout. The limit column is maxMessagePublishBufferSizeInMB. Throttle observations count samples equal to 1 over samples within the publish measurement windows. Zero samples mean not observed, and do not rule out a throttle shorter than the sampling interval.

Revision / mode Manually enabled? Observed request-frame throttling Limit (MiB) Publish msg/s Publish p99 (ms) Broker CPU (s/M msgs) CPU change
Without PR N/A N/A (absent) 256 19,999 [19,997–20,000] 61.3 [60.1–77.2] 75.63 [75.53–75.68] —
DISABLED No Not observed (0/180) 256 19,999 [19,998–20,000] 58.2 [58.1–77.7] 77.41 [76.38–77.94] +1.78 (+2.35%)
AUTO No Not observed (0/180) 256 19,999 [19,997–19,999] 70.8 [68.0–92.5] 78.20 [76.98–78.45] +2.57 (+3.40%)
ENABLED Yes Not observed (0/179) 256 19,998 [19,994–20,000] 86.0 [63.6–87.2] 77.82 [77.80–77.87] +2.20 (+2.90%)

Delivery p99 medians [min–max] were 77.1 [74.0–98.0] ms without the PR; 75.0 [74.0–96.1] ms in DISABLED; 91.0 [88.1–123.0] ms in AUTO; and 109.1 [90.0–109.1] ms in ENABLED. Maximum observed retained-frame capacities across the three runs were 39.58, 37.80 and 39.14 MiB respectively; these are sampled values, not exact peaks.

128 KiB: incomplete repetition set, exploratory only

The baseline has only one completed run and each PR mode has two. The ranges below are individual-run observations, not a statistically supported comparison or an estimated PR speedup/regression. All seven runs sustained approximately 500 msg/s.

Revision / mode Completed runs Manually enabled? Observed request-frame throttling Limit (MiB) Publish p99 (ms), min–max Broker CPU (s/M msgs), min–max
Without PR 1 N/A N/A (absent) 256 2.9 889.73
DISABLED 2 No Not observed (0/120) 256 3.2–3.2 866.62–898.58
AUTO 2 No Not observed (0/120) 256 3.1–3.1 884.07–903.56
ENABLED 2 Yes Not observed (0/120) 256 3.3–3.7 874.88–906.61

Maximum sampled retained-frame capacities were 162.25 MiB in DISABLED, 159.50 MiB in AUTO and 162.50 MiB in ENABLED.

What the results establish

  • At the fixed small-message offered rate, all variants sustained the target throughput. The PR increased median broker CPU per million messages by 2.35% in DISABLED, 3.40% in AUTO and 2.90% in ENABLED. Each candidate CPU range is above the baseline range on this host. These are observed fixed-load costs of the complete PR, not a claim of improved maximum throughput.
  • The small-message p99 ranges overlap the baseline, and whole-host CPU usage was approximately 98.5–99.0%. The data do not establish a stable latency difference or a reliable performance ranking between the three PR modes. Three repetitions and a shared VM do not establish statistical significance or production-wide effects.
  • No request-frame throttle was observed in the 899 candidate measurement-window samples. ENABLED means the policy was active, not that its threshold was crossed. The observed latency differences cannot be attributed to throttling from these samples; short unsampled transitions are still possible. The benchmark does not validate automatic activation/recovery under memory pressure or reproduce the original retention failure.
  • Across the 19 completed measured runs, all 19,480,000 messages, including warmup (14,610,000 measured), were delivered with zero duplicates, ordering violations or invalid messages. Recorded broker CPU counters did not reset. Retained logs show no OOM, and no profiling or heap-dump artifacts were produced. The cancelled run is not included in these totals.
  • This is a short, fixed-load comparison on a shared host. It does not measure maximum throughput. Host validation recorded inactive TuneD, swappiness=60, perf_event_paranoid=4 and THP=always; the governor was performance, with no exposed turbo control. CPU frequency and thermal-throttle samples were unavailable. Host tuning was not changed. Without profiles, these results cannot isolate accounting, locks, allocations, GC or scheduler contention as the cause of an observed change.

Reproduction and evidence

The comparison workflow was intentionally stopped after the runs above; its overall status is cancelled. The pinned driver and workflow are on a separate experimental branch, outside this PR. The complete small-message comparison can be reproduced with the driver phases prepare, verify-metrics, and fixed-small on one Linux amd64 host.

The saved raw-results artifact, request-frame-modes-comparison, is currently retained until 2026-11-27. It contains the manifest, reports, resolved configurations, exact commits/commands, host and image metadata, latency histograms, cgroup CPU summaries, memory metric JSON, and timestamped retained-frame/throttle samples. Only manifest entries with measurementRun=true and valid=true are included above. Use each entry's report path and the producer measurement timestamps in gateways-summary.json to reproduce the filtering.

Does this pull request potentially affect one of the following parts:

  • Broker configuration
  • Metrics
  • Public API
  • Wire protocol

Best practice

Use inProgressRequestFrameMemoryLimitEnabled=AUTO (the default) for broker-local memory-pressure protection. It activates when direct-memory usage reaches at least 85% and retained frame capacity exceeds 1.5 × maxMessagePublishBufferSizeInMB; enforcement starts on the next inbound read. Automatic protection turns off when direct-memory usage falls below 65%. Use ENABLED to enforce the retained-frame threshold regardless of direct-memory pressure, or DISABLED for metrics-only operation with no automatic activation.

When this policy is active, all connections share the latest dynamically configured maxMessagePublishBufferSizeInMB: retained frame capacity above the limit pauses socket reads, and capacity at or below half the limit clears that throttle. A nonpositive value disables this limiter. Dynamic mode and numeric-limit changes affect existing connections; final buffer releases drive recovery without a periodic polling task. Other broker throttle reasons remain independent.

Monitor both pulsar.broker.publish.in.progress.buffer.memory.usage and pulsar.broker.publish.in.progress.buffer.memory.throttled through the configured OpenTelemetry exporter. The first reports retained root-buffer capacity, not allocator-reserved memory or RSS; the second reports current request-frame throttling (0/1), not policy enablement. Enable managedLedgerCacheCopyEntries independently when EntryCache may retain inbound slices; support for dynamically changing that existing setting is provided by #27148.

@poorbarcode
poorbarcode marked this pull request as draft October 26, 2026 07:03
@poorbarcode
poorbarcode marked this pull request as ready for review October 26, 2026 14:16

@codelipenghui codelipenghui left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked the outstanding findings against head 346ec2490abbd78978c3d8646638cacff54cc6b0.

The new shared root-reference accounting, capacity reconciliation, and latest-state notifications address the earlier pooled-buffer, growth, and stale-notification reports by source inspection; those comments are omitted.

Requesting changes for two remaining issues:

  • Memory backpressure can prevent already-buffered frames from draining, even though those frames retain the allocations needed to clear the throttle.
  • Concurrent cache creation can overwrite a newer dynamic cache-copy setting with the old value.

Validation scope: 18 supplied focused tests passed on the previously reviewed head 8d750c0afe9ef95d271ea11ee0000f2dc0949ebe. The queued-frame stall was reproduced there using the actual decoder, flow-control handler and throttle tracker with a mocked connection; the original-decoder control passed. The latest revision was source-rechecked only, not retested locally. The cache race is based on the concrete interleaving described inline.

Comment on lines +182 to +184
if (throttled) {
serverCnx.getThrottleTracker().markThrottled(
ServerCnxThrottleTracker.ThrottleType.InProgressRequestFrameMemoryExceeded);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve progress for already-buffered frames under memory backpressure

This throttle disables auto-read, which also prevents PulsarFlowControlHandler from draining its existing queue. A receive allocation can contain frames A and B: if another throttle (for example, TopicPublishRate) stops delivery after A, B stays queued and retains the allocation. Once the scheduled memory-throttle update runs, releasing A and clearing the original throttle cannot resume delivery: B keeps retained memory above the resume watermark, while B cannot be processed until the memory throttle clears.

Deferring the update until after the current inbound callback does not solve this, because another throttle can leave that callback's decoded frames queued. This was reproduced on the earlier revision with the supported unpooled allocator; the current controller still enforces the same circular dependency.

Please separate stopping additional socket intake from draining already-accounted frames, or otherwise guarantee progress for the work retaining those allocations. Add a regression combining memory pressure, another throttle reason, and a queued tail sharing the receive allocation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The queued-frame progress fix is now included in bd7903c4e3, with the combined pooled-sibling/Topic/memory-throttle regression. Socket intake and queued-message delivery are controlled separately, while per-message business throttling is preserved. Details are in the updated follow-up reply; this fix no longer depends on #27126.

// The dynamic setting can change between constructing and publishing this cache. Re-read the volatile
// snapshot after insertion so that a cache missed by updateCopyEntriesInCache's concurrent iteration
// cannot remain on the old mode.
newEntryCache.setCopyEntries(copyEntriesInCache);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Serialize cache publication with copy-mode updates

The volatile read and setCopyEntries call are separate operations, so this can still leave the newly published cache on the old mode:

  1. The creator reads copyEntriesInCache == false as the setter argument and is descheduled.
  2. updateCopyEntriesInCache(true) stores the new mode and visits this already-published cache, setting it to true.
  3. The creator resumes and calls setCopyEntries(false).

Both methods then return with the manager/configuration set to true but this cache set to false. Future inserts continue retaining receive buffers despite copying being enabled; the sequential tests do not cover this interleaving.

Please serialize publication/mode assignment with updates, or have inserts consult a single manager-owned volatile policy instead of maintaining independently updated per-cache copies.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This race was fixed by using one shared volatile copy policy for cache inserts, with a regression for the publication/update interleaving. The implementation and tests have since been split out of this PR into #27148, which is now merged. See the updated follow-up reply.

@codelipenghui codelipenghui left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up validation of 346ec2490abbd78978c3d8646638cacff54cc6b0, updating the earlier source-only assessment of this head.

Focused validation now confirms the fixes for pooled-root accounting, in-place capacity growth, and stale throttle notifications. Thirty-five cases passed: 16 decoder cases, 14 controller cases, two broker integration tests, and three adapted local regression probes.

The same two outstanding findings remain:

  • P1: queued-frame progress still fails on this exact head, now reproduced with PooledByteBufAllocator.DEFAULT. The original-decoder control delivers both frames; the tracking decoder delivers only one after the other throttle clears.
  • P2: cache creation can still overwrite a newer dynamic copy-mode setting. This remains a source-established race; no runtime concurrency reproduction is claimed.

Requesting changes for these two issues. The queue regression uses EmbeddedChannel, the real decoding/flow-control/throttle components, and a mocked ServerCnx; it is not a production workload reproduction. No additional confirmed defects were identified in the reviewed delta.

Comment on lines +182 to +184
if (throttled) {
serverCnx.getThrottleTracker().markThrottled(
ServerCnxThrottleTracker.ThrottleType.InProgressRequestFrameMemoryExceeded);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Follow-up: queued-frame stall reproduced on this head with pooled buffers

I reran the combined-throttle regression against 346ec2490a using PooledByteBufAllocator.DEFAULT, the actual decoder, PulsarFlowControlHandler, and ServerCnxThrottleTracker with a mocked connection. After releasing the first frame and clearing TopicPublishRate, the tracking case still delivered only one of two frames; the original length-field-decoder control delivered both.

The circular dependency remains: the queued tail retains the charged root allocation, but this throttle disables auto-read, which also prevents the flow-control queue from draining. Deferring the notification does not help when another throttle has already queued that tail.

Please separate stopping additional socket intake from draining already-accounted frames, or otherwise ensure that the work retaining the allocation can complete. This needs a regression combining memory pressure with another throttle reason and pooled sibling frames.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is proposed separately in #27126, which removes PulsarFlowControlHandler from the broker pipeline. Throttling then pauses subsequent automatic socket reads while complete frames already read continue to ServerCnx. This eliminates the queued-tail dependency that keeps an allocation alive while waiting for the memory throttle to clear.

#27126 is still open, so this PR remains dependent on that change. After it is merged, it needs to be integrated here and the combined tracking-decoder/other-throttle regression rerun. This issue is not yet resolved in the current head of this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that removing the flow-control queue can break the circular dependency here. However, #26013 restored per-message throttling for a real publish-rate regression, so #27126 changes an existing behavior that needs careful validation.

I'd keep this blocked until #27126 is integrated and the combined-throttle regression passes on the updated head. We should also verify the impact on publish-rate overshoot, pending publish requests, and memory usage under realistic workloads.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update for @codelipenghui and @Denovo1998: the fix is now included in this PR in bd7903c4e3. This supersedes my earlier reply about depending on #27126; this fix no longer requires removing the flow-control handler.

PulsarFlowControlHandler and the existing handler order are retained. Request-frame memory throttling pauses new socket intake, while already-decoded frames can still be delivered when no business throttle is active. Clearing the last Topic/pending-publish/other business throttle schedules a drain of the existing queue even while memory throttling keeps auto-read disabled. Delivery rechecks business throttles for every frame, and draining an empty queue does not request more socket data.

The regression tests include the reported pooled sibling-frame case with Topic throttling, both release orders for every business throttle, throttling again during a drain, incomplete-frame continuation, and connection closure. They use the real decoder/flow-control/tracker components with an EmbeddedChannel and a mocked ServerCnx.

This addresses the queued-tail circular dependency while preserving per-message business backpressure. The design is also documented in PIP-499. The fixed-load performance results in the PR description do not establish publish-rate overshoot or pending-publish behavior under production memory pressure; those results should not be read as that broader validation.

// The dynamic setting can change between constructing and publishing this cache. Re-read the volatile
// snapshot after insertion so that a cache missed by updateCopyEntriesInCache's concurrent iteration
// cannot remain on the old mode.
newEntryCache.setCopyEntries(copyEntriesInCache);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Follow-up: cache copy-mode publication race remains unchanged

The creator can read false as this setter's argument and then be descheduled. Meanwhile, updateCopyEntriesInCache(true) stores the new policy and visits this already-published cache, setting it to true. When the creator resumes, setCopyEntries(false) overwrites that update. Both methods then return with configuration true but the new cache still retaining receive buffers on future inserts.

This read-then-set sequence is not atomic with the updater's iteration; making the fields volatile does not prevent the interleaving. The relevant code is unchanged in this revision. This finding is based on source analysis, not an executed race test.

Please serialize publication/mode assignment with updates, or make inserts consult one manager-owned volatile copy policy, and add concurrent creation/update coverage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 74852f3, with the exact publication-race regression added in cb808b5.

Caches no longer keep separate copies of the policy. Single-entry and batch insertions use the shared volatile ManagedLedgerFactoryConfig.copyEntriesInCache through the manager, eliminating both the publication-time setter and per-cache update propagation. Existing cached entries are preserved, and BookKeeper batch reads still force copying.

The new regression pauses the creator at the legacy setter entry, after its argument has been evaluated but before assignment. It completes one concurrent configuration update, verifies that the cache uses the new mode, then resumes the creator and checks actual buffer reference counts again. Construction interception delegates operations to a real cache; no production test hook or private-state manipulation is needed. Both enable/disable cases fail against the old implementation and pass with the fix.

Validation: all 40 related tests passed with retries disabled, and full ./gradlew quickCheck passed, including main/test Checkstyle, Spotless, and RAT.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: the shared copy-policy fix and its publication-race regression were split into #27148, which has now been merged.

Cache inserts consult the shared volatile policy instead of receiving separately propagated per-cache values, so the stale publication-time assignment described here is eliminated. The existing managedLedgerCacheCopyEntries setting remains independently configurable; updates affect future inserts without clearing existing cached entries.

The EntryCache implementation and tests were removed from this PR in 81730ae. They are reviewed and maintained in the separate, merged change.

Read a shared volatile copy policy on cache insertion instead of propagating per-cache snapshots that can overwrite dynamic updates. Preserve existing entries and forced copying of batch reads, and cover concurrent cache creation, overlapping updates, and asynchronous read completion.

Assisted-by: Codex
Reproduce a captured stale setter argument with one concurrent policy update in each direction. Delegate inserts to a real cache and assert buffer reference counts before and after the creator resumes.

Assisted-by: Codex

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please file an issue to show exactly which problem this is addressing. In the issue, it would also be useful to share the reproducer of the issue.

This change might have a significant performance impact and before applying this change, there would need to be careful performance analysis. This could be tested with tests/performance scenarios on Linux x64_64/amd64 architecture and the actual Docker image since the malloc implementation also matters.

I think that this change needs a PIP since it's also labeled for 4.0.x and 4.2.x.

It also allows EntryCache to copy future entries so cached slices do not pin large inbound allocations.

This should be a separate change and also configurable.

…change

Remove the managed-ledger copy policy updates and their tests from the request-frame memory tracking change. Keep the frame limiter lifecycle coverage independent of dynamic cache configuration.

Assisted-by: Codex
@poorbarcode

Copy link
Copy Markdown
Contributor Author

@lhotari, following up on your request to split out the EntryCache change:

This should be a separate change and also configurable.

Moved the EntryCache dynamic configuration, shared copy-policy implementation, and related tests into #27148 (dynamic_entry_cache), which is based on master and can be reviewed and merged independently. It uses the existing managedLedgerCacheCopyEntries setting, keeps its default false, and applies dynamic updates to future cache inserts without clearing existing cached entries.

Removed those changes from this PR in 81730ae and updated the description accordingly. This PR now contains only the request-frame memory tracking and limiting changes.

Use validated optimistic state checks between transitions and coalesce connection notifications per event loop. Keep exact global accounting, generation thresholds, and release-driven recovery. Add concurrent recovery and notification lifecycle tests and a shared-controller JMH benchmark.

Assisted-by: Codex
Sample direct memory outside the lock and validate the optimistic stamp before skipping unchanged automatic state. Include disabled, manual, and automatically enabled modes in the shared-controller benchmark.

Assisted-by: Codex
Exercise 1,000 connection callbacks on a real Netty event loop and verify the last release from another thread resumes all of them without another inbound event. Always shut down the event loop on failure.

Assisted-by: Codex
Comment on lines +139 to +143
InProgressRequestFrameMemoryLimiter inProgressRequestFrameMemoryLimiter =
pulsar.getBrokerService().getInProgressRequestFrameMemoryLimiter();
ch.pipeline().addLast(FrameDecoderUtil.FRAME_DECODER_HANDLER,
new InProgressRequestFrameTrackingDecoder(brokerConf.getMaxMessageSize(),
inProgressRequestFrameMemoryLimiter));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't add this handler unless inProgressRequestFrameMemoryLimitEnabled=true. This avoids regressions for users that don't need this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping the tracking decoder installed is intentional: continuously measuring retained request-frame memory is one of the goals of PIP-499. See Accounting for request-frame memory and Behavior when forced off.

DISABLED turns off request-frame throttling and automatic activation, but preserves accounting and the metric so operators can observe retention amplification while the limiter is off. Replacing the tracking decoder with the ordinary decoder in that mode would remove the information that this metric is intended to expose. The disabled path skips policy checks and direct-memory-utilization sampling; it still pays the accounting cost.

That cost is now reported in the PR description: in the native Linux amd64 / actual Docker-image fixed-load comparison, DISABLED used 2.35% more broker CPU per million messages than the no-PR baseline (three runs per variant). Keeping metrics available has a measured cost; the always-on accounting requirement is documented in the PIP for design review.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a very high risk that this introduces regressions. I don't see a reason why this feature couldn't be completely disabled. I'm strongly against adding such a feature that is on the hot path of Pulsar and it includes a lot of synchronization.

@lhotari lhotari left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inbound publish requests are decoded from Netty pooled direct buffers.

This is very different with master/branch-5.0. The default is AdaptiveByteBufAllocator. With adaptive allocator, less memory will be wasted compared to PooledByteBufAllocator.
Another important detail is that using an Alpine base image results in higher memory consumption (RSS) due to the poor malloc implementation. In Pulsar 5.x+, mimalloc is used to address this. A similar approach could be added to 4.x.

However, the biggest impact for the receive buffers happen with AdaptiveRecvByteBufAllocator parameters which are hardcoded in Pulsar:

bootstrap.childOption(ChannelOption.RECVBUF_ALLOCATOR,
new AdaptiveRecvByteBufAllocator(1024, 16 * 1024, 1 * 1024 * 1024));

With sustained traffic, the receive buffer size will grow up to 1 MB. Adjusting that would be necessary to actually make an impact. Without changing that, all connections pushing a lot of data will eventually have receive buffers of 1 MB.

PIP-430 suggested making AdaptiveRecvByteBufAllocator parameters configurable, but that hasn't been implemented:

pulsar/pip/pip-430.md

Lines 196 to 224 in 73c75fe

### Note About Entry Cache Size and How It's Related to Broker Cache Total Size
The calculation of the entry cache size is not accurate unless the `managedLedgerCacheCopyEntries` setting is set to `true`. This occurs because when the entry payload is read from BookKeeper or received from a Pulsar client publisher, it is "sliced" from an underlying buffer. The actual memory consumption can be higher when the other entries of the shared underlying buffer have been released and there is a single entry that retains the complete underlying buffer.
For entries received from a Pulsar client publisher, the buffer size is between 16kB to 1MB. This is currently not configurable and is set in code in the [BrokerService.defaultServerBootstrap method](https://github.com/apache/pulsar/blob/19b4a05f888da35a584cf7ff6d594dd9df5e03d1/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L564-L565).
```java
bootstrap.childOption(ChannelOption.RCVBUF_ALLOCATOR,
new AdaptiveRecvByteBufAllocator(1024, 16 * 1024, 1 * 1024 * 1024));
```
The actual underlying buffer size will also depend on Netty's LengthFieldBasedFrameDecoder (extends ByteToMessageDecoder) logic, which will cumulate buffers by copying until the complete entry has been read. In that case, the underlying buffer size can be larger than 1MB. The cumulated buffer will be shared across multiple entries since entry buffers are sliced from the cumulated buffer.
For entries received from BookKeeper, it's similar. However, BookKeeper client defaults to Netty's [default channel config settings](https://github.com/netty/netty/blob/c3416d8ad2260f3c9bc98a9061d25bba92020f93/transport/src/main/java/io/netty/channel/DefaultChannelConfig.java#L74-L76) for RCVBUF_ALLOCATOR, which has a maximum buffer size of 64kB.
It would be useful to make the Pulsar broker side `RCVBUF_ALLOCATOR` parameters configurable so that the parameters could be tuned to a smaller size. It is very unlikely that the 1MB maximum size improves performance significantly. There is a performance tradeoff in the case where messages are large and buffers would have to be merged by copying in the LengthFieldBasedFrameDecoder. To avoid this overhead, it would be possible to make LengthFieldBasedFrameDecoder use composite buffers for merging by setting the `cumulator` property to `ByteToMessageDecoder.COMPOSITE_CUMULATOR`, however that could have a negative performance impact on code that expects that the complete payload is a single non-continuous buffer.
Since in many cases, the underlying buffer where the slice is taken for a single entry is shared with entries that have been published or retrieved together, it is also common that these entries will get evicted together. Making the RCVBUF_ALLOCATOR settings configurable in the Pulsar broker is a sufficient mitigation for the problem. In cases where shared buffers add a lot of overhead of consumed memory, it will be possible to reduce it by setting the maximum size for `AdaptiveRecvByteBufAllocator` to 64kB, with the tradeoff of possible unnecessary buffer copies for messages exceeding 64kB. In BookKeeper server, there are settings `byteBufAllocatorSizeInitial`, `byteBufAllocatorSizeMin` and `byteBufAllocatorSizeMax` to configure the `AdaptiveRecvByteBufAllocator` parameters. The naming of these parameters in BookKeeper isn't optimal since the settings are specifically about `AdaptiveRecvByteBufAllocator` parameters and not "byteBufAllocator" parameters.
The proposal would be to add these configuration parameters to `broker.conf` for controlling Broker's `AdaptiveRecvByteBufAllocator` parameters:
```properties
# Netty adaptive receive buffer allocator's minimum size
brokerAdaptiveRecvByteBufAllocatorMinimumSize=1024
# Netty adaptive receive buffer allocator's initial size
brokerAdaptiveRecvByteBufAllocatorInitialSize=16384
# Netty receive adaptive buffer allocator's maximum size
# Tune this value to a lower value to reduce overhead of the entries cached in the Broker cache due to shared underlying buffers
brokerAdaptiveRecvByteBufAllocatorMaximumSize=1048576
```

I've previously requested this:

Please file an issue to show exactly which problem this is addressing. In the issue, it would also be useful to share the reproducer of the issue.

Unless there's a reproducer, it's also hard to evaluate whether this PR is really solving the actual problem.

…uests

Add DISABLED, AUTO and ENABLED request-frame limiting modes, and apply the latest dynamic memory threshold to all connections. Separate socket backpressure from queued frame delivery so memory throttling permits buffered requests and incomplete frames to make progress.

Cover threshold updates, automatic-mode transitions, concurrent releases, pooled sibling frames, overlapping business limits and connection cleanup. Verified targeted broker tests, microbenchmark compilation and quickCheck.

Assisted-by: Codex
Add a broker-wide OpenTelemetry gauge for the active throttle state and cover dynamic configuration and half-limit recovery in metric export tests.

Assisted-by: Codex
@poorbarcode

Copy link
Copy Markdown
Contributor Author

@lhotari, following up on the PIP, reproducer, allocator and performance requests in this review and the follow-up:

  • PIP: PIP-499, #27155 has been submitted for design review.
  • Problem statement and reproduction: these are consolidated in the PIP's Reproducing the original failure section. The PIP reports reproduction on Pulsar 4.0.14 and 5.0.1; this section provides the detailed 5.0.1 image/client/framework versions, resource budgets, broker/BookKeeper settings, and workload conditions. The documented setup uses a resource-constrained Bookie and small write cache to slow real writes, without an artificial BookKeeper delay.
  • Allocator, malloc/THP and receive-buffer sizing: discussion is continuing in the allocator/native-memory thread and the receive-buffer thread. The PIP now distinguishes receive-buffer sizing, decoder cumulation, retained roots, allocator reserves and RSS, and discusses receive-size tuning as a complementary alternative. Please continue that design discussion there.
  • Performance: the PR description now compares the no-PR common ancestor with the complete current implementation on the same native Linux amd64 VM, using actual Pulsar Docker images with mimalloc and identical telemetry settings. The completed small-message comparison has three runs per variant: broker CPU per million messages increased by 2.35% / 3.40% / 2.90% for DISABLED / AUTO / ENABLED. The description includes raw evidence, ranges, enablement/throttle observations and the configured limit. These are fixed-load results; they do not measure maximum throughput or validate the original memory-pressure failure.
  • Decoder installation: continuous accounting in all three modes is part of the PIP's observability goal, including DISABLED. I explained the requirement and measured cost in the inline reply.
  • Previously reported code issues: the queued-frame stall is fixed within this PR while retaining per-message flow control; the updated reply supersedes the old dependency on [improve][broker] Remove the inbound FlowControlHandler queue #27126. The independent EntryCache dynamic-copying and publication-race work was split into [improve][broker] Make entry cache copying dynamically configurable #27148 and has been merged.

Comment on lines +25 to +26
/** Track retained memory without applying request-frame backpressure, even under memory pressure. */
DISABLED,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a mode to completely disable this feature. This doesn't seem to be the case. It's misleading to call this DISABLED.

@poorbarcode poorbarcode Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed DISABLED to TRACKING, and added new DISABLED

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants