1133423 - setting an already-existing expando property on document is very slow

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P5)

Description

[Andreas Gal :gal]

Attachment: test.html

This was found by Neil Gershenfeld at MIT. Accessing properties of document is extremely slow in Firefox in comparison to Chrome.

The attached test case takes 6576 ms in Firefox and 25 ms in Chrome.

Comment 1

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

if the expando is set to an Element, I see 22ms on Nightly and 30ms in Chrome.

This is all about the proxy setup we have for Document.

Comment 2

[Peter Van der Beken [:peterv]]

Bz, this is probably a dupe but not sure which. Bug 965992?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yes, exactly.  I'd actually forgotten about that one...

That said, this testcase has two things that are slow-ish.  There's the get, which is bug 965992.  And there's the set.  In practice, for this testcase, the Proxy::get is about 16% of the time and the Proxy::set is 82% of the time.  That's because the set ends up in DOMProxyHandler::set which does getOwnPropDescriptor (21% of the time, mostly the JS_GetPropertyDescriptorById on the expando object) and then the defineProperty bits which do the actual set, mostly under js_DefineOwnProperty.

So let's do the get in bug 965992 and then the set in this bug.

That will still leave _adding_ expando props to a document being somewhat slow, but I'm not sure how much we care about that.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> but I'm not sure how much we care about that.

Because if I change the testcase to do:

  document[n] = n;

instead of document.x++, then I see us take about 6s still, while

Comment 5

[Boris Zbarsky [:bzbarsky]]

Jan, as long as you're adding ICs, thoughts on this one?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #5)
> Jan, as long as you're adding ICs, thoughts on this one?

I'm currently working on converting our SETPROP/SETELEM stubs to CacheIR. Once that's done (few weeks from now probably) it will be easy to optimize this. It's going to be very similar to what we did for *getting* expando properties in bug 965992 part 2. Keeping the needinfo until then.

Comment 7

[Naveed Ihsanullah [:naveed]]

We need telemetry to determine the impact of this. Set to investigate for now.

Comment 8

[Jan de Mooij [:jandem]]

I fixed the main blocker this week. I'll have a patch for this next week or so. Adding Telemetry for this will likely take more time than writing the patch.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Patch

This optimizes both setting expando properties and expando setter calls.

It improves the attached testcase from 3711 ms to 94 ms, about 40x faster. I get 25 ms in Chrome but I wonder if maybe |document| is not a proxy in Blink? If I use a NodeList instead we are faster than them with this patch.

Anyway, this is a big improvement and we can improve this more by compiling CacheIR to MIR (bug 1324561).

Comment 10

[Tom S [:evilpie]]

Comment on attachment 8849537 [details] [diff] [review]
Patch

Review of attachment 8849537 [details] [diff] [review]:
-----------------------------------------------------------------

Very straightforward patch for such a huge perf win.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> but I wonder if maybe |document| is not a proxy in Blink?

Correct, it's not.

I don't understand why the changes to dom/bindings/test/test_proxy_expandos.html are needed in the patch...

Apart from that, this looks awesome.  ;)

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #11)
> I don't understand why the changes to
> dom/bindings/test/test_proxy_expandos.html are needed in the patch...

I added this test for bug 965992. It only tests *getting* expando properties (data properties + getters). I changed it to also test the new cases we're optimizing here (setting properties + setters).

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.  I guess the |obj.foo = "bar";| bit in the test is just dead code, then?  Or is it needed to ensure that in the loop we are always setting an existing prop?

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #13)
> Or is it needed to ensure that in the loop we are always setting an
> existing prop?

Yeah I kept it for that reason, but we could remove it and the loop will still test the same things after the first iteration (a single failure-to-attach is not going to disable the IC + the first iteration likely runs in the interpreter anyway).

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/485abf63b62b
Optimize sets of expando properties and expando setter calls on DOM proxies. r=evilpie

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/485abf63b62b