Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed GC blobs from channel context (DDS). The GC blobs in data store is used to generate initial GC data for DDS #5007

Merged
merged 3 commits into from
Feb 5, 2021

Conversation

agarwal-navin
Copy link
Contributor

@agarwal-navin agarwal-navin commented Feb 1, 2021

Fixes #4943.

Couple of notes:

  • This change introduces a small inefficiency when a new DDS is created after the data store is attached. When the summarizer tries to get its GC data for the first time, it will generate the GC data even if there was no change in the DDS since it got the attach message. This is because the initial summary of the data store will not have the data for this DDS.
    • There is a simple fix for this - Add the GC blob to the DDS's summary only when creating attach message. However, this will introduce extra GC blobs in the initial summary which won't get removed until this DDS is summarized again. The inefficiency seems worth this trade-off. We can always add this if we realize that summarizing the DDS is becoming too expensive.
  • Documents before this change won't have the getInitialGCSummaryDetails in IFluidChannelContext. So, the summarizer would end up summarizing these DDSs the first time. I can handle this scenario by adding some back-compat code but I don't think its worth maintaining that code for DDSs. Please let me know if you think we need to handle this scenario.

@vladsud
Copy link
Contributor

vladsud commented Feb 1, 2021

Looks reasonable to me.

@github-actions github-actions bot requested a review from vladsud February 3, 2021 00:29
@agarwal-navin agarwal-navin changed the title Prototype for removing GC blobs from DDS and re-using data store's GC blobs Removed GC blobs from channel context (DDS). The GC blobs in data store is used to generate initial GC data for DDS Feb 3, 2021
@agarwal-navin agarwal-navin marked this pull request as ready for review February 3, 2021 00:42
* @param channelId - The id of the channel context that is asked for the initial GC details.
* @returns the requested channel's GC details in the initial summary.
*/
private async getChannelInitialGCDetails(channelId: string): Promise<IGarbageCollectionSummaryDetails> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume we have reasonable test coverage for new logic through existing UTs, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, we do not have unit tests for data store runtime! and for channel contexts too. We should definitely add those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #5057 to add testing for these layers.

@msfluid-bot
Copy link
Collaborator

@fluidframework/base-host: No change
Metric NameBaseline SizeCompare SizeSize Diff
main.js 169.41 KB 169.41 KB No change
Total Size 169.41 KB 169.41 KB No change
@fluid-example/bundle-size-tests: +1001 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
container.js 193.96 KB 194.94 KB +1001 Bytes
map.js 45.92 KB 45.92 KB No change
matrix.js 144.59 KB 144.59 KB No change
odspDriver.js 195.89 KB 195.89 KB No change
sharedString.js 158.65 KB 158.65 KB No change
Total Size 739.01 KB 739.99 KB +1001 Bytes

Baseline commit: 96aaec4

Generated by 🚫 dangerJS against aec5b9f

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.

Reduce # of GC blobs in a document by removing GC blobs from DDS's snapshot
3 participants