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

ref: isParentRemoved to cache subtree #1543

Merged
merged 10 commits into from
Oct 4, 2024

Conversation

JonasBa
Copy link
Contributor

@JonasBa JonasBa commented Jul 20, 2024

isParentRemoved is called super often for each node that we add. I think the current logic results in an On^2 time complexity as each visit to a node traverser the entire chain of parents (please correct me if I'm wrong).

The motivation behind this change is to iterate on the graph once ahead of time which can be done in On time and store the parent nodes which were removed. That way we can query the removed set in O1 time and only perform this lookup once per node

Related to #1271

Copy link

changeset-bot bot commented Jul 20, 2024

🦋 Changeset detected

Latest commit: b10de2e

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

This PR includes changesets to release 19 packages
Name Type
rrweb Major
@rrweb/rrweb-plugin-canvas-webrtc-record Major
@rrweb/rrweb-plugin-canvas-webrtc-replay Major
@rrweb/rrweb-plugin-console-record Major
@rrweb/rrweb-plugin-console-replay Major
@rrweb/rrweb-plugin-sequential-id-record Major
@rrweb/rrweb-plugin-sequential-id-replay Major
rrweb-snapshot Major
rrdom Major
rrdom-nodejs Major
rrweb-player Major
@rrweb/all Major
@rrweb/replay Major
@rrweb/record Major
@rrweb/types Major
@rrweb/packer Major
@rrweb/utils Major
@rrweb/web-extension Major
rrvideo Major

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

@JonasBa JonasBa marked this pull request as ready for review July 20, 2024 13:39
@JonasBa
Copy link
Contributor Author

JonasBa commented Jul 20, 2024

@eoghanmurray I think this will substantially improve performance for nested trees. We dont really benchmark these much besides the deep nested tree test I added, but this shows a 30% improvement on the create 1000x 1 DOM nodes with deeply nested children benchmark. On a side note, I think we should work on making the output of benchmarks friendlier as it's kind of annoying to share.

Before
CleanShot 2024-07-20 at 09 52 48@2x
After
CleanShot 2024-07-20 at 09 49 06@2x

@Juice10
Copy link
Contributor

Juice10 commented Sep 17, 2024

On a side note, I think we should work on making the output of benchmarks friendlier as it's kind of annoying to share.

@JonasBa totally agree with you, I've spent some time integrating vitest's benchmark features which should help. Here is an example output for this performance improvement, which would suggest that your performance improvement really hit its mark:

 ✓ test/benchmark/dom-mutation.bench.ts (12) 377261ms
   ✓ benchmark: mutation observer: 'create 1000x 1 DOM nodes with deeply …' (2) 247290ms
     name              hz       min        max       mean        p75        p99       p995       p999      rme  samples
   · local rrweb   0.2603    750.04   6,525.87   3,841.00   5,304.50   6,525.87   6,525.87   6,525.87  ±34.37%       10   fastest
   · latest rrweb  0.0659  1,922.43  27,734.97  15,167.97  22,735.11  27,734.97  27,734.97  27,734.97  ±39.73%       10
   ✓ benchmark: mutation observer: 'create 1000x10 DOM nodes' (2) 35445ms
     name              hz     min       max      mean       p75       p99      p995      p999      rme  samples
   · local rrweb   0.7420  222.91  2,521.98  1,347.76  1,932.18  2,521.98  2,521.98  2,521.98  ±39.86%       10   fastest
   · latest rrweb  0.7333  222.95  2,571.04  1,363.65  1,971.98  2,571.04  2,571.04  2,571.04  ±40.14%       10
   ✓ benchmark: mutation observer: 'create 1000x10x2 DOM nodes and remove…' (2) 56692ms
     name              hz     min       max      mean       p75       p99      p995      p999      rme  samples
   · local rrweb   0.6453  311.46  2,656.83  1,549.71  2,165.12  2,656.83  2,656.83  2,656.83  ±34.16%       10   fastest
   · latest rrweb  0.3609  323.76  4,819.88  2,771.15  3,986.68  4,819.88  4,819.88  4,819.88  ±36.90%       10
   ✓ benchmark: mutation observer: 'create 1000 DOM nodes and append into…' (2) 6043ms
     name              hz     min     max    mean     p75     p99    p995    p999      rme  samples
   · local rrweb   3.8657  105.66  368.41  258.69  328.98  368.41  368.41  368.41  ±48.83%        5
   · latest rrweb  3.9143  106.55  362.30  255.48  324.03  362.30  362.30  362.30  ±48.45%        5   fastest
   ✓ benchmark: mutation observer: 'create 10000 DOM nodes and move it to…' (2) 24879ms
     name              hz     min       max      mean       p75       p99      p995      p999      rme  samples
   · local rrweb   0.8378  347.70  1,915.17  1,193.59  1,605.66  1,915.17  1,915.17  1,915.17  ±63.53%        5   fastest
   · latest rrweb  0.8371  342.64  1,916.61  1,194.61  1,612.96  1,916.61  1,916.61  1,916.61  ±63.76%        5
   ✓ benchmark: mutation observer: 'modify attributes on 10000 DOM nodes' (2) 6900ms
     name              hz     min     max    mean     p75     p99    p995    p999     rme  samples
   · local rrweb   5.0940  162.96  240.62  196.31  209.97  240.62  240.62  240.62  ±8.65%       10   fastest
   · latest rrweb  5.0144  165.47  240.62  199.42  212.38  240.62  240.62  240.62  ±8.81%       10


 BENCH  Summary

  local rrweb - test/benchmark/dom-mutation.bench.ts > benchmark: mutation observer: 'create 1000x 1 DOM nodes with deeply …'
    3.95x faster than latest rrweb

  local rrweb - test/benchmark/dom-mutation.bench.ts > benchmark: mutation observer: 'create 1000x10 DOM nodes'
    1.01x faster than latest rrweb

  local rrweb - test/benchmark/dom-mutation.bench.ts > benchmark: mutation observer: 'create 1000x10x2 DOM nodes and remove…'
    1.79x faster than latest rrweb

  latest rrweb - test/benchmark/dom-mutation.bench.ts > benchmark: mutation observer: 'create 1000 DOM nodes and append into…'
    1.01x faster than local rrweb

  local rrweb - test/benchmark/dom-mutation.bench.ts > benchmark: mutation observer: 'create 10000 DOM nodes and move it to…'
    1.00x faster than latest rrweb

  local rrweb - test/benchmark/dom-mutation.bench.ts > benchmark: mutation observer: 'modify attributes on 10000 DOM nodes'
    1.02x faster than latest rrweb

@JonasBa
Copy link
Contributor Author

JonasBa commented Sep 18, 2024

On a side note, I think we should work on making the output of benchmarks friendlier as it's kind of annoying to share.

@JonasBa totally agree with you, I've spent some time integrating vitest's benchmark features which should help. Here is an example output for this performance improvement, which would suggest that your performance improvement really hit its mark:

 ✓ test/benchmark/dom-mutation.bench.ts (12) 377261ms
   ✓ benchmark: mutation observer: 'create 1000x 1 DOM nodes with deeply …' (2) 247290ms
     name              hz       min        max       mean        p75        p99       p995       p999      rme  samples
   · local rrweb   0.2603    750.04   6,525.87   3,841.00   5,304.50   6,525.87   6,525.87   6,525.87  ±34.37%       10   fastest
   · latest rrweb  0.0659  1,922.43  27,734.97  15,167.97  22,735.11  27,734.97  27,734.97  27,734.97  ±39.73%       10
   ✓ benchmark: mutation observer: 'create 1000x10 DOM nodes' (2) 35445ms
     name              hz     min       max      mean       p75       p99      p995      p999      rme  samples
   · local rrweb   0.7420  222.91  2,521.98  1,347.76  1,932.18  2,521.98  2,521.98  2,521.98  ±39.86%       10   fastest
   · latest rrweb  0.7333  222.95  2,571.04  1,363.65  1,971.98  2,571.04  2,571.04  2,571.04  ±40.14%       10
   ✓ benchmark: mutation observer: 'create 1000x10x2 DOM nodes and remove…' (2) 56692ms
     name              hz     min       max      mean       p75       p99      p995      p999      rme  samples
   · local rrweb   0.6453  311.46  2,656.83  1,549.71  2,165.12  2,656.83  2,656.83  2,656.83  ±34.16%       10   fastest
   · latest rrweb  0.3609  323.76  4,819.88  2,771.15  3,986.68  4,819.88  4,819.88  4,819.88  ±36.90%       10
   ✓ benchmark: mutation observer: 'create 1000 DOM nodes and append into…' (2) 6043ms
     name              hz     min     max    mean     p75     p99    p995    p999      rme  samples
   · local rrweb   3.8657  105.66  368.41  258.69  328.98  368.41  368.41  368.41  ±48.83%        5
   · latest rrweb  3.9143  106.55  362.30  255.48  324.03  362.30  362.30  362.30  ±48.45%        5   fastest
   ✓ benchmark: mutation observer: 'create 10000 DOM nodes and move it to…' (2) 24879ms
     name              hz     min       max      mean       p75       p99      p995      p999      rme  samples
   · local rrweb   0.8378  347.70  1,915.17  1,193.59  1,605.66  1,915.17  1,915.17  1,915.17  ±63.53%        5   fastest
   · latest rrweb  0.8371  342.64  1,916.61  1,194.61  1,612.96  1,916.61  1,916.61  1,916.61  ±63.76%        5
   ✓ benchmark: mutation observer: 'modify attributes on 10000 DOM nodes' (2) 6900ms
     name              hz     min     max    mean     p75     p99    p995    p999     rme  samples
   · local rrweb   5.0940  162.96  240.62  196.31  209.97  240.62  240.62  240.62  ±8.65%       10   fastest
   · latest rrweb  5.0144  165.47  240.62  199.42  212.38  240.62  240.62  240.62  ±8.81%       10


 BENCH  Summary

  local rrweb - test/benchmark/dom-mutation.bench.ts > benchmark: mutation observer: 'create 1000x 1 DOM nodes with deeply …'
    3.95x faster than latest rrweb

  local rrweb - test/benchmark/dom-mutation.bench.ts > benchmark: mutation observer: 'create 1000x10 DOM nodes'
    1.01x faster than latest rrweb

  local rrweb - test/benchmark/dom-mutation.bench.ts > benchmark: mutation observer: 'create 1000x10x2 DOM nodes and remove…'
    1.79x faster than latest rrweb

  latest rrweb - test/benchmark/dom-mutation.bench.ts > benchmark: mutation observer: 'create 1000 DOM nodes and append into…'
    1.01x faster than local rrweb

  local rrweb - test/benchmark/dom-mutation.bench.ts > benchmark: mutation observer: 'create 10000 DOM nodes and move it to…'
    1.00x faster than latest rrweb

  local rrweb - test/benchmark/dom-mutation.bench.ts > benchmark: mutation observer: 'modify attributes on 10000 DOM nodes'
    1.02x faster than latest rrweb

Nice work @Juice10, is that available on master? I need to rebase some of these PRs, and I will remove the script benchmark change here (iirc I added it because something was broken)

@Juice10
Copy link
Contributor

Juice10 commented Sep 19, 2024

@JonasBa Not yet on master, but hopefully I'll be able to finish it and create a PR for it today

@JonasBa
Copy link
Contributor Author

JonasBa commented Sep 19, 2024

Sounds good, I went ahead and reverted the change to the profiling script

@JonasBa
Copy link
Contributor Author

JonasBa commented Sep 23, 2024

@Juice10 Can we merge this, or do we need a separate review?

@Juice10 Juice10 merged commit 53b83bb into rrweb-io:master Oct 4, 2024
6 checks passed
@Juice10
Copy link
Contributor

Juice10 commented Oct 4, 2024

Thanks Jonas!

@JonasBa
Copy link
Contributor Author

JonasBa commented Oct 4, 2024

Thank you @Juice10!

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