Skip to content

[typescript] McapReader: add option to prefetch message indexes in parallel - #1641

Open
snosenzo wants to merge 5 commits into
mainfrom
sam/prefetch-message-indexes
Open

[typescript] McapReader: add option to prefetch message indexes in parallel#1641
snosenzo wants to merge 5 commits into
mainfrom
sam/prefetch-message-indexes

Conversation

@snosenzo

@snosenzo snosenzo commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Changelog

-[typescript] Add optional IReadablefield supportsConcurrentReads to advertise capability to McapReaders

  • [typescript] Add optional prefetchMessageIndexes argument to McapIndexedReader which will enable caching and prefetch message indexes if the underlying Readable supports concurrent reads.

Docs

None. Documented in JSDoc docs

Description

This allows message indexes to be loaded in parallel starting when the Reader is initialized which significantly speeds up message index reading. Currently the concurrency is locked to up to 6 concurrent requests, based around browser HTTP:1 limits. This could be specified in the future, but I didn't want to add more api surface area.

If prefetch is enabled and no cache size is specified it will automatically size the cache to the size of the message indexes in the mcap, with a safe maximum cache size of 256MB.

If one of the prefetches fails then it will bail on prefetching and return the error in the first readMessages call so that the error can be surfaced to the owner of the Reader. Subsequent reads will act normally.

numReads latencyMs noCache (ms) cached (ms) prefetch (ms) cached vs noCache prefetch vs noCache prefetch vs cached
1 0 429.7 455.1 438.4 -5.9% -2.0% 3.7%
1 10 4,825.1 4,707.0 2,988.0 2.4% 38.1% 36.5%
5 0 2,325.7 2,198.6 2,529.8 5.5% -8.8% -15.1%
5 10 23,871.2 15,359.5 13,487.8 35.7% 43.5% 12.2%

The losses are expected for the no/lowest-latency, single read cases, since this benchmark uses an in memory mcap buffer and the overhead of the requests wins out. Enabling this option is not advised in those cases. This option is meant to be used primarily for remote file data sources which have a latencies of >20ms. In that range the savings are very apparent.

claude[bot]

This comment was marked as resolved.

claude[bot]

This comment was marked as resolved.

claude[bot]

This comment was marked as resolved.

claude[bot]

This comment was marked as resolved.

claude[bot]

This comment was marked as resolved.

Base automatically changed from sam/cache-message-indexes to main April 24, 2026 13:05
claude[bot]

This comment was marked as resolved.

@claude claude Bot 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.

All ten prior threads are resolved — the latest iteration addresses every concern from previous rounds. Traced the prefetch → CachedReadable byte cache → ChunkCursor.loadMessageIndexes() lifecycle: offsets and sizes align across #startMessageIndexPrefetch, #isMessageIndexCached, and the readFullMessageIndexRange path in ChunkCursor, so cache hits land correctly. The concurrent-read dedup via the pending map, error-once-then-fallback semantics, cumulative-bytes budget cap, and per-chunk cache check before awaiting all trace correctly.

Test coverage is thorough: prefetch-before-read, gated in-flight await, error surfacing with fallback, no-concurrent-reads throw, cache-budget-respected, and the new CachedReadable concurrent-read / double-count / replaced-entry tests all exercise the important paths.

No new blockers.

@jtbandes

Copy link
Copy Markdown
Member

@snosenzo do we still want this?

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants