Closed
Bug 665404
Opened 13 years ago
Closed 13 years ago
Create JaegerCompartments lazily
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: [MemShrink:P2][fixed-in-tracemonkey])
Attachments
(1 file, 1 obsolete file)
9.70 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
Spin-off from bug 661068 -- create JaegerCompartments lazily, because you often have small ones that don't need any method JITting, and currently when we create the TrampolineCompiler we allocate 64KB of memory.
Assignee | ||
Comment 1•13 years ago
|
||
Pretty straightforward. Should getJaegerCompartment() be called jaegerCompartment() instead? This would require decorating .jaegerCompartment. (A similar question applies to getTraceMonitor() in bug 661068.)
Attachment #540378 -
Flags: review?(luke)
Assignee | ||
Comment 2•13 years ago
|
||
The previous patch missed a necessary call to ensureJaegerCompartmentExists().
Attachment #540378 -
Attachment is obsolete: true
Attachment #540378 -
Flags: review?(luke)
Attachment #540395 -
Flags: review?(luke)
Comment 3•13 years ago
|
||
Comment on attachment 540395 [details] [diff] [review] patch, v2 Review of attachment 540395 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jscompartment.h @@ +428,5 @@ > + > + js::mjit::JaegerCompartment *getJaegerCompartment() const { > + JS_ASSERT(jaegerCompartment); > + return jaegerCompartment; > + } We have been using the style that "getX()" is a fallible operation and "x()" is the asserted-infallible field access. The absence of a 'cx' parameter does also indicate infallibility, but it still look weird to see getX() used infallibly in expressions. (I know we have some outliers like getParent/getProto; they should be changed/removed with objshrink.) To resolve the name conflict, you can append a _ if you'd like. ::: js/src/methodjit/MethodJIT-inl.h @@ +73,5 @@ > } > + if (!cx->compartment->hasJaegerCompartment()) { > + bool ok = cx->compartment->ensureJaegerCompartmentExists(cx); > + if (!ok) > + return Compile_Abort; Any reason you can't push this down into TryCompile() and factor out these two instances? Ideally there would we one pinch point where this happens and mjit::Compiler::performCompilation looks reasonable.
Attachment #540395 -
Flags: review?(luke) → review+
Assignee | ||
Comment 4•13 years ago
|
||
> Any reason you can't push this down into TryCompile() and factor out these > two instances? Ideally there would we one pinch point where this happens > and mjit::Compiler::performCompilation looks reasonable. Good idea, I'll do that. I'll also rename getJaegerCompartment() as jaegerCompartment(). (I did likewise with getTraceMonitor() in bug 661068.)
Assignee | ||
Comment 5•13 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/3bd218337175
Whiteboard: fixed-in-tracemonkey
Comment 6•13 years ago
|
||
cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/3bd218337175
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Whiteboard: fixed-in-tracemonkey → [MemShrink:P2][fixed-in-tracemonkey]
You need to log in
before you can comment on or make changes to this bug.
Description
•