[Feature Request]: Optimize full snapshot serialization performance

[ababik]

Preflight Checklist

• I have searched the issue tracker for a feature request that matches the one I want to file, without success.

What package is this feature request for?

rrweb

Problem Description

The full snapshot serialization process takes a significant amount of time, blocking the UI thread. I spent some time analyzing the process and noticed that there is a relatively expensive closest function call for each text node, which can be avoided.

Proposed Solution

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.

Alternatives Considered

N/A

Additional Information

I ran a series of performance measurements for the full snapshot serialization of a document generated by the following code:

for (let i1 = 0; i1 < 100; i1++) {
    let parent = document.createElement("div");
    document.getElementById("container").append(parent);
    for (let i2 = 0; i2 < 100; i2++) {
        let child = document.createElement("div");
        child.innerText = `DIV ${i1} - ${i2}`;
        parent.append(child);
        parent = child;
    }
}

The original function takes avg 34.55 ms while optimized is avg 27.35 ms.

[eoghanmurray]

Would it be possible to re-run your benchmark measurements against #1349 ?

I think my approach adds a little less complexity than your one, so if it performs just as good then I'd prefer to go with it, but you'll have to let me know if performance is okay!

[ababik]

Thanks for getting back to me. I'll work on this during next couple of days and let you know. Thanks.

[ababik]

I like the idea of having all masking logic in the same place; the code is more maintainable.
Although it's less performant compared to my approach, where I determine the list of elements to mask only once.
I ran the same test, and it takes ~31ms.
The original implementation (closest): 34.55 ms
Your implementation: ~31 ms
My implementation: 27.35 ms

It's definitely worth merging your optimization.

[eoghanmurray]

There are some good ideas with your approach of doing an upfront querySelectorAll('.a-mask-class') as this will definitely be faster than the repeated calls to .matches('.a-mask-class'); however I've thought of an additional potential problem with the querySelectorAll approach in that it returns a static NodeList rather than a live HTMLCollection, so if the DOM is mutated while it is being traversed, we will miss e.g. the insertion of a new that is supposed to be masked.

[eoghanmurray]

I've just updated my PR with an added check which might reduce the number of calls to .matches; it won't affect your benchmark as all your nodes have a innerText, but might help in other situations.

[patrickhousley]

Hey @eoghanmurray . This may be a stupid question but isn't the snapshot process synchronous thereby precluding any possible DOM mutations while the snapshot code is running?