Add a fast path in Element::SetInnerHTML when the value is small and trivial text #26398

Eijebong · 2020-05-03T13:21:08Z

Inspired from gecko which has a similar fast path. This makes innerHTML
more than 10 times faster for this case.

Fixes #25892

highfive · 2020-05-03T13:21:14Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmltablesectionelement.rs, components/script/dom/htmltablecolelement.rs, components/script/dom/node.rs, components/script/dom/element.rs, components/script/dom/htmltablerowelement.rs and 6 more
@KiChjang: components/script/dom/htmltablesectionelement.rs, components/script/dom/htmltablecolelement.rs, components/script/dom/node.rs, components/script/dom/element.rs, components/script/dom/htmltablerowelement.rs and 6 more

highfive · 2020-05-03T13:21:16Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

Eijebong · 2020-05-03T13:22:53Z

tests/html/test_dom_performance.html
Before:

After:

Eijebong · 2020-05-03T13:24:37Z

@bors-servo try=wpt

bors-servo · 2020-05-03T13:24:42Z

⌛ Trying commit 4fec5d6 with merge 3e00866...


        Auto merge of #26398 - Eijebong:fast-path-set-inner-html, r=<try>

Add a fast path in Element::SetInnerHTML when the value is small and trivial text Inspired from gecko which has a similar fast path. This makes innerHTML more than 10 times faster for this case. Fixes #25892

bors-servo · 2020-05-03T14:07:53Z

💔 Test failed - status-taskcluster

Eijebong · 2020-05-03T14:15:46Z

1 unexpected results that are NOT known-intermittents:
  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_innerHTML_tests_innerHTML_1.html:
  │ FAIL [expected PASS] html5lib_innerHTML_tests_innerHTML_1.html 2555d238e04f3d2853cfbc5f6dd366f82cf0e868
  │   → assert_equals: expected "#document\n| <head>\n| <body>" but got "#document"
  │ 
  │ test_fragment@http://web-platform.test:8000/html/syntax/parsing/test.js:217:16
  │ test_next/</<@http://web-platform.test:8000/html/syntax/parsing/test.js:330:29
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1988:25
  │ test_next/<@http://web-platform.test:8000/html/syntax/parsing/test.js:324:20
  └ step_timeout/<@http://web-platform.test:8000/resources/testharness.js:937:15

I have no idea what this test is even supposed to be. There's a 18391 character long line in there with what looks like sha1sums everywhere... 😕

Eijebong · 2020-05-03T14:24:07Z

My guess is that gecko also blacklists HTMLSharedElement but there's no such thing in servo.

https://searchfox.org/mozilla-central/source/dom/html/HTMLSharedElement.h#20

The documentation here is missing so I have no idea what they even are.

Eijebong · 2020-05-03T15:43:23Z

@bors-servo try=wpt


        Auto merge of #26398 - Eijebong:fast-path-set-inner-html, r=<try>

Add a fast path in Element::SetInnerHTML when the value is small and trivial text Inspired from gecko which has a similar fast path. This makes innerHTML more than 10 times faster for this case. Fixes #25892

bors-servo · 2020-05-03T15:43:29Z

⌛ Trying commit 207c477 with merge 31ad6e1...

jdm · 2020-05-03T16:06:29Z

Judging by https://searchfox.org/mozilla-central/rev/b9a814e53b3b6f5cb665a78f4777868e7a16bfcd/dom/html/HTMLSharedElement.cpp#273-293, HTMLSharedElement looks like a class that is used as the implementation for a bunch of HTML elements that each have one or two custom behaviours to reduce code duplication.

jdm · 2020-05-03T16:11:46Z

I suspect that you're missing weird parsing mode for the head element, judging by https://searchfox.org/mozilla-central/rev/b9a814e53b3b6f5cb665a78f4777868e7a16bfcd/dom/html/HTMLSharedElement.h#25-27 and the test contents being (pre-decoded) "%23document%0A%7C%20%3Chead%3E%0A%7C%20%3Cbody%3E".

Eijebong · 2020-05-03T16:21:43Z

I was actually missing it for HtmlHtmlElement. With my current patch the test seems to be ok. We'll see if something else is broken

bors-servo · 2020-05-03T16:25:05Z

💔 Test failed - status-taskcluster

Eijebong · 2020-05-03T16:36:53Z

▶ TIMEOUT [expected OK] /_webgl/conformance/glsl/misc/struct-nesting-of-variable-names.html

Hopefully unrelated :p

jdm · 2020-05-03T16:38:50Z

I argue that you are also missing it from HTMLHeadElement given the code that I linked :)

Eijebong · 2020-05-03T17:55:11Z

Oh wow I somehow missed those lines when I read it... Will add for htmlheadelement

jdm · 2020-05-03T20:09:37Z

@bors-servo r+
Thanks!

bors-servo · 2020-05-03T20:09:38Z

