Performance of document.title and <node>.textContent

[David Vest]

Hi!

A couple of days ago I landed two[1][2] patches to fix a seemingly
fairly trivial spec issue with setting of document.title. The bug[3] had
been simmering in the webkit bug tracker for a couple of years.

Yesterday I got copied on a performance regression[4] which indeed
sounded scary, so I started to look into it.

It turns out that my fix has made case 1 of

http://jsperf.com/setting-document-title

a magnitude slower by removing an optimization to reuse text node
children because not being allowed by the spec. I filed for a revert[5]
which are currently on the way in.

So when widening the scope a bit, how does other similar attributes work
in Blink? Take <node>.textContent, that do follow the spec by not
reusing nodes. Imagine we we had the same optimization for that?

In http://jsperf.com/setting-element-textcontent

you roughly see the same magnitude difference between the two cases in
Google Chrome (I use 33.0.1736.3 dev) as we had *after* my patch for
document.title.

Adding the same optimization to .textContent makes it 10x faster when
just replacing a string with another string! And <node>.textContent
should be a lot more performance sensitive than document.title. (It
would break the spec and perhaps would have web compat problems and what
not...)

So one line of thinking is that if we can have the slow but correct
.textContent, we should be able to have the slow and correct
document.title? Or should we focus on trying to make stuff like
.textContent fast and "wrong" but redefine "wrong"? I don't know how
realistic that it though.

Thoughts on this?

David

[1] https://codereview.chromium.org/107513013/ - Remove children of title node before updating
[2] https://codereview.chromium.org/128603002/ - Inhibit title update when removing children before setting new value
[3] https://bugs.webkit.org/show_bug.cgi?id=28864 - Setting document.title reuses <title>'s textnode child
[4] https://code.google.com/p/chromium/issues/detail?id=331076 - 3%-9% regression in dom_perf/Total/score at 242107:242126
[5] https://codereview.chromium.org/138213005/ - Revert "Remove children of title node before updating"

[Philip Jägenstedt]

On Wed, Jan 15, 2014 at 5:07 PM, David Vest <da...@opera.com> wrote:
> Hi!
>
> A couple of days ago I landed two[1][2] patches to fix a seemingly
> fairly trivial spec issue with setting of document.title. The bug[3] had
> been simmering in the webkit bug tracker for a couple of years.
>
> Yesterday I got copied on a performance regression[4] which indeed
> sounded scary, so I started to look into it.
>
> It turns out that my fix has made case 1 of
>
> http://jsperf.com/setting-document-title
>
> a magnitude slower by removing an optimization to reuse text node
> children because not being allowed by the spec.

This intrigued me, so I tried this:

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/2749

Blink/WebKit fail, while Gecko and Presto pass. I don't have IE11
available right now, can someone else check?

Assuming that all other engines agree, I think this is clearly a case
where interoperability should take precedence.

Philip

[Hajime Morrita]

Generally speaking, spec conformance vs. performance is already trade-off and we need to decide which to take case by case.

And in this case, I'd just accept the slow down and rebaseline the expectation so that we agree to other engines. I don't think document.title is such a hot path for majority of sane applications. And fortunately, the dom_perf benchmark isn't part of popular benchmarks like Dromaeo (Yay!!)

FYI, dom_perf was once moved to WebKit so you can see what is the likely culprit although the bot uses an internal copy of it. I guess this one went slowing down due to the change:

http://trac.webkit.org/browser/trunk/PerformanceTests/DOM/resources/dom-perf/accessors.js

Which does:

----

Accessors.DocumentSet = function() {

    var nLoops = kVeryTinyCount;

    for (var loop = 0; loop < nLoops; loop++)

        document.title = "name";

}

----

If I remember correctly, dom_perf was added to track V8 binding performance, and my guess is that document.title was picked here just because it seemed a trivial accessor like Node.firstChild. But the change revealed that the assumption was wrong.

BTW, I hope more benchmarks to be publicly accessible as many non-Google folks are working on performance critical pieces.

--

morrita

[Hajime Morrita]

On Wed, Jan 15, 2014 at 8:17 PM, Hajime Morrita <mor...@chromium.org> wrote:

Generally speaking, spec conformance vs. performance is already trade-off and we need to decide which to take case by case.

s/already/always/

(>o<)

[Zlip792 .]

IE11 shows just empty in front of "log:" 

and throw this:

rendering mode: CSS1Compat
document.title: FAIL

Regards

[Adam Barth]

On Wed, Jan 15, 2014 at 3:17 AM, Hajime Morrita <mor...@chromium.org> wrote:

