⚗ [RUMF-902] implement a new mutation observer

[BenoitZugmeyer]

Motivation

The current mutation observer logic is complicated and inefficient. Let's implement a new mutation observer.

Changes

• Introduce a few utility functions that were partially reverted from Revert "[RUMF-895] cleanup the global "mirror" variable (#801)" #805. This makes getting serialized node ids from DOM node a bit safer thanks to distinct TS types.
• Implement a new mutation observer
• Use the new mutation observer behind a FF, and adjust existing tests

Testing

Manual, unit/e2e

Perf impact:

Before:

SDK time: about 1900ms

After:

SDK time: about 560ms

---

I have gone over the contributing documentation.

[codecov-commenter]

Codecov Report

Merging #810 (0d860e0) into master (270cff1) will increase coverage by 0.57%.
The diff coverage is 91.44%.

@@            Coverage Diff             @@
##           master     #810      +/-   ##
==========================================
+ Coverage   86.24%   86.81%   +0.57%     
==========================================
  Files          78       80       +2     
  Lines        3737     3869     +132     
  Branches      848      879      +31     
==========================================
+ Hits         3223     3359     +136     
+ Misses        514      510       -4     

Impacted Files	Coverage Δ
packages/rum-recorder/src/boot/recorder.ts	100.00% <ø> (ø)
packages/rum-recorder/src/domain/rrweb/types.ts	100.00% <ø> (ø)
packages/rum-recorder/src/domain/rrweb/utils.ts	82.97% <ø> (-1.34%)	⬇️
packages/rum-recorder/src/domain/rrweb/observer.ts	76.86% <48.14%> (-1.77%)	⬇️
packages/rum-recorder/test/utils.ts	83.87% <85.71%> (+4.40%)	⬆️
.../rum-recorder/src/domain/rrweb/mutationObserver.ts	99.09% <99.09%> (ø)
...er/src/domain/rrweb-snapshot/serializationUtils.ts	100.00% <100.00%> (ø)
...rum-recorder/src/domain/rrweb-snapshot/snapshot.ts	82.37% <100.00%> (+0.81%)	⬆️
packages/rum-recorder/src/domain/rrweb/mutation.ts	65.15% <100.00%> (+0.47%)	⬆️
packages/rum-recorder/src/domain/rrweb/record.ts	64.00% <100.00%> (ø)
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 270cff1...0d860e0. Read the comment docs.

[bcaudan]

Great work 🎉

[vlad-mh]

nice, super exciting perf wins! let's test it out
some nits, but mostly ideas for further improvements

[vlad-mh]

some optimization thoughts, probably for a later PR:

• both nodeOrAncestorsIsIgnored and nodeOrAncestorsShouldBeHidden walk the parent hierarchy of mutation.target. Ultimately, we should do it only once.
• document.contains by definition must be O(N), N being number of dom elements? If we walk the parent hierarchy of mutation.target anyway, we might as well just check the oldest parent is document?

[BenoitZugmeyer]

Yes I agree, this is still something I have in mind: optimizing this loop or using the 'cache' optimization could still be usefull here. In practice, I observed that this whole filter loop takes less than 1ms even when hundreds of mutations occurs.

document.contains is fast. It is not O(N), and the browser probably uses internal tricks to avoid walking on all parents. See for yourself, run this in a busy page:

{ const list = document.querySelectorAll('*'); const start = performance.now(); list.forEach(e => { while (e) { e = e.parentNode };  }); console.log(performance.now() - start) }

{ const list = document.querySelectorAll('*'); const start = performance.now(); list.forEach(e => document.contains(e)); console.log(performance.now() - start) }

[vlad-mh]

I'm nitpicking at this point, so feel free to ignore but if you processed mutation.removedNodes prior to mutation.addedNodes I think you would, in your second loop, only end up doing one lookup in the set rather than two?

in my mind:

    forEach(mutation.removedNodes, (node) => {
        removedNodes.set(node, mutation.target)
    }
    forEach(mutation.addedNodes, (node) => {
       if (!removedNodes.has(node)) {
        addedAndMovedNodes.set(node, mutation.target)
      }    
    })

[BenoitZugmeyer]

A node cannot be both removed and added in a single mutation. Then, the process order doen't matter.

[vlad-mh]

can we live with just 1 loop here? 🤔

const textMutations: TextMutation[] = []
const handledNodes = new Set<Node>()

for (n in mutations) {
   if (handledNodes.has(mutation.target)) {
       continue;
    }
    handledNodes.add(mutation.target)
    const value = mutation.target.textContent
    if (value === mutation.oldValue) {
        continue
    }
    textMutations.push({
        id: getSerializedNodeId(mutation.target),
        value,
    })

[BenoitZugmeyer]

This was my first version for both processCharacterDataMutations and processAttributesMutations. But, for attributes, the loop got a bit hairy... so I split the loop into "deduplication" and "emission". To have a consistent pattern between the two functions, I applied the same structure to character data mutations. I find it a bit easier to read, and it shouldn't matter performance wise.