Closed Bug 665404 Opened 9 years ago Closed 9 years ago

Create JaegerCompartments lazily

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: [MemShrink:P2][fixed-in-tracemonkey])

Attachments

(1 file, 1 obsolete file)

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.
No longer depends on: 661068
Attached patch patch (obsolete) — Splinter Review
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)
Attached patch patch, v2Splinter Review
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 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+
> 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.)
http://hg.mozilla.org/tracemonkey/rev/3bd218337175
Whiteboard: fixed-in-tracemonkey
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.