📌 Commit f01801d has been approved by jdm

bors-servo · 2020-05-03T20:09:43Z

⌛ Testing commit f01801d with merge 0a774b7...


        Auto merge of #26398 - Eijebong:fast-path-set-inner-html, r=jdm

Add a fast path in Element::SetInnerHTML when the value is small and trivial text Inspired from gecko which has a similar fast path. This makes innerHTML more than 10 times faster for this case. Fixes #25892

bors-servo · 2020-05-03T20:53:51Z

💔 Test failed - status-taskcluster

components/script/dom/element.rs


        Add a fast path in Element::SetInnerHTML when the value is small and …

…trivial text Inspired from gecko which has a similar fast path. This makes innerHTML more than 10 times faster for this case. Fixes #25892

jdm · 2020-05-04T16:35:06Z

@bors-servo r+

bors-servo · 2020-05-04T16:35:07Z

📌 Commit 1b2464b has been approved by jdm

bors-servo · 2020-05-04T16:43:51Z

⌛ Testing commit 1b2464b with merge 271ff16...

bors-servo · 2020-05-04T17:44:23Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 271ff16 to master...

highfive assigned ferjm May 3, 2020

highfive added the S-awaiting-review label May 3, 2020

highfive added the S-tests-failed label May 3, 2020

Eijebong force-pushed the Eijebong:fast-path-set-inner-html branch from 4fec5d6 to 207c477 May 3, 2020

highfive removed the S-tests-failed label May 3, 2020

highfive added the S-tests-failed label May 3, 2020

Eijebong force-pushed the Eijebong:fast-path-set-inner-html branch from 207c477 to f01801d May 3, 2020

highfive removed the S-tests-failed label May 3, 2020

highfive unassigned ferjm May 3, 2020

highfive assigned jdm May 3, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels May 3, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels May 3, 2020

emilio reviewed May 3, 2020

View changes

components/script/dom/element.rs Outdated Show resolved Hide resolved

Add a fast path in Element::SetInnerHTML when the value is small and …

Loading status checks…

1b2464b

…trivial text Inspired from gecko which has a similar fast path. This makes innerHTML more than 10 times faster for this case. Fixes #25892

Eijebong force-pushed the Eijebong:fast-path-set-inner-html branch from f01801d to 1b2464b May 4, 2020

highfive added S-awaiting-review and removed S-tests-failed labels May 4, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels May 4, 2020

bors-servo merged commit 271ff16 into servo:master May 4, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label May 4, 2020

jdm mentioned this pull request May 27, 2020

Crash on altspace help page (index out of bounds: the len is 0 but the index is 0) #26590

Closed

servo / servo

Add a fast path in Element::SetInnerHTML when the value is small and trivial text #26398

Add a fast path in Element::SetInnerHTML when the value is small and trivial text #26398

Eijebong commented May 3, 2020

highfive commented May 3, 2020

highfive commented May 3, 2020

Eijebong commented May 3, 2020

Eijebong commented May 3, 2020

bors-servo commented May 3, 2020

bors-servo commented May 3, 2020

Eijebong commented May 3, 2020

Eijebong commented May 3, 2020

Eijebong commented May 3, 2020

bors-servo commented May 3, 2020

jdm commented May 3, 2020

jdm commented May 3, 2020

Eijebong commented May 3, 2020

bors-servo commented May 3, 2020

Eijebong commented May 3, 2020

jdm commented May 3, 2020

Eijebong commented May 3, 2020

jdm commented May 3, 2020

bors-servo commented May 3, 2020

bors-servo commented May 3, 2020

bors-servo commented May 3, 2020

jdm commented May 4, 2020

bors-servo commented May 4, 2020

bors-servo commented May 4, 2020

bors-servo commented May 4, 2020

servo / servo

Join GitHub today

Add a fast path in Element::SetInnerHTML when the value is small and trivial text #26398

Add a fast path in Element::SetInnerHTML when the value is small and trivial text #26398

Conversation

Eijebong commented May 3, 2020

highfive commented May 3, 2020

highfive commented May 3, 2020

Eijebong commented May 3, 2020

Eijebong commented May 3, 2020

bors-servo commented May 3, 2020

bors-servo commented May 3, 2020

Eijebong commented May 3, 2020

Eijebong commented May 3, 2020

Eijebong commented May 3, 2020

bors-servo commented May 3, 2020

jdm commented May 3, 2020

jdm commented May 3, 2020

Eijebong commented May 3, 2020

bors-servo commented May 3, 2020

Eijebong commented May 3, 2020

jdm commented May 3, 2020

Eijebong commented May 3, 2020

jdm commented May 3, 2020

bors-servo commented May 3, 2020

bors-servo commented May 3, 2020

bors-servo commented May 3, 2020

jdm commented May 4, 2020

bors-servo commented May 4, 2020

bors-servo commented May 4, 2020

bors-servo commented May 4, 2020