Make entering a compartment and pushing a dummy frame an atomic stack operation

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Trunk
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
Created attachment 551151 [details] [diff] [review]
patch for m-c

There's currently some twitchiness involved with pushing the dummy frame when entering a compartment (see the comments in AutoCompartment::enter and ContextStack::saveFrameChain).  The root evil here is that its not "right" to set the compartment before calling pushDummyFrame and vice versa.  So the obvious fix is to make pushDummyFrame do the compartment change itself, atomically.  This will also remove some hackage that bhackett had to do for TI.
Attachment #551151 - Flags: review?(mrbkap)
(Assignee)

Comment 1

6 years ago
Created attachment 551154 [details] [diff] [review]
patch for TI

This patch is the m-c patch rebased and merged for TI and with the abovementioned hacks removed.

Brian, so I was able to find a single test case (testStackIter.js) that caused the problems (I was just running the tests incorrectly at first).  The attached patch fixes them.
Attachment #551154 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #551154 - Flags: review? → review?(bhackett1024)
(Assignee)

Comment 2

6 years ago
Oops, forgot to qref: pretend there is no "cx->compartment = dest" in saveFrameChain (as its strictly redundant given the cx->resetCompartment).
Attachment #551154 - Flags: review?(bhackett1024) → review+
Comment on attachment 551151 [details] [diff] [review]
patch for m-c

This is pretty awesome.

>diff --git a/js/src/vm/Stack.h b/js/src/vm/Stack.h
>+    /*
>+     * ensureSpace distinguishes between a destination compartment (which is
>+     * used to guard access to the trusted buffer space) and the cx's current
>+     * compartment (which is used to report errors). These are usually the same
>+     * ensureSpace defaults to CX_COMPARTMENT which means "use cx->compartment".
>+     */
>+    static const size_t CX_COMPARTMENT = 1;
>+
>     inline bool ensureSpace(JSContext *cx, MaybeReportError report,
>-                            Value *from, ptrdiff_t nvals) const;
>+                            Value *from, ptrdiff_t nvals,
>+                            JSCompartment *dest = (JSCompartment *)CX_COMPARTMENT) const;

Instead of this crazy CX_COMPARTMENT stuff, why not overload ensureSpace with an ensureSpace that doesn't take the |dest| parameter at all and just calls ensureSpace with cx->compartment as the last parameter?

Looks awesome otherwise. r=me.
Attachment #551151 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 4

6 years ago
(In reply to Blake Kaplan (:mrbkap) from comment #3)
> Instead of this crazy CX_COMPARTMENT stuff, why not overload ensureSpace
> with an ensureSpace that doesn't take the |dest| parameter at all

Hah, of course, thanks!
(Assignee)

Comment 5

6 years ago
Oh wait, I did try this and it isn't so easy: the issue isn't just the default value but that we need to propagate this "use the cx's compartment" choice from ensureOnTop to ensureSpace (which is hard to do it with overloads without duplicating code).
(Assignee)

Comment 6

6 years ago
yeah, mrbkap was right, no CX_COMPARTMENT
http://hg.mozilla.org/integration/mozilla-inbound/rev/0bd518ded931
Whiteboard: [inbound]
(Assignee)

Comment 7

6 years ago
http://hg.mozilla.org/projects/jaegermonkey/rev/e0b67d8cc908
Merged in m-c:
http://hg.mozilla.org/mozilla-central/rev/0bd518ded931
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Version: unspecified → Trunk
(Assignee)

Comment 9

6 years ago
Backing out to test a significant Windows-only (NT5.1 and NT6.1) Dromaeo(SunSpider) regression:
http://hg.mozilla.org/integration/mozilla-inbound/rev/748a4c754183
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

6 years ago
Confirmed, the patch caused the regression.
http://hg.mozilla.org/mozilla-central/rev/748a4c754183
(Assignee)

Comment 12

6 years ago
I can repro the perf regression just by passing the dest parameter (but otherwise keeping everything unmodified).  Also interesting is that ONLY Dromaeo(SunSpider) was affected, not other Dromaeo tests, not SunSpider, not any other Talos measurement.  Perhaps this breaks some critical PGO optimization?  I'm going to see if I can repro the regression on a non-PGO local opt build and play with it more there.
PGO is currently disabled for JS, no?
(Assignee)

Comment 14

6 years ago
(In reply to Ryan VanderMeulen from comment #13)
> PGO is currently disabled for JS, no?

Beats me.

So I tested locally (simple optimized non-PGO build) and I see no speed difference.  Le sigh.  I'll start experimenting with try.
(Assignee)

Comment 15

6 years ago
Heh, if nothing else, this can land when TI lands since TI did not experience any dip:
http://graphs-new.mozilla.org/graph.html#tests=[[75,28,12],[75,1,12]]&sel=none&displayrange=30&datatype=running
(Assignee)

Comment 16

6 years ago
Or, more accurately, since the patch hasn't been backed out of TI, it will just landed with the merge.
(Assignee)

Comment 17

6 years ago
Hah!  As if I couldn't flip-flop any more on the CX_COMPARTMENT issue, using CX_COMPARTMENT does indeed fix the talos regression on try!

The reasoning in the original patch, which was later decided to be premature optimization, is that the 'dest' parameter to the (inlined) ensureSpace is only needed for the out-of-line slow path so it should be a simple constant, not a load (cx->compartment).  Now, one would hope that a smart compiler would notice that the load was only needed on the branch so it would push the load down so it only happened on the slow path.  But, if you are the compiler, maybe this is risky since it is beneficial for the load to be early (so it can have more time to complete before needed).  (Maybe with profiling (which RyanVM ensures me is not happening for JS) the compiler would be able to make this judgment.)  A constant, OTOH, is trivially constant-propagated down to the call-site.
(Assignee)

Comment 18

6 years ago
Still, 8% on Dromaeo(SunSpider) is an extraordinarily huge hit for such a small optimization!  Perhaps someone who knows how to profile on Windows can see what we are doing in Dromaeo(SunSpider) there that puts so much pressure on VM stack functions which, in theory, shouldn't be so hot.  Maybe bz cares about Dromaeo....... :)
(Assignee)

Comment 19

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/e4b0baf5db72
http://hg.mozilla.org/projects/jaegermonkey/rev/0534f46a7091
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/e4b0baf5db72
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: mozilla8 → mozilla9
> Maybe bz cares about Dromaeo....

It's interesting.  The JS team has consistently been not caring about Dromaeo for some reason.... even though a bunch of stuff it measures is in fact JS performance relevant to actual websites (and I'm not talking just the pure-JS tests, which are _not_ relevant to actual websites).

I do care, I guess, since no one else is willing to, but I'm also spread very very thin right now, and have not in fact done any Windows profiling yet.  :(
You need to log in before you can comment on or make changes to this bug.