962256 - Exact rooting regressed dromaeo_css and mootools by ~5%

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

See this for a starting place:
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=b3c08e6fa790&newRev=61fd0f987cf2&submit=true

We should be able to profile the harder-hit tests to find the regressing roots.

Comment 1

[Boris Zbarsky [:bzbarsky]]

More precisely, it regressed dromaeo_css by 5%.  The mootools subtest regressed by 10%.  The subtests of the mootools subtest I don't have data on from compare-talos, but it's worth running on dromaeo.com to see what's going on there.

Comment 2

[Benjamin Kerensa [:bkerensa]]

Could you help us understand how this will impact users?

Comment 3

[Boris Zbarsky [:bzbarsky]]

How a 10% regression in our handling of a popular library will impact users?  Probably by making web pages slower.

Comment 4

[Benjamin Kerensa [:bkerensa]]

(In reply to Boris Zbarsky [:bz] from comment #3)
> How a 10% regression in our handling of a popular library will impact users?
> Probably by making web pages slower.

Thanks for that I was not aware of the popularity of the library. Tracking based on Comment 3.

Comment 5

[Terrence Cole [:terrence]]

For the mootools:div~div test, most of the extra time looks like it's distributed like:

6.3% - js::Proxy::callProp
  6.1% - js::Proxy::get
    5.5% - mozilla::dom::HTMLDocumentBinding::DOMProxyHandler::get
      1.8% - JS_ForwardGetPropertyTo
      1.6% - JS_HasPropertyById
      0.9% - JS_AlreadyHasOwnPropertyById
5.2% - js::jit::DoGetElemFallback
  2.1% - proxy_GetElement
    1.9% - js::Proxy::get
      1.4% - mozilla::dom::NodeListBinding::DOMProxyHandler::get
1.5% - js::jit::DoGetPropFallback
1.2% - nsINode::QuerySelectorAll

Percentages are increase of total time.

Also of note: Call->Invoke->Invoke->RunScript->Interpret appears to be about 0.2% slower in this trace.

Comment 6

[Boris Zbarsky [:bzbarsky]]

JS_ForwardGetPropertyTo ends up re-rooting everything.  Maybe we should just handlify it?  The callsites in mozilla::dom::HTMLDocumentBinding::DOMProxyHandler::get are all already rooted.

JS_HasPropertyById is already handlified, though....  So is JS_AlreadyHasOwnPropertyById.

nsINode::QuerySelectorAll shouldn't be affected by exact rooting at all.  :(

Comment 7

[Andrew McCreight [:mccr8]]

I can take a look at handlifying JS_ForwardGetPropertyTo.  It doesn't seem to be on the list of bug 959787.

Comment 8

[Andrew McCreight [:mccr8]]

I'd guess that JS_HasPropertyById and JS_AlreadyHasOwnPropertyById are more expensive with exact rooting because they locally root the return value of a function they call (an object and a shape).  Handlification doesn't help because they aren't returning those values to any other function, they are just checking that the return value satisfies some property.

JS_HasPropertyById only checks that the returned shape isn't null (ignoring the object), and JS_AlreadyHasOwnPropertyById only checks that the returned object is the same as the object passed in (ignoring the shape).  In theory, I could imagine creating some specialized version of LookupPropertyById that avoids the need to root the return values, by performing those checks deep within the callees, but in practice it would have to happen deep within some call stack, and it seems to involve some kind of generic function callbacks, so I'm not sure if you could even do it.

Comment 9

[Terrence Cole [:terrence]]

Attachment: shave_some_roots_in_hasprop-v0.diff

Two of those four roots aren't used after the call, so it may be safish to from-marked-location on them, as attached. Unfortunately that code calls into the ops table, so if someone is using those outparams to root and expects them to stay live, it could be quite bad.

Another option would be to make the ops support <NoGC>. Or to have some ballast rooted slots around. We could also have an interface that passes in the temporaries so that the caller could shave roots, maybe using the JS stack. All of that is going to be much more code, however.

Comment 10

[Terrence Cole [:terrence]]

Attachment: shave_roots_bl_get_stubs-v0.diff

This is a bit less sketchy. It removes or delays some roots in baseline's GetProp and GetElem fallbacks. I don't think it introduces any new rooting issues, but I've kicked off a try run to verify. Please disregard the review request if that is red.

https://tbpl.mozilla.org/?tree=Try&rev=c9d5fc5339fc

It seems to save about 0.3% - 1.2%, depending on the trace. Having run a handful more traces now, I can say that the stack from main up to event dispatch is either 0% or 6% slower, depending on the trace. I'm guessing this is a layout/code size/timing issue related to adding tons of new inline code more than specific instructions. Would be nice to hit this with VTune to be sure.

The majority of the remaining overhead (~5%) that might feasibly be in our control appears to be the Rooted in Proxy's INVOKE_ON_PROTOTYPE. I'm going to try getPrototype<NoGC> next.

Comment 11

[Terrence Cole [:terrence]]

I was (unsurprisingly) reading instrument's output incorrectly. For GetElem, the above patch actually does:

          Total   Self
Without:  660ms  255ms
With:     619ms  188ms
No EXR:   536ms  160ms

Not perfect, but still a substantial improvement. Looks like there is one hazard for me to deal with.

The GetProp stub is restored to its original performance, but is too few samples to make any real conclusions either way.

Comment 12

[Terrence Cole [:terrence]]

Attachment: shave_roots_bl_get_stubs-v1.diff

This should be better.

https://tbpl.mozilla.org/?tree=Try&rev=d9056942b386

Comment 13

[Terrence Cole [:terrence]]

Attachment: A single-file testcase useful for profiling.

Proxy::get is not implicated: we bail to handler->get before creating the root. One instance of Proxy::get is even faster in the traces I'm looking at. It looks like the 1% performance shift either way is down to noise.

What *is* implicated is mozilla::dom::HTMLDocumentBinding::DOMProxyHandler::get: the self time here goes from 74ms before to 158ms afterwards. Other than that it's down to 5ms here, 7ms there. There may be some fat in the SetElementIC to trim too, including 20ms in DefinePropertyOrElement. Note, another instance of DOMProxyHandler for NodeListBinding is only a tiny bit slower too, so it may be something specific to HTMLDocumentBinding.

Comment 14

[Terrence Cole [:terrence]]

Looking at the generated code, it seems that HTMLDocumentBinding::DOMProxyHandler::get has 4 new Rooted where 1 would do and NodeListBinding::DOMProxyHandler::get has maybe 1, so the numbers above do seem reasonable.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmm.  So the Rooted in HTMLDocumentBinding::DOMProxyHandler::get are:

1)  unforgeableHolder.  Really does need to be rooted, afaict.
2)  expando.  Needs to be rooted if not null.
3)  nameVal.  Needs to be rooted, afaict.
4)  result.  This could probably be a raw JSObject*.

