Closed
Bug 676937
Opened 13 years ago
Closed 13 years ago
Make entering a compartment and pushing a dummy frame an atomic stack operation
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: luke, Assigned: luke)
Details
Attachments
(2 files)
12.53 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
14.51 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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•13 years ago
|
Attachment #551154 -
Flags: review? → review?(bhackett1024)
Assignee | ||
Comment 2•13 years ago
|
||
Oops, forgot to qref: pretend there is no "cx->compartment = dest" in saveFrameChain (as its strictly redundant given the cx->resetCompartment).
Updated•13 years ago
|
Attachment #551154 -
Flags: review?(bhackett1024) → review+
Comment 3•13 years ago
|
||
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•13 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•13 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•13 years ago
|
||
yeah, mrbkap was right, no CX_COMPARTMENT http://hg.mozilla.org/integration/mozilla-inbound/rev/0bd518ded931
Whiteboard: [inbound]
Assignee | ||
Comment 7•13 years ago
|
||
http://hg.mozilla.org/projects/jaegermonkey/rev/e0b67d8cc908
Comment 8•13 years ago
|
||
Merged in m-c: http://hg.mozilla.org/mozilla-central/rev/0bd518ded931
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Version: unspecified → Trunk
Assignee | ||
Comment 9•13 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•13 years ago
|
||
Confirmed, the patch caused the regression.
Assignee | ||
Comment 12•13 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.
Comment 13•13 years ago
|
||
PGO is currently disabled for JS, no?
Assignee | ||
Comment 14•13 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•13 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•13 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•13 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•13 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•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/e4b0baf5db72 http://hg.mozilla.org/projects/jaegermonkey/rev/0534f46a7091
Whiteboard: [inbound]
Comment 20•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/e4b0baf5db72
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: mozilla8 → mozilla9
Comment 21•13 years ago
|
||
> 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.
Description
•