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

RESOLVED FIXED in Firefox 55

Status

()

P5
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: gal, Assigned: jandem)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla55
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:investigate][qf])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Posted file 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.
Summary: accessing document properties is very slow → accessing expando properties on document is very slow
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.
Bz, this is probably a dupe but not sure which. Bug 965992?
Flags: needinfo?(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.
Component: DOM → JavaScript Engine: JIT
Flags: needinfo?(bzbarsky)
Summary: accessing expando properties on document is very slow → setting an already-existing expando property on document is very slow
> 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
Depends on: 965992
Blocks: 1307062
Priority: -- → P5
Jan, as long as you're adding ICs, thoughts on this one?
Flags: needinfo?(jdemooij)
(Assignee)

Comment 6

2 years ago
(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.
Blocks: 1259927
We need telemetry to determine the impact of this. Set to investigate for now.
Whiteboard: [qf:investigate][qf]
(Assignee)

Comment 8

2 years ago
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.
(Assignee)

Comment 9

2 years ago
Posted patch PatchSplinter Review
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).
Assignee: nobody → jdemooij
Flags: needinfo?(jdemooij)
Attachment #8849537 - Flags: review?(evilpies)
Comment on attachment 8849537 [details] [diff] [review]
Patch

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

Very straightforward patch for such a huge perf win.
Attachment #8849537 - Flags: review?(evilpies) → review+
> 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.  ;)
(Assignee)

Comment 12

2 years ago
(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).
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?
(Assignee)

Comment 14

2 years ago
(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

2 years ago
Pushed by jandemooij@gmail.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/485abf63b62b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.