Last Comment Bug 665404 - Create JaegerCompartments lazily
: Create JaegerCompartments lazily
Status: RESOLVED FIXED
[MemShrink:P2][fixed-in-tracemonkey]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 3 votes (vote)
: ---
Assigned To: Nicholas Nethercote [:njn]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: mslim-fx5+
  Show dependency treegraph
 
Reported: 2011-06-19 19:01 PDT by Nicholas Nethercote [:njn]
Modified: 2011-07-02 18:18 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (8.87 KB, patch)
2011-06-19 19:41 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Splinter Review
patch, v2 (9.70 KB, patch)
2011-06-19 22:22 PDT, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2011-06-19 19:01:04 PDT
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.
Comment 1 Nicholas Nethercote [:njn] 2011-06-19 19:41:31 PDT
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.)
Comment 2 Nicholas Nethercote [:njn] 2011-06-19 22:22:10 PDT
Created attachment 540395 [details] [diff] [review]
patch, v2

The previous patch missed a necessary call to ensureJaegerCompartmentExists().
Comment 3 Luke Wagner [:luke] 2011-06-20 10:36:29 PDT
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.
Comment 4 Nicholas Nethercote [:njn] 2011-06-20 21:00:07 PDT
> 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.)
Comment 5 Nicholas Nethercote [:njn] 2011-06-21 16:56:10 PDT
http://hg.mozilla.org/tracemonkey/rev/3bd218337175
Comment 6 Chris Leary [:cdleary] (not checking bugmail) 2011-06-27 11:39:09 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/3bd218337175

Note You need to log in before you can comment on or make changes to this bug.