improve(handles): Skip no-op bind step when serializing handles during regular summarization #25353
Conversation
github-actions
bot
added
area: dds
github-actions
bot
removed
the
area: tests
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes handle serialization during summarization by skipping the bind operation when it's guaranteed to be a no-op. The change leverages the fact that during summarization, all DDSes are attached and handles are already bound.
- Introduces a
forSummarizerflag to FluidSerializer to control bind behavior - Adds assertions to verify the invariants that make bind unnecessary during summarization
- Creates a dedicated serializer instance for summarization operations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/dds/shared-object-base/src/sharedObject.ts | Creates a new FluidSerializer instance with forSummarizer flag for summarization |
| packages/dds/shared-object-base/src/serializer.ts | Adds forSummarizer parameter and conditional bind logic with assertions |
| assert(bind.isAttached, "Expected bind source to be attached in the Summarizer"); | ||
| assert( | ||
| handle.isAttached, | ||
| "Expected target to have been already attached (via binding to source)", | ||
| ); |
There was a problem hiding this comment.
The assert statements should use string literals for error messages instead of descriptive text. Consider using hex codes or simple string literals as specified in the coding guidelines.
Copilot generated this review using guidance from repository custom instructions.
markfields
changed the title
| @@ -225,7 +228,16 @@ export class FluidSerializer implements IFluidSerializer { | |||
| handle: IFluidHandleInternal, | |||
| bind: ISharedObjectHandle, | |||
| ): ISerializedHandle { | |||
| bind.bind(handle); | |||
| if (this.forSummarizer) { | |||
There was a problem hiding this comment.
Consider creating a separate serializer for summarizer like the GCHandleVisitor. IMO, that is much cleaner and is similar to existing patterns.
| @@ -225,7 +228,16 @@ export class FluidSerializer implements IFluidSerializer { | |||
| handle: IFluidHandleInternal, | |||
| bind: ISharedObjectHandle, | |||
| ): ISerializedHandle { | |||
| bind.bind(handle); | |||
| if (this.forSummarizer) { | |||
| assert(bind.isAttached, "Expected bind source to be attached in the Summarizer"); | |||
There was a problem hiding this comment.
I think this should be safe. But should we consider adding this behind a feature flag or something to be sure?
Description
What's changed
No longer call bind in the FluidSerializer during summarization. Instead, assert the two invariants discussed below which prove that bind would be a no-op.
Background
During Summarization, we can be sure of these two facts:
(1) is true because of asserts and errors in ChannelCollection and DataStoreRuntime
(2) is true because the Summarizer only runs on attached containers, and the source DDS is attached (see 1), so its outbound handles would have already been bound.
And besides all this -- Summarizer shouldn't even have local changes! But we know there are cases where it does so it's good to be sure.
Reviewer Guidance
Mostly depending on existing test coverage to prove it's ok. Did not come up with specific new tests to add, but open to suggestions.