IdCompressor in the runtime #14340
IdCompressor in the runtime #14340
Conversation
This reverts commit 1e66288.
| @@ -1442,6 +1485,8 @@ export class ContainerRuntime | |||
| disableChunking, | |||
| disableAttachReorder: this.disableAttachReorder, | |||
| disablePartialFlush, | |||
| idCompressorEnabled: compressorEnabled ? true : undefined, | |||
There was a problem hiding this comment.
Isn't idCompressorEnabled: compressorEnabled enough? If it is explicitly set to false, you want false to be logged.
| ); | ||
| } | ||
| assert( | ||
| this.idCompressor !== undefined, |
There was a problem hiding this comment.
nit: Use this.idCompressorEnabled.
| assert( | ||
| this.idCompressor !== undefined, |
There was a problem hiding this comment.
nit: Use this.idCompressorEnabled.
| @@ -139,6 +139,40 @@ describeNoCompat("Runtime IdCompressor", (getTestObjectProvider) => { | |||
| assert(getIdCompressor(map) === undefined); | |||
| }); | |||
|
|
|||
| it("can't enable compressor on an existing container", async () => { | |||
There was a problem hiding this comment.
I missed this in my last comment - You should another test that validates it cannot be disabled once enabled.
There was a problem hiding this comment.
"Ignores runtime option if container is created and IdCompressor has been previously enabled" is the test that covers this scenario
There was a problem hiding this comment.
Nice! Maybe rename this test title for consistency with the other test.
@justus-camp @agarwal-navin Has this bundle size increase been discussed? Seems likely to be a point of contention during integration. Maybe consider whether the runtime performance gains of using the btree library are worth it vs. just a vanilla |
|
Hmmm. I did not look at this, no. Maybe worth evaluating with @justus-camp and SharedTree folks. |
@ChumpChief @agarwal-navin FYI I'm setting up some discussions to talk about need/mitigation here. |
@dannimad @taylorsw04 @ChumpChief @agarwal-navin this should be a blocker for the current minor release |
## Description Moving the IdCompressor into the runtime (#14340 ) has resulted in a large bundle size increase that we don't want consumers to take on while the compressor isn't being used anywhere. By changing to dynamic imports, only consumers who enable the compressor will have to pay download cost.
## Description Follow-up from #14340 where I introduced `createSummarizerWithTestConfig`, which should be the default usage pattern. This removes the original `createSummarizer` and replaces it, refactoring tests as necessary to use the new pattern.
Description
This PR exists as a point of discussion for implementation details around integrating the IdCompressor into the runtime. This has been forked from #13398.
Details
What to Review
The IdCompressor has already been used in production in legacy SharedTree and is thoroughly tested. Anything in
packages/runtime/container-runtime/src/id-compressorcan be ignored for the purpose of this review as it's just being moved to the runtime folder.Overview
The idea is to have a container runtime level IdCompressor that is shared across DDSs. DDS authors will be able to generate compressed Ids using the runtime-level compressor. Upon DDS op submission, the runtime checks the compressor's state to see if any compressed ops were generated that need to be sent. If there are
IdAllocationops, the runtime puts anIdAllocationop in front of the DDS's op.Stashed Ops
The idea is to "resume" the IdCompressor that the stashed client was using by including the current compressor state in its contents. This is done using
localOpMetadatauntil the container is closed usingcloseAndGetPendingLocalState. At this point, thelocalOpMetadatais copied intocontents.stashedStateso that the stashed state is stored for rehydrating the compressor. This state is deleted before sending the op over the wire for other compressors to synchronize.Enablement and Compat
This feature is turned off by default and must be explicitly enabled. The feature gate is
enableRuntimeIdCompressor. The idea here is to do the same thing we did with compression where we make breaking changes without them turning on until reaching proper saturation.Exposure to DDS
The
IdCompressorexists inside the container runtime but needs to be accessed by DDSs. I've added accessors to the IdCompressor from the container runtime down to the DataStoreRuntime level.I've introduced two new interfaces to
runtime-definitions:IdCompressorCoreandIdCompressor.IdCompressorCorecontains methods that shouldn't be exposed to a DDS author, but should be exposed at the container runtime level to manage theIdCompressor's state. Upon retrieving theIdCompressor, the DDS has access to all methods present in theIdCompressorinterface.Finalizing Id Ranges
IdRanges are included insideDataStoreOp's contents at theSharedObjectCorelevel inside ofsubmitLocalMessage. It must be included in the op's contents so that ops that are resubmitted throughreplayPendingStatescorrectly resubmit theIdRange. If appended at a higher level, theIdRangewould be lost on replay.Summarization
The
IdCompressor's state is added to the summary insideContainerRuntime::addContainerStateToSummary. I've added it as a blob (should it be a tree?). The blob is fetched as part of theContainerRuntime's constructor.