Update McapReader to allow for prefetching and caching of chunk message indexes - #1636
Update McapReader to allow for prefetching and caching of chunk message indexes#1636snosenzo wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
All four prior threads addressed — resolved and minimized.
Re-traced the prefetch→cache→cursor lookup chain across both the concurrent and sequential paths. The byte ranges, cache keys (chunkStartOffset), and subarray slicing are symmetrical between Initialize and ChunkCursor.loadMessageIndexes. The defensive copy in the sequential path correctly guards against buffer-reusing readables like FileHandleReadable.
Two things worth addressing before this leaves draft:
- Test gap: No test exercises the stated primary benefit — calling
readMessages()multiple times on the same reader to verify the cache eliminates repeated message-index reads. See inline suggestion. - Docs section: Still empty in the PR description. This adds public API surface to both
IReadable(supportsConcurrentReads) andMcapIndexedReader.Initialize(prefetchMessageIndexes). Should either link a docs update or explicitly note "None" with rationale (e.g., "API is TypeScript-only with JSDoc, no external docs to update").
| expect(readable.readCalls - readsAfterInit).toEqual(2); | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
suggestion: The PR description and docs both emphasize that the primary value of prefetch is reuse across multiple readMessages calls ("seeking, topic filtering, playback UIs"). But no test calls readMessages() twice on the same reader to verify the cache actually persists and eliminates message-index reads on subsequent iterations.
Something like:
it("reuses cached indexes on a second readMessages() call", async () => {
const { buffer, messages } = buildTwoChunkFile();
const readable = makeTrackingReadable(buffer, { supportsConcurrentReads: false });
const reader = await McapIndexedReader.Initialize({
readable,
prefetchMessageIndexes: true,
});
await expect(collect(reader.readMessages())).resolves.toEqual(messages);
const readsAfterFirst = readable.readCalls;
// Second read should still only issue chunk-data reads, no message-index reads
await expect(collect(reader.readMessages())).resolves.toEqual(messages);
expect(readable.readCalls - readsAfterFirst).toEqual(2);
});| indexRequests.push({ | ||
| chunkStartOffset: chunkIndex.chunkStartOffset, | ||
| offset: messageIndexStartOffset, | ||
| length: chunkIndex.messageIndexLength, |
There was a problem hiding this comment.
question: messageIndexLength is documented in the MCAP spec as the total byte length of all MessageIndex records following the chunk. The read here assumes that min(messageIndexOffsets.values()) is the start of that contiguous range — i.e., that no gap exists between the chunk record and the first message index, and that all message indexes for a chunk are contiguous.
That assumption holds for spec-conformant files, but is it enforced anywhere during parsing? If a malformed file has non-contiguous message index offsets, this read would silently fetch the wrong bytes. The non-cached path in ChunkCursor would also be wrong in that case, so this isn't a regression — but since you're now reading all indexes up front, the blast radius of a malformed offset is larger (corrupted cache vs. one bad chunk read).
Worth a brief comment here noting the contiguity assumption?
There was a problem hiding this comment.
Do we account for non-spec-conformant files elsewhere? Is this really an issue?
Changelog
prefetchMessageIndexesflag toMcapIndexedReaderclasssupportsConcurrentReadsflag toIReadableinterfaceDocs
Description
Both of the new optional fields are opt-in and will cause changes to existing data flows.
Enabling
prefetchMessageIndexeswill request all of the message indexes for the chunks during initialization of the data source. These indexes will be cached and will eliminate message index requests that would normally happen before each reading of a chunk. This speeds up message reading from chunks for an up front cost.These indexes are relatively small compared to the size of the chunks themselves (16-bytes per message), and shouldn't be a problem to load and store in memory.
Examples: 1 hr of 250Hz messages -> 900,000 messages -> 14.4MB
10,000,000 messages -> 160MB
100,000,000 messages -> 1.6GB (in this case I would advise against caching and prefetching message indexes)
supportsConcurrentReadsis an optional field on theReadableinterface which allows theMcapIndexedReaderto parallelize the requests for the message indexes if it is enabled. This could also be used for other parallelization optimizations in the McapReader in the future.Benchmark results
latency=50ms chunks~10 msgSize=16Blatency=5ms chunks~100 msgSize=16Blatency=50ms chunks~100 msgSize=16Blatency=5ms chunks~1000 msgSize=16Blatency=50ms chunks~100 msgSize=1024Blatency=0ms chunks~1000 msgSize=1024B(204 MiB file)Observations that match the feature's design:
Concurrent prefetch gives ~6x reduction on the prefetch phase itself because of the
MAX_CONCURRENT_READS = 6pool inMcapIndexedReader.Initialize. Total speedups land around 1.5x–1.7x on latency-bound runs, however on subsequent reads these gains will grow because the message indices will already be read.For low-latency cases it's clear that this upfront cost isn't saving much except on subsequent reads where the index information will not have to be fetched when chunks are refreshed for new subscriptions.