Generally speaking, spec conformance vs. performance is already trade-off and we need to decide which to take case by case.

And in this case, I'd just accept the slow down and rebaseline the expectation so that we agree to other engines. I don't think document.title is such a hot path for majority of sane applications. And fortunately, the dom_perf benchmark isn't part of popular benchmarks like Dromaeo (Yay!!)

FYI, dom_perf was once moved to WebKit so you can see what is the likely culprit although the bot uses an internal copy of it. I guess this one went slowing down due to the change:

http://trac.webkit.org/browser/trunk/PerformanceTests/DOM/resources/dom-perf/accessors.js

Which does:

----

Accessors.DocumentSet = function() {

    var nLoops = kVeryTinyCount;

    for (var loop = 0; loop < nLoops; loop++)

        document.title = "name";

}

----

If I remember correctly, dom_perf was added to track V8 binding performance, and my guess is that document.title was picked here just because it seemed a trivial accessor like Node.firstChild. But the change revealed that the assumption was wrong.

We should just delete that subtest.  It's a pretty meaningless benchmark.

BTW, I hope more benchmarks to be publicly accessible as many non-Google folks are working on performance critical pieces.

+1

Some benchmarks we need to keep private because we don't own the copyright on the data used in the benchmark, but that's not the case for dom_perf.  We already made it public when we contributed it to WebKit.  We just need to make the copy we run on the bots public as well.

Adam

[Elliott Sprehn]

We should get the spec changed so assigning a value reuses the text node for textContent. We already have a thing like that for innerHTML. Developers shouldn't need to do silly stuff like element.firstChild.nodeValue = "foo" to avoid the overhead of allocating and inserting a Text node, I doubt most even realize it replaces the node itself.

[Eric Seidel]

Agreed.

element.textContent = "a";
var a = element.firstChild;
element.textContent = "b";
var b = element.firstChild;

I really don't think that developers care weather "a == b" or not.
But I'm sure they care that element.innerHTML = "small string" is fast
and avoiding even one malloc for a new node makes sense!

[Erik Arvidsson]

I agree that we should get the spec changed. One concern though is that I doubt this is a real issue in real apps. For micro benchmarks we should see if we can make new text nodes less slow.

--
erik

[Eric Seidel]

Actually, I think I might retract my earlier statement. It was based
on me remembering that innerHTML performance was a big sore spot when
we re-wrote the HTML parser. But after some digging, I find that was
also due to a (bad!) microbenchmark:
https://bugs.webkit.org/show_bug.cgi?id=48719

Also, was entertained to find that this isn't the first time this
optimization has come up:
https://bugs.webkit.org/show_bug.cgi?id=18169

Based on the data from that js-perf that FF is an order of magnitude
slower on these microbenchmarks, maybe this doesn't actually matter.
It does seem really silly that element.textContent causes this extra
malloc... but maybe that's just noise.

[Elliott Sprehn]

It's not the malloc I'm worried about, it's the churn with destroying the renderer and the line box tree boxes and then the layout. There's no trivial way to optimize that case since we remove the text node from the DOM and insert a whole new one.

Assigning a text string shouldn't require so much churn, and adding lots of layers of optimization complexity are silly if we can just reuse the text node and stuff a new string into its line box. If that string is the same width we can even avoid layout... None of that is possible without huge hacks if we're going to remove the node.

[Erik Arvidsson]

On Wed, Jan 15, 2014 at 1:50 PM, Elliott Sprehn <esp...@google.com> wrote:

It's not the malloc I'm worried about, it's the churn with destroying the renderer and the line box tree boxes and then the layout. There's no trivial way to optimize that case since we remove the text node from the DOM and insert a whole new one.

Assigning a text string shouldn't require so much churn, and adding lots of layers of optimization complexity are silly if we can just reuse the text node and stuff a new string into its line box. If that string is the same width we can even avoid layout... None of that is possible without huge hacks if we're going to remove the node.

Agreed, I had a quick look and I don't think there is a lot of room for improvement without changing/breaking the spec.

--
erik

[David Vest]

Hajime Morrita <mor...@chromium.org> writes:

> Generally speaking, spec conformance vs. performance is already trade-off
> and we need to decide which to take case by case.
>
> And in this case, I'd just accept the slow down and rebaseline the
> expectation so that we agree to other engines.

How exactly do I rebaseline the expectation in this case?

