Skip to content

[improve][ml] Use adaptive allocation for managed ledger cache copies - #276

Closed
lhotari wants to merge 2 commits into
masterfrom
lhotari-improve-ml-cache-adaptive
Closed

lhotari wants to merge 2 commits into
masterfrom
lhotari-improve-ml-cache-adaptive

Conversation

@lhotari

@lhotari lhotari commented Sep 16, 2026

Copy link
Copy Markdown
Owner

Motivation

Follow up on apache#25280 and apache#26597. Batch reads are now enabled by default, and their entries must be copied into cache-owned buffers even when managedLedgerCacheCopyEntries=false: retaining one entry must not retain the entire BookKeeper response frame. This makes the ml-cache allocator relevant to the default storage-read path.

Use Netty's adaptive allocator as the built-in default for these copies. The expected workload is many small entries with similar sizes. The rationale is reduced fragmentation waste under that assumption:

  • Netty 4.2.18's small-allocation path separates sizes into magazine groups and reuses individually freed slots within a chunk. Entries that remain cached therefore do not prevent reuse of the slots released by other entries. Its size classes include power-of-two payload sizes plus room for headers, limiting rounding waste for that common pattern. Small-entry chunks start at 128 KiB rather than the pooled allocator's usual 4 MiB chunk size. See the pinned implementation.
  • The size-class design discussion specifically addresses small, plentiful allocations and long-lived small buffers preventing reuse of large chunks. It introduced reusable free-list slots to reduce that fragmentation. This is a closer match to cache insertion and eviction than the original histogram-only adaptive design.
  • Netty's allocator documentation describes adaptive's smaller chunks and makes it the Netty 4.2 default. The chunk-sizing discussion also explains how page-aligned chunk sizing reduces fragmentation pressure on the system allocator.

This is a workload-based default choice, not a measured performance claim. Size-class rounding, partially occupied chunks, and allocator caches still consume memory; adaptive does not guarantee lower RSS for every size distribution or eviction pattern. No benchmarks were run.

managedLedgerCacheCopyEntries=false retains the incoming buffer; it does not imply unpooled allocation. The previous cache-copy allocator was pooled. This change preserves APIs, data formats, and explicit allocator settings, while deliberately changing allocation and memory-retention behavior when no allocator type is configured. Operators can retain the previous policy with -Dpulsar.allocator.ml-cache.type=pooled.

Modifications

  • Default only the ml-cache allocator to adaptive. General and other named allocators remain pooled. Preserve named overrides, global type fallback, and legacy pulsar.allocator.pooled settings.
  • Document the default, its scope, and the explicit pooled override in pulsar_env.sh.
  • Fix the remaining batch-read mock size-limit bug: a zero-length first entry must count as an accepted entry, so [0, 2048] with a 1024-byte limit is split into separate rounds.
  • Add allocator default/override tests, end-to-end batch splitting coverage with caching enabled/disabled and open/reopened ledgers, and the missing BookKeeper batch-disabled configuration case.
  • Strengthen the batch-cache regression to use slices of one shared response, evict all but one cached entry, and verify that the response is released while the cached entry remains readable.

The merged batch-read change already corrected mandatory cache copying, unsupported-bookie fallback documentation, and ensemble/quorum configuration names. Its protocol guard now checks the actual BookKeeper client configuration; this follow-up retains that design.

Verifying this change

  • ./gradlew quickCheck (license headers and main/test checkstyle).
  • Targeted PulsarByteBufAllocatorTest, PulsarByteBufAllocatorTypeTest, ManagedLedgerBatchReadTest, RangeEntryCacheImplTest, and ManagedLedgerTest.testBatchReadRequiresClientSupport, with retries disabled.
  • Verified the new regressions fail for the intended reasons when mandatory batch-cache copying and the zero-length mock fix are temporarily removed; restored both afterward.

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

The built-in managed-ledger cache allocator changes from pooled to adaptive. Explicit allocator settings retain their precedence.

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Preserve explicit allocator overrides and cover shared batch buffer release. Fix mock batch size splitting after an empty entry and cover the BookKeeper batch-read kill switch.

Assisted-by: Codex
Comment on lines +143 to +145
if (pooled != null) {
return "true".equalsIgnoreCase(pooled) ? AllocatorType.POOLED : AllocatorType.UNPOOLED;
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

this should be only the default for the "default" allocator

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Implemented. The built-in pooled fallback is now explicit for the default allocator; when no global or legacy type is configured, ml-cache continues to select adaptive. Explicit named, global, and legacy overrides retain their precedence. Targeted allocator tests and quickCheck pass.

Keep the pooled fallback explicit for the default allocator while using adaptive for ml-cache when no allocator type is configured.

Assisted-by: Codex
@lhotari lhotari closed this Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant