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

[masking] Optimize full snapshot serialization performance #1338

Closed

Conversation

ababik
Copy link
Contributor

@ababik ababik commented Oct 24, 2023

Implementation for #1337

Within the snapshot serialization, I refined the method by which rrweb determines if text should be masked. Presently, for every individual text node during recursion, the closest method is invoked to ascertain if the node is a descendant of an element marked for masking. I modified this by passing the masking flag directly as a parameter to the recursion function, thereby eliminating superfluous lookups.

@changeset-bot
Copy link

changeset-bot bot commented Oct 24, 2023

⚠️ No Changeset found

Latest commit: 444618f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@eoghanmurray
Copy link
Contributor

I'm concerned that this introduces a bit too much (incremental) complexity by adding a separate method of doing the check.

Would you consider the following strategy?

  • pass a new boolean isMasked down the recursive calls, starting off false
  • if this is still false, in serializeElementNode do a single masking check without using closest, so like the body of the function needMaskingText, but without having to move to node.parentElement, and only doing classList.contains/classMatchesRegex and el.matches(maskTextSelector)
  • pass the value from that down recursively, so descendents don't have to redo the check if it is true

That might be a bit slower than your method, but would be faster for the regex case and would mean we'd have the masking logic all in one place.

@eoghanmurray
Copy link
Contributor

I've embodied what I've described above in #1349

Would you be able to compare it from a performance perspective in the same way you did your one?

My take turned out to be not as elegant as I hoped as it exposed that needMaskingText is also used during mutations, and I'm not sure whether it will correctly check the ancestors in the regexp case.

eoghanmurray added a commit that referenced this pull request Nov 24, 2023
… the DOM (#1349)

* masking performance: avoid the repeated calls to `closest` when recursing through the DOM
 - needsMask===true means that an ancestor has tested positively for masking, and so this node and all descendents should be masked
 - needsMask===false means that no ancestors have tested positively for masking, we should check each node encountered
 - needsMask===undefined means that we don't know whether ancestors are masked or not (e.g. after a mutation) and should look up the tree
* Add tests including an explicit characterData mutation tests 
* Further performance improvement: avoid calls to `el.matches` when on a leaf node, e.g. a `<br/>`
---------

Authored-by: eoghanmurray <eoghan@getthere.ie>
Based on initial PR #1338 by Alexey Babik <alexey.babik@noibu.com>
@eoghanmurray eoghanmurray changed the title Optimize full snapshot serialization performance [masking] Optimize full snapshot serialization performance Feb 26, 2024
@eoghanmurray
Copy link
Contributor

#1349 is merged so closing this one

jxiwang pushed a commit to amplitude/rrweb that referenced this pull request Jul 31, 2024
… the DOM (rrweb-io#1349)

* masking performance: avoid the repeated calls to `closest` when recursing through the DOM
 - needsMask===true means that an ancestor has tested positively for masking, and so this node and all descendents should be masked
 - needsMask===false means that no ancestors have tested positively for masking, we should check each node encountered
 - needsMask===undefined means that we don't know whether ancestors are masked or not (e.g. after a mutation) and should look up the tree
* Add tests including an explicit characterData mutation tests 
* Further performance improvement: avoid calls to `el.matches` when on a leaf node, e.g. a `<br/>`
---------

Authored-by: eoghanmurray <eoghan@getthere.ie>
Based on initial PR rrweb-io#1338 by Alexey Babik <alexey.babik@noibu.com>
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.

2 participants