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

Chunked encoding in tree summary #17602

Merged
merged 33 commits into from Nov 10, 2023

Conversation

daesunp
Copy link
Contributor

@daesunp daesunp commented Oct 3, 2023

Description

This PR changes the sharedTree summary to use the chunked forest codec.

@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: tree public api change Changes to a public API base: main PRs targeted against main branch labels Oct 3, 2023
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Oct 3, 2023

@fluid-example/bundle-size-tests: +11.45 KB
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 447.51 KB 447.51 KB No change
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 240.61 KB 240.61 KB No change
loader.js 178.94 KB 178.94 KB No change
map.js 48.06 KB 48.06 KB No change
matrix.js 141.84 KB 141.84 KB No change
odspDriver.js 90.27 KB 90.27 KB No change
odspPrefetchSnapshot.js 41.9 KB 41.9 KB No change
sharedString.js 162.75 KB 162.75 KB No change
sharedTree2.js 263.51 KB 274.96 KB +11.45 KB
Total Size 1.72 MB 1.73 MB +11.45 KB

Baseline commit: ed4ffda

Generated by 🚫 dangerJS against a88b6f8

const delta: [FieldKey, Delta.Insert[]][] = fields.map(([fieldKey, content]) => {
const jsonableTree = mapCursorField(
Copy link
Contributor

Choose a reason for hiding this comment

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

this conversion is unneeded. Since you just need a cursor, decode(content).cursor() can be used as the Delta's content below, and you don't need to generate a jsonable tree then make a cursor from that.

Copy link
Contributor Author

@daesunp daesunp Oct 10, 2023

Choose a reason for hiding this comment

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

I have tried to prevent generating a jsonable tree to convert back to a cursor by changing to the following (shown below), but for some reason this does not convert the field cursor into an array of node cursors and the resulting cursors are still in fields mode (which leads to an error).

const cursors = mapCursorField( decode(content).cursor(), (cursor) => cursor as ITreeCursorSynchronous )

However, if I convert to a jsonable tree first (shown below), the cursors seem to be properly converted and it works.

const cursors = mapCursorField( decode(content).cursor(), jsonableTreeFromCursor, ).map(singleTextCursor);

Would there be a better way to convert the field cursor into an array of node cursors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to const cursors = mapCursorField(decode(content).cursor(), (cursor) => cursor.fork());

@daesunp daesunp mentioned this pull request Oct 11, 2023
const factory = new SharedTreeFactory({ jsonValidator: typeboxValidator });
const factory = new SharedTreeFactory({
jsonValidator: typeboxValidator,
summaryEncodeType: TreeCompressionStratagy.Compressed,
Copy link
Contributor

Choose a reason for hiding this comment

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

As we store textual diffs for these, if they are going to explicitly an option, I think uncompressed would make more sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to TreeCompressionStrategy.Uncompressed

case TreeCompressionStrategy.Uncompressed:
return uncompressedEncode(cursor);
default:
return schemaCompressedEncode(schema, policy, cursor);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think using unreachableCase here would make this code less likley to get out of date if we added more options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated default case to unreachableCase

@daesunp daesunp marked this pull request as ready for review November 9, 2023 19:40
@daesunp daesunp requested review from a team as code owners November 9, 2023 19:40
@daesunp daesunp merged commit 96239d2 into microsoft:main Nov 10, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: tree area: dds Issues related to distributed data structures base: main PRs targeted against main branch public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants