Skip to content

Add cache for chunkShapes#26988

Open
brrichards wants to merge 8 commits intomicrosoft:mainfrom
brrichards:topLevelLength-cache
Open

Add cache for chunkShapes#26988
brrichards wants to merge 8 commits intomicrosoft:mainfrom
brrichards:topLevelLength-cache

Conversation

@brrichards
Copy link
Copy Markdown
Contributor

@brrichards brrichards commented Apr 9, 2026

Summary

Adds a Map<number, ChunkShape> cache to TreeShape so that repeated calls to withTopLevelLength(n) with the same small value (n < 8) return the same ChunkShape instance instead of creating a new one each time. Values >= 8 are not cached to prevent unbounded map growth.

Full heap byte size difference between objectForest vs chunkedForest tested with plain text after changes. The heap snapshots are taken after building and materializing a fresh plain text tree:

Text characters objectForest chunkedForest diff
10 355,336 359,560 1.19%
100 401,224 375,664 -6.37%
1,000 943,520 706,652 -25.10%
10,000 5,334,648 2,807,992 -47.36%

@brrichards brrichards requested a review from a team as a code owner April 9, 2026 23:31
Copilot AI review requested due to automatic review settings April 9, 2026 23:31
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a small-instance cache on TreeShape so repeated calls to withTopLevelLength(n) for small n reuse the same ChunkShape instance, reducing repeated allocations in chunked-forest shape construction.

Changes:

  • Added a per-TreeShape cache for ChunkShape instances keyed by topLevelLength for values < 8.
  • Added a unit test asserting caching behavior for small lengths and non-caching for lengths >= 8.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
packages/dds/tree/src/feature-libraries/chunked-forest/uniformChunk.ts Adds chunkShapeCache and reuses cached ChunkShape instances for small topLevelLength values.
packages/dds/tree/src/test/feature-libraries/chunked-forest/uniformChunk.spec.ts Adds coverage to validate caching vs non-caching behavior for withTopLevelLength.

Comment thread packages/dds/tree/src/feature-libraries/chunked-forest/uniformChunk.ts Outdated
Comment thread packages/dds/tree/src/feature-libraries/chunked-forest/uniformChunk.ts Outdated
Comment thread packages/dds/tree/src/feature-libraries/chunked-forest/uniformChunk.ts Outdated
Comment thread packages/dds/tree/src/feature-libraries/chunked-forest/uniformChunk.ts Outdated
Comment thread packages/dds/tree/src/test/feature-libraries/chunked-forest/uniformChunk.spec.ts Outdated
@brrichards brrichards force-pushed the topLevelLength-cache branch from 13ed8bf to 238bbe2 Compare April 10, 2026 15:26
Comment thread packages/dds/tree/src/feature-libraries/chunked-forest/uniformChunk.ts Outdated
@Josmithr Josmithr requested a review from a team April 13, 2026 17:46
@@ -176,6 +199,13 @@ export class TreeShape {
}

public withTopLevelLength(topLevelLength: number): ChunkShape {
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.

Nit: seems like we should probably assert that topLevelLength is positive (unless the ChunkShape constructor will already do that.

Copy link
Copy Markdown
Contributor Author

@brrichards brrichards Apr 15, 2026

Choose a reason for hiding this comment

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

Yeah, ChunkShape has what you described:
assert(topLevelLength > 0, 0x4c6 /* topLevelLength must be greater than 0 */);

Copy link
Copy Markdown
Contributor

@Josmithr Josmithr left a comment

Choose a reason for hiding this comment

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

Looks good to me! It sounded like you had done some perf analysis on this? Might be nice to include a summary of those results in the PR description for historical context.

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.

4 participants