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

perf: optimize performance of the DoubleLinkedList get #1220

Merged
merged 2 commits into from
May 22, 2023

Conversation

wfk007
Copy link
Contributor

@wfk007 wfk007 commented May 6, 2023

benchmark-dom-mutation.html

almost the same

- before average: 142.6 
+ after average: 138.9

before:

┌─────────┬────────────────────────────┬───────────────────────────────┬─────────────────────┬───────┬──────────┬────────────────────────────────────────────────────┐
    │ (index) │           title            │             html              │        eval         │ times │ duration │                     durations                      │
    ├─────────┼────────────────────────────┼───────────────────────────────┼─────────────────────┼───────┼──────────┼────────────────────────────────────────────────────┤
    │    0    │ 'create 1000x10 DOM nodes' │ 'benchmark-dom-mutation.html' │ 'window.workload()' │  10   │  142.6   │ '141, 138, 158, 136, 132, 174, 124, 126, 145, 152' │
    └─────────┴────────────────────────────┴───────────────────────────────┴─────────────────────┴───────┴──────────┴────────────────────────────────────────────────────┘

after:

┌─────────┬────────────────────────────┬───────────────────────────────┬─────────────────────┬───────┬──────────┬────────────────────────────────────────────────────┐
    │ (index) │           title            │             html              │        eval         │ times │ duration │                     durations                      │
    ├─────────┼────────────────────────────┼───────────────────────────────┼─────────────────────┼───────┼──────────┼────────────────────────────────────────────────────┤
    │    0    │ 'create 1000x10 DOM nodes' │ 'benchmark-dom-mutation.html' │ 'window.workload()' │  10   │  138.9   │ '139, 134, 151, 134, 133, 137, 168, 131, 131, 131' │
    └─────────┴────────────────────────────┴───────────────────────────────┴─────────────────────┴───────┴──────────┴────────────────────────────────────────────────────┘

benchmark-dom-mutation-add-and-remove.html

almost the same

- before average: 233.8
+ after average: 225.4

before:

┌─────────┬─────────────────────────────────────────────────────────┬──────────────────────────────────────────────┬─────────────────────┬───────┬──────────┬────────────────────────────────────────────────────┐
    │ (index) │                          title                          │                     html                     │        eval         │ times │ duration │                     durations                      │
    ├─────────┼─────────────────────────────────────────────────────────┼──────────────────────────────────────────────┼─────────────────────┼───────┼──────────┼────────────────────────────────────────────────────┤
    │    0    │ 'create 1000x10x2 DOM nodes and remove a bunch of them' │ 'benchmark-dom-mutation-add-and-remove.html' │ 'window.workload()' │  10   │  233.8   │ '228, 252, 236, 222, 234, 205, 224, 302, 203, 232' │
    └─────────┴─────────────────────────────────────────────────────────┴──────────────────────────────────────────────┴─────────────────────┴───────┴──────────┴────────────────────────────────────────────────────┘

after:

┌─────────┬─────────────────────────────────────────────────────────┬──────────────────────────────────────────────┬─────────────────────┬───────┬──────────┬────────────────────────────────────────────────────┐
    │ (index) │                          title                          │                     html                     │        eval         │ times │ duration │                     durations                      │
    ├─────────┼─────────────────────────────────────────────────────────┼──────────────────────────────────────────────┼─────────────────────┼───────┼──────────┼────────────────────────────────────────────────────┤
    │    0    │ 'create 1000x10x2 DOM nodes and remove a bunch of them' │ 'benchmark-dom-mutation-add-and-remove.html' │ 'window.workload()' │  10   │  225.4   │ '211, 236, 255, 250, 242, 208, 223, 244, 183, 202' │
    └─────────┴─────────────────────────────────────────────────────────┴──────────────────────────────────────────────┴─────────────────────┴───────┴──────────┴────────────────────────────────────────────────────┘

benchmark-dom-mutation-multiple-descendant-add.html

almost the same

- before average: 66.4
+ after average: 58

before:

┌─────────┬──────────────────────────────────────────────────────────────────┬───────────────────────────────────────────────────────┬─────────────────────┬───────┬──────────┬──────────────────────┐
    │ (index) │                              title                               │                         html                          │        eval         │ times │ duration │      durations       │
    ├─────────┼──────────────────────────────────────────────────────────────────┼───────────────────────────────────────────────────────┼─────────────────────┼───────┼──────────┼──────────────────────┤
    │    0    │ 'create 1000 DOM nodes and append into its previous looped node' │ 'benchmark-dom-mutation-multiple-descendant-add.html' │ 'window.workload()' │   5   │   66.4   │ '67, 67, 63, 65, 70' │
    └─────────┴──────────────────────────────────────────────────────────────────┴───────────────────────────────────────────────────────┴─────────────────────┴───────┴──────────┴──────────────────────┘

after:

┌─────────┬──────────────────────────────────────────────────────────────────┬───────────────────────────────────────────────────────┬─────────────────────┬───────┬──────────┬──────────────────────┐
    │ (index) │                              title                               │                         html                          │        eval         │ times │ duration │      durations       │
    ├─────────┼──────────────────────────────────────────────────────────────────┼───────────────────────────────────────────────────────┼─────────────────────┼───────┼──────────┼──────────────────────┤
    │    0    │ 'create 1000 DOM nodes and append into its previous looped node' │ 'benchmark-dom-mutation-multiple-descendant-add.html' │ 'window.workload()' │   5   │    58    │ '60, 60, 56, 58, 56' │
    └─────────┴──────────────────────────────────────────────────────────────────┴───────────────────────────────────────────────────────┴─────────────────────┴───────┴──────────┴──────────────────────┘

