Closed
Bug 1118604
Opened 9 years ago
Closed 9 years ago
Move activation stack to JSRuntime
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: shu, Assigned: lth)
References
Details
Attachments
(1 file, 2 obsolete files)
78.66 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Without PJS, there's no reason for the activation stack to be allocated per thread.
Assignee | ||
Comment 1•9 years ago
|
||
I assume this means moving the activation-related members and methods from PerThreadData to JSRuntime.
Reporter | ||
Comment 2•9 years ago
|
||
Yes, that was my intention. It's not clear it's useful to have PTD around still, but it's fairly well entrenched in parts of the code and I don't think it hurts to keep it around.
Comment 3•9 years ago
|
||
(In reply to Shu-yu Guo [:shu] from comment #2) > It's not clear it's useful to have PTD around > still, but it's fairly well entrenched in parts of the code and I don't > think it hurts to keep it around. The helper threads can also have a PTD. It seems we can also move these fields to JSRuntime: jitTop, jitJSContext, jitStackLimit_, regexpStack, traceLogger.
Comment 4•9 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #3) > jitTop, jitJSContext, jitStackLimit_ Hm, it might also be nice to have these in JitRuntime instead of JSRuntime, but that's not very important and can wait.
Comment 5•9 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #3) > traceLogger Or maybe this one needs to be in PerThreadData for off-thread compilation. Sorry for the bug spam.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → lhansen
Assignee | ||
Comment 6•9 years ago
|
||
This is probably roughly right -- it passes all --tbpl test so far -- but I'm still running tests and fixing misc problems. I know this patch does not compile yet with the ARM and MIPS simulators, for example.
Assignee | ||
Comment 7•9 years ago
|
||
This is probably clean. Testing now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f3d97d6c5f0
Attachment #8553032 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
After this patch lands there will still be some types and methods related to activation iteration that take a PerThreadData* instead of a JSRuntime*. We should investigate whether that's as it needs to be or if more cleanup is possible. I suspect the latter.
Assignee | ||
Comment 9•9 years ago
|
||
Large patch but most of this is mechanical. Full try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50d40016139b
Attachment #8553699 -
Attachment is obsolete: true
Attachment #8554526 -
Flags: review?(jdemooij)
Comment 10•9 years ago
|
||
Comment on attachment 8554526 [details] [diff] [review] Move activation fields Review of attachment 8554526 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: js/src/asmjs/AsmJSValidate.cpp @@ +8509,5 @@ > // act.prevJitTop_ = cx->mainThread().jitTop; > // act.prevJitJSContext_ = cx->mainThread().jitJSContext; > // cx->mainThread().jitJSContext = cx; > // act.prevJitActivation_ = cx->mainThread().jitActivation; > // cx->mainThread().jitActivation = act; Nit: please update these lines as well. ::: js/src/jit/arm/Simulator-arm.cpp @@ +4438,5 @@ > > +void > +js::PerThreadData::setSimulator(js::jit::Simulator *sim) > +{ > + runtime_->setSimulator(sim); We can now simplify the simulator(s) by merging Simulator and SimulatorRuntime. I think we can just move everything in SimulatorRuntime to Simulator. Maybe we can then rename CreateSimulatorRuntime to CreateSimulator, that way we can assume we always have a non-null simulator in Simulator::Current(). We can also remove AutoLockSimulatorRuntime, might be nice for performance. Follow-up bug :) ::: js/src/vm/Stack.cpp @@ +1693,5 @@ > { > settle(); > } > > ActivationIterator::ActivationIterator(PerThreadData *perThreadData) Nit: is this constructor still necessary?
Attachment #8554526 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #10) > Comment on attachment 8554526 [details] [diff] [review] > Move activation fields > > We can now simplify the simulator(s) by merging Simulator and SimulatorRuntime. > ... > We can also remove AutoLockSimulatorRuntime, might be nice for performance. > > Follow-up bug :) Thank you for that clarification :) I will file it. > ::: js/src/vm/Stack.cpp > @@ +1693,5 @@ > > { > > settle(); > > } > > > > ActivationIterator::ActivationIterator(PerThreadData *perThreadData) > > Nit: is this constructor still necessary? I don't know. It is still being referenced from the GC, but whether it is ultimately necessary to have it depends on some details of the GC that I haven't investigated yet. (I could have sworn I had filed a bug about that but I have not, so I will now. Thanks.)
Assignee | ||
Comment 12•9 years ago
|
||
Pushed with updates to AsmJSValidate.cpp: https://hg.mozilla.org/integration/mozilla-inbound/rev/d590c1f472b2
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d590c1f472b2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•