The default bug view has changed. See this FAQ.

Create JaegerCompartments lazily

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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)

Updated

6 years ago
No longer depends on: 661068
(Assignee)

Comment 1

6 years ago
Created attachment 540378 [details] [diff] [review]
patch

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

6 years ago
Created attachment 540395 [details] [diff] [review]
patch, v2

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

6 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

6 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

6 years ago
http://hg.mozilla.org/tracemonkey/rev/3bd218337175
Whiteboard: fixed-in-tracemonkey
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/3bd218337175
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 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.