benchmark-dom-mutation-add-and-move.html

decrease by about 86.11%

- before average: 1625.4
+ after average: 225.8

before:

┌─────────┬───────────────────────────────────────────────────────┬────────────────────────────────────────────┬─────────────────────┬───────┬──────────┬────────────────────────────────┐
    │ (index) │                         title                         │                    html                    │        eval         │ times │ duration │           durations            │
    ├─────────┼───────────────────────────────────────────────────────┼────────────────────────────────────────────┼─────────────────────┼───────┼──────────┼────────────────────────────────┤
    │    0    │ 'create 10000 DOM nodes and move it to new container' │ 'benchmark-dom-mutation-add-and-move.html' │ 'window.workload()' │   5   │  1625.4  │ '1692, 1298, 2176, 1006, 1955' │
    └─────────┴───────────────────────────────────────────────────────┴────────────────────────────────────────────┴─────────────────────┴───────┴──────────┴────────────────────────────────┘

after:

┌─────────┬───────────────────────────────────────────────────────┬────────────────────────────────────────────┬─────────────────────┬───────┬──────────┬───────────────────────────┐
    │ (index) │                         title                         │                    html                    │        eval         │ times │ duration │         durations         │
    ├─────────┼───────────────────────────────────────────────────────┼────────────────────────────────────────────┼─────────────────────┼───────┼──────────┼───────────────────────────┤
    │    0    │ 'create 10000 DOM nodes and move it to new container' │ 'benchmark-dom-mutation-add-and-move.html' │ 'window.workload()' │   5   │  225.8   │ '235, 212, 256, 216, 210' │
    └─────────┴───────────────────────────────────────────────────────┴────────────────────────────────────────────┴─────────────────────┴───────┴──────────┴───────────────────────────┘

@changeset-bot
Copy link

changeset-bot bot commented May 6, 2023

🦋 Changeset detected

Latest commit: d4e1a64

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

This PR includes changesets to release 8 packages
Name Type
rrweb Patch
rrweb-snapshot Patch
rrdom Patch
rrdom-nodejs Patch
rrweb-player Patch
@rrweb/types Patch
@rrweb/web-extension Patch
rrvideo Patch

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

Copy link
Member

@YunFeng0817 YunFeng0817 left a comment

Choose a reason for hiding this comment

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

Looks really great!

@charliegracie
Copy link
Contributor

Hi.

Recently we were looking into the same issue it seems. I was just about to open a PR when I noticed yours. We came up with a pretty similar change in the end. The one difference is that instead of building the array of DoubleLinkedListNode elements we added a tail node to the current linked list. The change to the DoubleLinkedList is pretty trivial and then you can just walk the list backwards without having to find the last node by fetching tail each time.

I could share a diff if you are interested.

@wfk007
Copy link
Contributor Author

wfk007 commented May 15, 2023

@charliegracie Sure, I am really really interested in your change and it seems more elegant than this.

@wfk007 wfk007 force-pushed the perf/rrweb-record-linkedList-get branch from 4464146 to 80ceb0e Compare May 15, 2023 12:24
@wfk007
Copy link
Contributor Author

wfk007 commented May 16, 2023

@YunFeng0817 @charliegracie I updated this PR with the tail pointer scheme and updated the benchmark tests.

According to the last benchmark test(benchmark-dom-mutation-add-and-move.html), the performance increased by about 86.11%, and it works well in other benchmark tests too.

Could your please help me review this change at your convenience?

Copy link
Member

@YunFeng0817 YunFeng0817 left a comment

Choose a reason for hiding this comment

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

@charliegracie Could you please also take a look at this? Is this similar to your solution?

@YunFeng0817 YunFeng0817 merged commit a1ec9a2 into rrweb-io:master May 22, 2023
eoghanmurray pushed a commit to eoghanmurray/rrweb that referenced this pull request Jul 27, 2023
* perf: optimize performance of the DoubleLinkedList get

* fix: delete addedNodeIndexArr
eoghanmurray pushed a commit to eoghanmurray/rrweb that referenced this pull request Jul 27, 2023
* perf: optimize performance of the DoubleLinkedList get

* fix: delete addedNodeIndexArr
eoghanmurray pushed a commit to eoghanmurray/rrweb that referenced this pull request Jul 27, 2023
* perf: optimize performance of the DoubleLinkedList get

* fix: delete addedNodeIndexArr
eoghanmurray pushed a commit to eoghanmurray/rrweb that referenced this pull request Aug 3, 2023
* perf: optimize performance of the DoubleLinkedList get

* fix: delete addedNodeIndexArr
eoghanmurray pushed a commit to eoghanmurray/rrweb that referenced this pull request Aug 8, 2023
* perf: optimize performance of the DoubleLinkedList get

* fix: delete addedNodeIndexArr
eoghanmurray pushed a commit to eoghanmurray/rrweb that referenced this pull request Aug 8, 2023
* perf: optimize performance of the DoubleLinkedList get

* fix: delete addedNodeIndexArr
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.

3 participants