Skip to content

Improve mutation processing performance #1652

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.

Sign up for GitHub

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

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Improve mutation processing performance #1652

Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
43c5d72
Iterate over the added nodes in 'one pass' so that we don't need to b…
eoghanmurray Feb 10, 2025
ed757b3
Test changes, rearrangement of mutations
eoghanmurray Feb 10, 2025
1d8b37b
Add some ids as I'm interested in tracing these nodes through pushAdd…
eoghanmurray Feb 10, 2025
99da1e4
Do away with the second pass as we can handle shadow DOM in the first…
eoghanmurray Feb 10, 2025
490aea9
Performance oriented refactor focusing on scenario where a large numb…
eoghanmurray Feb 11, 2025
d9587d4
Satisfy typescript which could be smarter here ... we can guarantee t…
eoghanmurray Feb 11, 2025
2a0eedd
Utilize `lastChild` to avoid possibly crawling through hundreds of nodes
eoghanmurray Feb 11, 2025
96ea20d
We've already got `nextSibling` here so can skip a step and avoid the…
eoghanmurray Feb 11, 2025
2aa8597
Test rearrangements in the adds array due to new algorithm; should be…
eoghanmurray Feb 11, 2025
8d4e766
We were calling `inDom` in all cases, so don't do the other ancestor …
eoghanmurray Feb 11, 2025
3b33611
Don't think we're explicitly looking at the slimdom stuff in relation…
eoghanmurray Feb 11, 2025
f260c0d
Add changeset
eoghanmurray Feb 11, 2025
87b091a
Don't think `main` subfolder was ever used as an output target; this …
eoghanmurray Feb 11, 2025
1441ef3
Placate eslint (`while(true)` is a Pythonism rather than do..while) -…
eoghanmurray Feb 11, 2025
b5b04e2
Forgot to add the mutation.html file - also add doctype
eoghanmurray Feb 11, 2025
720e174
Simplify the parentId check, doesn't need to ever by null
eoghanmurray Feb 11, 2025
c166319
Move mutation tests into their own files to demonstrate an idea which…
eoghanmurray Feb 12, 2025
4a751ee
Apply formatting changes
eoghanmurray Feb 12, 2025
7734331
Some inconsequential tests to cover blocking scenarios
eoghanmurray Feb 12, 2025
4c2af79
Was trying to 'catch out' the mutation handling by having siblings pr…
eoghanmurray Feb 12, 2025
8b27440
fixup! Move mutation tests into their own files to demonstrate an ide…
eoghanmurray Feb 12, 2025
453beb0
I can't recreate a scenario for this case in testing, so add a warnin…
eoghanmurray Feb 12, 2025
03f2146
Put each snap file in it's own folder and shorten names
eoghanmurray Feb 12, 2025
9666d32
Satisfy eslint
eoghanmurray Feb 13, 2025
e90b18b
Repeat the mutation tests but with the blocking/ignored nodes already…
eoghanmurray Mar 7, 2025
9a7a47e
Indicate that replay no longer needs the queue, as added nodes should…
eoghanmurray Mar 7, 2025
f9f660c
Merge branch 'master' into pushAddOrder
Juice10 Sep 5, 2025
7d39bbc
Update packages/rrweb/src/replay/index.ts
eoghanmurray Sep 5, 2025
0b0d662
Update packages/rrweb/src/record/mutation.ts
eoghanmurray Sep 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
We were calling inDom in all cases, so don't do the other ancestor … 
…checks if it goes bad early
  • Loading branch information
@eoghanmurray
eoghanmurray committed Mar 7, 2025
commit 8d4e766d9cd82712ee6b94887c614015f9797adb
18 changes: 9 additions & 9 deletions packages/rrweb/src/record/mutation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,17 +218,17 @@ export default class MutationBuffer {
// we have a new parentNode for a 'row' of DOM children
// perf: we reuse these calculations across all child nodes

ancestorBad =
isSelfOrAncestorInSet(this.droppedSet, parentNode) ||
this.removesSubTreeCache.has(parentNode);

if (ancestorBad && isSelfOrAncestorInSet(this.movedSet, n)) {
// not bad, just moved
ancestorBad = false;
}
if (!inDom(parentNode)) {
// this check should overrule moved also
ancestorBad = true;
} else {
ancestorBad =
isSelfOrAncestorInSet(this.droppedSet, parentNode) ||
this.removesSubTreeCache.has(parentNode);

if (ancestorBad && isSelfOrAncestorInSet(this.movedSet, n)) {
// not bad, just moved
ancestorBad = false;
}
}

if (this.addedSet.has(parentNode.lastChild as Node)) {
Copy link
Contributor

@pauldambra pauldambra Jul 8, 2025

Choose a reason for hiding this comment

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

i guess the casts to Node are to avoid !!parentNode.lastChild && x.has(parentNode.lastChild)

i think safe here but as X is such a source of bugs that it always scares me 🙈

Copy link
Contributor Author

@eoghanmurray eoghanmurray Sep 5, 2025

Choose a reason for hiding this comment

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

Ya, typescript sees new Set<Node>(), so wants to only allow Nodes in the has function, whereas .lastChild can return null. However the .has(null) on a Javascript Set will return false which is fine. We don't want nulls in the set, so preventing .add(null) makes sense, however the error on the .has is a bit too strict for my taste.

Expand Down