How would 1 Rooted do here?

Comment 16

[Terrence Cole [:terrence]]

Those 4 roots have non-overlapping lifetimes, so for the objects you could do what we do for the interpreter: Rooted<JSObject*> root; Rooted<JSObject*>& unforgeableHolder = root; Rooted<JSObject*>& expando = root; etc. Or just re-use the root with a generic name.

The Value nameVal is only used in the else branch of a MOZ_LIKELY if-statement, so could be moved into the MOZ_UNLIKELY half and hopefully not created on most calls.

Of course all this is generated code, so those interdependent pieces might be quite hard to implement in practice. I was not even able to find where any of that is generated with my brief grepping.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  I'll write up a patch to try that so we can test it.

Comment 18

[Terrence Cole [:terrence]]

Comment on attachment 8366346 [details] [diff] [review]
shave_roots_bl_get_stubs-v1.diff

Switching review, since Steve is sick this week.

Comment 19

[Jon Coppeard (:jonco)]

Comment on attachment 8366346 [details] [diff] [review]
shave_roots_bl_get_stubs-v1.diff

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

So reduces the amount of rooting by rooting things later or not at all, mostly the JSScript where this is not used past something that can GC.

As such it looks fine, except for:

::: js/src/jit/BaselineIC.cpp
@@ +3971,2 @@
>          // Handle optimized arguments[i] access.
>          if (!GetElemOptimizedArguments(cx, frame, &lhsCopy, rhs, res, &isOptimizedArgs))

