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

MergeTree: Make SortedSegmentSet Iterative and Add Some Tests #11605

Merged
merged 3 commits into from Aug 22, 2022

Conversation

anthony-murphy
Copy link
Contributor

@anthony-murphy anthony-murphy commented Aug 19, 2022

Description

This is related to #11603 and #1009. Like the merge tree walk functions the recursiveness here was also causing perf issue when trying during undo, which frequently adds and removes items from a SortedSegmentSet via a tracking group. There were no directly test for SortedSegmentSet, as it was mainly tested via tracking groups, so adding a couple tests. Additionally, using a SortedSegmentSet with an object that provided a segments {segment: ISegment} was broken previously. This change also fixes that issue by allowing multiple elements with a match ordinal.

AB#151

@anthony-murphy anthony-murphy requested review from a team as code owners August 19, 2022 19:57
@github-actions github-actions bot added area: contributor experience area: dds Issues related to distributed data structures public api change Changes to a public API base: next PRs targeted against next branch labels Aug 19, 2022
@@ -42,5 +42,5 @@
"unaugmented"
],
"editor.detectIndentation": true,
"editor.insertSpaces": false, // Use detectIndentation instead
"editor.insertSpaces": true, // Use detectIndentation instead
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was having problems with my editor defaulting to tabs, this seemed to help. editor.detectIndentation will still override it, but it must use insertSpaces when it is not sure, like in a new file

@msfluid-bot
Copy link
Collaborator

@fluid-example/bundle-size-tests: +16.53 KB
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 392.45 KB 394.42 KB +1.98 KB
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 191.92 KB 197.48 KB +5.55 KB
loader.js 151.12 KB 151.06 KB -57 Bytes
map.js 42.63 KB 47.38 KB +4.75 KB
matrix.js 131.63 KB 134.64 KB +3.01 KB
odspDriver.js 150.23 KB 150.11 KB -127 Bytes
odspPrefetchSnapshot.js 38.39 KB 38.35 KB -41 Bytes
sharedString.js 152.42 KB 153.88 KB +1.46 KB
Total Size 1.25 MB 1.27 MB +16.53 KB

Baseline commit: fe10534

Generated by 🚫 dangerJS against 7c80d42

Copy link
Contributor

@Abe27342 Abe27342 left a comment

Choose a reason for hiding this comment

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

looks good. I wonder if we could get some better code re-use between SortedSegmentSet and partial lengths. The fact that segments' ordinals can change but they must remain stable is still enough for them to satisfy reasonable comparator invariants, and from there it's a pretty generic data structure.

@anthony-murphy
Copy link
Contributor Author

looks good. I wonder if we could get some better code re-use between SortedSegmentSet and partial lengths. The fact that segments' ordinals can change but they must remain stable is still enough for them to satisfy reasonable comparator invariants, and from there it's a pretty generic data structure.

interesting! i hadn't thought about that. you should open an issue and describe the overlap between the two, as if we can share code i'd love to get it scheduled

@anthony-murphy anthony-murphy merged commit 3ffd62a into microsoft:next Aug 22, 2022
@anthony-murphy anthony-murphy deleted the finditem-post-iterative branch August 22, 2022 22:33
@Abe27342
Copy link
Contributor

Abe27342 commented Aug 23, 2022

looks good. I wonder if we could get some better code re-use between SortedSegmentSet and partial lengths. The fact that segments' ordinals can change but they must remain stable is still enough for them to satisfy reasonable comparator invariants, and from there it's a pretty generic data structure.

interesting! i hadn't thought about that. you should open an issue and describe the overlap between the two, as if we can share code i'd love to get it scheduled

sounds good. I tracked this internally in AB#1663 and put it in next sprint. Could be a nice ramp-up task and works toward our code size quality goals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: contributor experience area: dds Issues related to distributed data structures base: next PRs targeted against next 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