Last Comment Bug 676937 - Make entering a compartment and pushing a dummy frame an atomic stack operation
: Make entering a compartment and pushing a dummy frame an atomic stack operation
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Luke Wagner [:luke]
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2011-08-05 14:00 PDT by Luke Wagner [:luke]
Modified: 2011-08-23 22:34 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch for m-c (12.53 KB, patch)
2011-08-05 14:00 PDT, Luke Wagner [:luke]
mrbkap: review+
Details | Diff | Splinter Review
patch for TI (14.51 KB, patch)
2011-08-05 14:04 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2011-08-05 14:00:33 PDT
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.
Comment 1 Luke Wagner [:luke] 2011-08-05 14:04:09 PDT
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.
Comment 2 Luke Wagner [:luke] 2011-08-05 14:07:32 PDT
Oops, forgot to qref: pretend there is no "cx->compartment = dest" in saveFrameChain (as its strictly redundant given the cx->resetCompartment).
Comment 3 Blake Kaplan (:mrbkap) 2011-08-09 18:17:49 PDT
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.
Comment 4 Luke Wagner [:luke] 2011-08-09 20:15:11 PDT
(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!
Comment 5 Luke Wagner [:luke] 2011-08-10 10:58:35 PDT
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).
Comment 6 Luke Wagner [:luke] 2011-08-10 15:48:35 PDT
yeah, mrbkap was right, no CX_COMPARTMENT
Comment 8 Mounir Lamouri (:mounir) 2011-08-11 04:21:30 PDT
Merged in m-c:
Comment 9 Luke Wagner [:luke] 2011-08-12 09:54:34 PDT
Backing out to test a significant Windows-only (NT5.1 and NT6.1) Dromaeo(SunSpider) regression:
Comment 10 Luke Wagner [:luke] 2011-08-12 16:21:20 PDT
Confirmed, the patch caused the regression.
Comment 11 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-14 04:54:26 PDT
Comment 12 Luke Wagner [:luke] 2011-08-16 15:59:35 PDT
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.
Comment 13 Ryan VanderMeulen [:RyanVM] 2011-08-16 16:49:22 PDT
PGO is currently disabled for JS, no?
Comment 14 Luke Wagner [:luke] 2011-08-16 18:15:49 PDT
(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.
Comment 15 Luke Wagner [:luke] 2011-08-17 09:28:03 PDT
Heh, if nothing else, this can land when TI lands since TI did not experience any dip:[[75,28,12],[75,1,12]]&sel=none&displayrange=30&datatype=running
Comment 16 Luke Wagner [:luke] 2011-08-17 09:35:35 PDT
Or, more accurately, since the patch hasn't been backed out of TI, it will just landed with the merge.
Comment 17 Luke Wagner [:luke] 2011-08-18 09:42:28 PDT
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.
Comment 18 Luke Wagner [:luke] 2011-08-18 09:44:14 PDT
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....... :)
Comment 20 Ed Morley [:emorley] 2011-08-21 11:44:17 PDT
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2011-08-23 22:34:22 PDT
> 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.  :(

Note You need to log in before you can comment on or make changes to this bug.