It turns out that GetElemOptimizedArguments() can set isOptimizedArgs to false and also update lhsCopy, meaning that this rearrangement is not valid.

We need to root lhsCopy in the original place so that GelElementOperation() below can see the new value.

The name 'isOptimizedArgs' is pretty misleading - it seems it really means 'is optimised args, and has been handled already'.  Maybe renaming to 'handledOptimizedArgs' would make this clearer.

Comment 20

[Terrence Cole [:terrence]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fe77ee5b2957

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe77ee5b2957

Comment 22

[Boris Zbarsky [:bzbarsky]]

How do the numbers look now?  Should I still be looking into the comment 16 issue?

Comment 23

[Terrence Cole [:terrence]]

(In reply to Boris Zbarsky [:bz] from comment #22)
> How do the numbers look now?  Should I still be looking into the comment 16
> issue?

http://graphs.mozilla.org/graph.html#tests=[[72,63,21]]&sel=none&displayrange=30&datatype=running

Hard to interpret. We're maybe about halfway back to where we were on dromaeo-css on 11 Jan, but still ahead of where we were on 10 Jan? The roots in the last patch appeared to be about 20% of the overall slowdown with the roots in comment 16 being about 60%. I think we should still at least try to take a stab at those still. Deadline for uplift is this weekend; is there anything I can do to help?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Another interesting question: why are we even ending up in Proxy::get here?  What's a typical jsid coming through on those gets?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: yet another part. Eliminate some unnecessary roots in HTMLDocumentBinding::DOMProxyHandler::get.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Unnecessary root elimination, diff -w for ease of review

Note that this basically backs out bug 950991, since JSAutoCompartment in fact can't GC now

Comment 27

[Boris Zbarsky [:bzbarsky]]

I filed bug 965992 on not calling into get() here at all.  Once we land the attached patch, I think we should just mark this resolved and then focus on avoiding the slow path.  ;)

Comment 28

[Terrence Cole [:terrence]]

Agreed! I'm going to close this, as I think we've reached satisfaction with our investigation.

Comment 29

[Terrence Cole [:terrence]]

Whoops! I didn't realize that Boris' patch was still waiting on review.

Comment 30

[Sylvestre Ledru [:Sylvestre]]

And please don't forget the uplift request to 29 ;)

Comment 31

[Boris Zbarsky [:bzbarsky]]

Hrm.  Which parts of this need uplifting to 29?  We should not have landed parts of the bug without others... :(

Comment 32

[Sylvestre Ledru [:Sylvestre]]

That might sound stupid but patches which fixed this bug ? :)

Comment 33

[Boris Zbarsky [:bzbarsky]]

Yes, and which of those are not on 29 already?

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Sorry, I don't know. This bug is on my radar because it still open and tracked.

Comment 35

[Bobby Holley (:bholley)]

Comment on attachment 8368199 [details] [diff] [review]
yet another part.  Eliminate some unnecessary roots in HTMLDocumentBinding::DOMProxyHandler::get.

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

The generated code here looks reasonable enough. I'll trust Boris on the design bits. r=bholley

Comment 36

[Boris Zbarsky [:bzbarsky]]

So to answer my own question, the parts that are not my patch already landed on 29, so don't need backporting.

Comment 37

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/960a7f055c95

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8368199 [details] [diff] [review]
yet another part.  Eliminate some unnecessary roots in HTMLDocumentBinding::DOMProxyHandler::get.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Exact rooting turn on
User impact if declined: Slightly slower DOM manipulation
Testing completed (on m-c, etc.): Compiles and runs.
Risk to taking this patch (and alternatives if risky): Risk should be pretty low.
String or IDL/UUID changes made by this patch:  None.

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/960a7f055c95

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/db0cfa150cdd