Conversation
There was a problem hiding this comment.
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-
TreeShapecache forChunkShapeinstances keyed bytopLevelLengthfor 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. |
13ed8bf to
238bbe2
Compare
| @@ -176,6 +199,13 @@ export class TreeShape { | |||
| } | |||
|
|
|||
| public withTopLevelLength(topLevelLength: number): ChunkShape { | |||
There was a problem hiding this comment.
Nit: seems like we should probably assert that topLevelLength is positive (unless the ChunkShape constructor will already do that.
There was a problem hiding this comment.
Yeah, ChunkShape has what you described:
assert(topLevelLength > 0, 0x4c6 /* topLevelLength must be greater than 0 */);
Josmithr
left a comment
There was a problem hiding this comment.
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.
Summary
Adds a
Map<number, ChunkShape>cache toTreeShapeso that repeated calls towithTopLevelLength(n)with the same small value (n < 8) return the sameChunkShapeinstance instead of creating a new one each time.Values >= 8are not cached to prevent unbounded map growth.Full heap byte size difference between
objectForestvschunkedForesttested with plain text after changes. The heap snapshots are taken after building and materializing a fresh plain text tree:objectForestchunkedForest