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

perf(genadds) traverse children in reverse order #1398

Closed

Conversation

mdellanoce
Copy link
Contributor

I noticed when you add a deep/complex tree as part of a mutation, a large portion of the tree ends up being added to the addList. This is because genAdds traverses the children of each node in order, but serialization in pushAdd wants each node to have a serialized parent and nextSibling. Reversing the order of traversal in genAdds makes it more likely to avoid adding nodes to the addList.

I ran the benchmarks before/after and this was a pretty small improvement across the board.

Copy link

changeset-bot bot commented Jan 16, 2024

🦋 Changeset detected

Latest commit: ecabd34

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
rrweb Patch
rrweb-snapshot Patch
rrdom Patch
rrdom-nodejs Patch
rrweb-player Patch
@rrweb/types Patch
@rrweb/web-extension Patch
rrvideo Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mdellanoce mdellanoce force-pushed the md-genadds-reverse-child-order branch from bebcb75 to ecabd34 Compare January 17, 2024 18:52
@mdellanoce
Copy link
Contributor Author

mdellanoce commented Jan 18, 2024

this might have an impact on performance when things do make it to the addList though, since the node order is reversed, and addList is traversed from the tail node to the head. I haven't found a case where that happens yet...

Update: i think this^ shouldn't be a problem b/c the addList already accounts for next/previous sibling relationships

@mdellanoce mdellanoce marked this pull request as draft January 18, 2024 16:03
@eoghanmurray
Copy link
Contributor

eoghanmurray commented Apr 13, 2024

You ran the ./packages/rrweb/test/benchmark/dom-mutation.test.ts benchmark?
Is there any addition you could make to those benchmarks that would demonstrate worse case for what you experienced?

The only possible objection I can think of is whether this could makes things worse for different types of data sets, but really your write-up makes a great case that the existing approach is worst-case.

@colingm
Copy link
Contributor

colingm commented May 1, 2024

For now thinking we will abandon this PR, it seems to most likely be addressed anyways by #1277 and so we can always revisit it after that one goes through

@mdellanoce mdellanoce closed this May 1, 2024
@mdellanoce mdellanoce deleted the md-genadds-reverse-child-order branch May 1, 2024 19:28
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.

3 participants