> I don't think document.title is such a hot path for majority of sane
> applications. And fortunately, the dom_perf benchmark isn't part of
> popular benchmarks like Dromaeo (Yay!!)
>
> FYI, dom_perf was once moved to WebKit so you can see what is the likely
> culprit although the bot uses an internal copy of it. I guess this one went
> slowing down due to the change:
>
> http://trac.webkit.org/browser/trunk/PerformanceTests/DOM/resources/dom-perf/accessors.js
>
> Which does:
> ----
> Accessors.DocumentSet = function() {
> var nLoops = kVeryTinyCount;
> for (var loop = 0; loop < nLoops; loop++)
> document.title = "name";
> }
> ----
>
> If I remember correctly, dom_perf was added to track V8 binding
> performance, and my guess is that document.title was picked here just
> because it seemed a trivial accessor like Node.firstChild. But the change
> revealed that the assumption was wrong.

Thanks for your help and investigation!

Putting this back in is now at
https://codereview.chromium.org/139313009/

David

[Anne van Kesteren]

On Wed, Jan 15, 2014 at 7:02 PM, Erik Arvidsson <a...@chromium.org> wrote:
> Agreed, I had a quick look and I don't think there is a lot of room for
> improvement without changing/breaking the spec.

We already went through this once:
http://lists.w3.org/Archives/Public/www-dom/2012JanMar/thread.html#msg38


--
http://annevankesteren.nl/

[David Vest]

Elliott Sprehn <esp...@google.com> writes:

> We should get the spec changed so assigning a value reuses the text node
> for textContent. We already have a thing like that for innerHTML.
> Developers shouldn't need to do silly stuff like
> element.firstChild.nodeValue = "foo" to avoid the overhead of allocating
> and inserting a Text node, I doubt most even realize it replaces the node
> itself.

I agree. After getting the document.title thing out of the way, I'll
look into what's possible to achieve here.

A while back I filed (and later marked INVALID) a spec bug for this
optimization to <title>.text:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=24240

because I found out that Firefox (and Blink/Webkit) actually seemed to
reuse the child in that particular case, which presumably would make the
spec easier to change. See comment 3 for demo.

David

[David Vest]

Thanks for the link and sorry for bringing up an old topic. The thread
seems to stop at a mail from you:

"Has anyone run the numbers yet to see if this is actually worth it? To
do this (and also for innerHTML) requires quite a bit of special casing
and seems more complicated than the benefits it brings."

According to my testing

http://jsperf.com/setting-element-textcontent

the speed difference is about 10x for the fast-path in Blink. I can't
say how much of that translate into benefits for the web at large.

David

[Anne van Kesteren]

That seems quite a bit. It might be worth revisiting this on
www...@w3.org then. I recommend copying Boris Zbarsky and Olli Pettay
on the email, and maybe Robert O'Callahan.


--
http://annevankesteren.nl/

[Hajime Morrita]

On Thu, Jan 16, 2014 at 7:04 PM, David Vest <da...@opera.com> wrote:

Hajime Morrita <mor...@chromium.org> writes:

> Generally speaking, spec conformance vs. performance is already trade-off
> and we need to decide which to take case by case.
>
> And in this case, I'd just accept the slow down and rebaseline the
> expectation so that we agree to other engines.

How exactly do I rebaseline the expectation in this case?

You can just ask someone  (probably one who filed the bug) to do so.

IIRC, the perf baseline is maintained in the internal repo.

[Ian Hickson]

On Wed, 15 Jan 2014, David Vest wrote:
>
> A couple of days ago I landed two[1][2] patches to fix a seemingly
> fairly trivial spec issue with setting of document.title. The bug[3] had
> been simmering in the webkit bug tracker for a couple of years.

By the way, the spec just changed to make document.title just use
textContent, rather than doing the node manipulation itself.

--
Ian Hickson U+1047E )\._.,--....,'``. fL
http://ln.hixie.ch/ U+263A /, _.. \ _\ ;`._ ,.
Things that are impossible just take longer. `._.-(,_..'--(,_..'`-.;.'

[Mathias Bynens]

On Fri, Jan 17, 2014 at 7:56 PM, Ian Hickson <i...@hixie.ch> wrote:
> On Wed, 15 Jan 2014, David Vest wrote:
>>
>> A couple of days ago I landed two[1][2] patches to fix a seemingly
>> fairly trivial spec issue with setting of document.title. The bug[3] had
>> been simmering in the webkit bug tracker for a couple of years.
>
> By the way, the spec just changed to make document.title just use
> textContent, rather than doing the node manipulation itself.

For future reference, here’s that change: http://html5.org/r/8404