1596238 - Iterating DOM Nodes is 40% - 50% slower than Chrome using NodeIterator with a filter which doesn't find any nodes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Sean Feng [:sefeng]]

STR:
1) Visit https://mozilla.seanfeng.dev/html_standard.html
2) Wait for the page to finish loading.
3) Open the developer console to see the result.

https://mozilla.seanfeng.dev/html_standard.html is a benchmark I created, which uses NodeIterator API to iterate the DOM tree (Since it's a web page of html standards, the DOM tree is big). Check the <script> section at the bottom of the page source for more detail about the test.

Firefox Nightly
Mean: 2686.45 | 2703 | 2783.9
Median: 2673 | 2657.5 | 2760.5

Chrome
Mean: 1612.4 | 1563.518 | 1566.9
Median 1936 | 1318.0 | 1305.3

My test results reveal that we are 40% - 50% slower than Chrome. The differences may vary depend on the page structure.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Our relevant code is very AddRef/Release heavy, even in case when no AddRef/Release is needed.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1596238, Make NodeIterator less AddRef/Release heavy by trying to avoid use of strong pointer when the node is skipped because of whatToShow, r=ehsan

TreeWalker could use some similar changes, but that is a different bug. TreeWalker does use TestNode method too, which is why
the new argument is optional. A new bug will be filed for TreeWalker.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Rarely used ancient API being slow. ->qf:p5

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/776f4bc4d611
Make NodeIterator less AddRef/Release heavy by trying to avoid use of strong pointer when the node is skipped because of whatToShow, r=Ehsan

Comment 5

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/776f4bc4d611

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

On 64bit linux,
Nightly Mean: 1054 Median: 1051
Chrome Mean: 1139 Median: 1133

Comment 7

[Frederik Braun [:freddy]]

(In reply to Olli Pettay [:smaug] from comment #2)

TreeWalker could use some similar changes, but that is a different bug. TreeWalker does use TestNode method too, which is why
the new argument is optional. A new bug will be filed for TreeWalker.

Was this bug ever filed?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

No because when I was looking at it some more, I realized it doesn't have the same issue after all.