621032 - Move JSThreadData::(mathCache, iterationCounter) into the compartment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

In the compartment world JSThreadData::cachedNativeIterators, JSThreadData::mathCache should live in the compartment, not thread.

Comment 1

[Igor Bukanov]

cachedNativeIterators should live in the compartment if we move the property tree there and allocate shapes from the GC heap. Otherwise without extra compartment checks on cache query a code can assume a hit when it see the cache entry corresponding to a different compartment.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This should block, since it turns out to be a prerequisite for bug 584860 (see comment 23).

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: mathcache patch

This moves the MathCache to the compartment.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: iterationCounter patch

And this moves iterationCounter to the TraceMonitor. It seemed to fit better there to me.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: updated patch

Oops.

Comment 6

[Luke Wagner [:luke]]

Why do you want to move the math cache into compartments?  There are potentially many many more compartments than JSThreadData's and the math cache is sorta big.

Comment 7

[Igor Bukanov]

Comment on attachment 499431 [details] [diff] [review]
mathcache patch

>diff --git a/js/src/jscompartment.cpp b/js/src/jscompartment.cpp
> JSCompartment::JSCompartment(JSRuntime *rt)
>   : rt(rt), principals(NULL), data(NULL), marked(false), active(false), debugMode(rt->debugMode),
>-    anynameObject(NULL), functionNamespaceObject(NULL)
>+    anynameObject(NULL), functionNamespaceObject(NULL), mathCache(NULL)

Nit: as we are getting more fields change this into one initialization per line like in 
a(...),
b(...)
...

> {
>     JS_INIT_CLIST(&scripts);
> 
>     memset(scriptsToGC, 0, sizeof(scriptsToGC));
> }

Pre-existing nit: replace nenset with PodArrayZero(scriptsToGC)

>diff --git a/js/src/jscompartment.h b/js/src/jscompartment.h
>--- a/js/src/jscompartment.h
>+++ b/js/src/jscompartment.h
>@@ -278,20 +278,31 @@ struct JS_FRIEND_API(JSCompartment) {
>     bool wrap(JSContext *cx, js::PropertyDescriptor *desc);
>     bool wrap(JSContext *cx, js::AutoIdVector &props);
>     bool wrapException(JSContext *cx);
> 
>     void sweep(JSContext *cx, uint32 releaseInterval);
>     void purge(JSContext *cx);
>     void finishArenaLists();
>     bool arenaListsAreEmpty();
>+
>+  private:
>+    js::MathCache                *mathCache;
>+
>+    js::MathCache *allocMathCache(JSContext *cx);
>+
>+public:
>+    js::MathCache *getMathCache(JSContext *cx) {
>+        return mathCache ? mathCache : allocMathCache(cx);
>+    }
> };


Sidenote: We need to decide about the style here: i.e. either put all the fields into the beginning or and of the class or mix them with related methods. I will post about it.

> 
> #define JS_TRACE_MONITOR(cx)    (cx->compartment->traceMonitor)
> #define JS_SCRIPTS_TO_GC(cx)    (cx->compartment->scriptsToGC)
>+#define JS_MATH_CACHE(cx)       (cx->compartment->getMathCache(cx))

Nit: avoid new macros, add an inline helper like GetMathCache(cx). 

r+ with this fixed.

Comment 8

[Igor Bukanov]

Comment on attachment 499436 [details] [diff] [review]
updated patch

>From: Bill McCloskey <[e-mail]>
>try: -p linux,linux64,win32
>
>diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h
>--- a/js/src/jscntxt.h
>+++ b/js/src/jscntxt.h
>@@ -955,19 +955,16 @@ struct JSThreadData {
>      * do not interleave js_GetSrcNote calls.
>      */
>     JSGSNCache          gsnCache;
> 
>     /* Property cache for faster call/get/set invocation. */
>     js::PropertyCache   propertyCache;
> 
> #ifdef JS_TRACER
>-    /* Counts the number of iterations run by a trace. */
>-    unsigned            iterationCounter;
>-
>     /* Maximum size of the tracer's code cache before we start flushing. */
>     uint32              maxCodeCacheBytes;
> #endif
> 
>     /* State used by dtoa.c. */
>     DtoaState           *dtoaState;
> 
>     /*
>diff --git a/js/src/jscompartment.h b/js/src/jscompartment.h
>--- a/js/src/jscompartment.h
>+++ b/js/src/jscompartment.h
>@@ -87,16 +87,19 @@ struct TraceMonitor {
>      *
>      * !tracecx && !recorder: not on trace
>      * !tracecx && recorder: recording
>      * tracecx && !recorder: executing a trace
>      * tracecx && recorder: executing inner loop, recording outer loop
>      */
>     JSContext               *tracecx;
> 
>+    /* Counts the number of iterations run by the currently executing trace. */
>+    unsigned                iterationCounter;
>+
>     /*
>      * Cached storage to use when executing on trace. While we may enter nested
>      * traces, we always reuse the outer trace's storage, so never need more
>      * than of these.
>      */
>     TraceNativeStorage      *storage;
> 
>     /*
>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -2358,17 +2358,17 @@ TraceRecorder::TraceRecorder(JSContext* 
>         /*
>          * Count the number of iterations run by a trace, so that we can blacklist if
>          * the trace runs too few iterations to be worthwhile. Do this only if the methodjit
>          * is on--otherwise we must try to trace as much as possible.
>          */
> #ifdef JS_METHODJIT
>         if (cx->methodJitEnabled) {
>             w.comment("begin-count-loop-iterations");
>-            LIns* counterPtr = w.nameImmpNonGC((void *) &JS_THREAD_DATA(cx)->iterationCounter);
>+            LIns* counterPtr = w.nameImmpNonGC((void *) &traceMonitor->iterationCounter);
>             LIns* counterValue = w.ldiVolatile(counterPtr);
>             LIns* test = w.ltiN(counterValue, LOOP_COUNT_MAX);
>             LIns *branch = w.jfUnoptimizable(test);
>             /* 
>              * stiVolatile() uses ACCSET_STORE_ANY;  If LICM is implemented
>              * (bug 545406) this counter will need its own region.
>              */
>             w.stiVolatile(w.addi(counterValue, w.immi(1)), counterPtr);
>@@ -6519,17 +6519,17 @@ ExecuteTree(JSContext* cx, TreeFragment*
>                       FramePCOffset(cx, cx->fp()),
>            f->execs,
>            f->code());
> 
>     debug_only_stmt(uint32 globalSlots = globalObj->numSlots();)
>     debug_only_stmt(*(uint64*)&tm->storage->global()[globalSlots] = 0xdeadbeefdeadbeefLL;)
> 
>     /* Execute trace. */
>-    JS_THREAD_DATA(cx)->iterationCounter = 0;
>+    tm->iterationCounter = 0;
>     debug_only(int64 t0 = PRMJ_Now();)
> #ifdef MOZ_TRACEVIS
>     VMSideExit* lr = (TraceVisStateObj(cx, S_NATIVE), ExecuteTrace(cx, f, state));
> #else
>     VMSideExit* lr = ExecuteTrace(cx, f, state);
> #endif
>     debug_only(int64 t1 = PRMJ_Now();)
> 
>@@ -6538,17 +6538,17 @@ ExecuteTree(JSContext* cx, TreeFragment*
> 
>     /* Restore interpreter state. */
>     LeaveTree(tm, state, lr);
> 
>     *lrp = state.innermost;
>     bool ok = !(state.builtinStatus & BUILTIN_ERROR);
>     JS_ASSERT_IF(cx->throwing, !ok);
> 
>-    size_t iters = JS_THREAD_DATA(cx)->iterationCounter;
>+    size_t iters = tm->iterationCounter;
> 
>     f->execs++;
>     f->iters += iters;
> 
> #ifdef DEBUG
>     JSStackFrame *fp = cx->fp();
>     const char *prefix = "";
>     if (iters == LOOP_COUNT_MAX)

Comment 9

[Igor Bukanov]

(In reply to comment #8)
> Comment on attachment 499436 [details] [diff] [review]
> updated patch

Sorry for the diff spam - there were no comments there.

Comment 10

[Igor Bukanov]

(In reply to comment #6)
> Why do you want to move the math cache into compartments?

The trace jit embeds the pointer into the math cache. Since the tracejit lives in a compartment and the compartment can move between threads we cannot have MathCache in JSThread unless we want to slow down the cache access via indirect loads like cx->thread->data.mathCache.

> There are
> potentially many many more compartments than JSThreadData's and the math cache
> is sorta big.

Math cache is lazily allocated. So moving it into the compartment bloats the compartment that does not use by 1 word. On the other hand if 2 different compartment do use it, then separated caches may benefit them.

Comment 11

[Igor Bukanov]

I split this bug to do the cachedNativeIterator move in another bug to mark this one as a strict regression for bug 584860.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to comment #10)
> Math cache is lazily allocated. So moving it into the compartment bloats the
> compartment that does not use by 1 word. On the other hand if 2 different
> compartment do use it, then separated caches may benefit them.

Maybe a good solution would be to occasionally flush the math cache? We're going to have to do that with the trace cache anyway, in bug 620833, so we might as well do this here as well.

Comment 13

[Luke Wagner [:luke]]

(In reply to comment #12)
> Maybe a good solution would be to occasionally flush the math cache? We're
> going to have to do that with the trace cache anyway, in bug 620833, so we
> might as well do this here as well.

Agreed.

Really, this is just begging for the thing ("zone" or modified JSThreadData) discussed in bug 587288 comments 8 - 12.  This problem is only going to happen more as we want single-threaded access to data structures but, for memory usage concerns, don't want to stick them in the compartment.

Comment 14

[Luke Wagner [:luke]]

(In reply to comment #13)
> Really, this is just begging for the thing ("zone" or modified JSThreadData)
> discussed in bug 587288 comments 8 - 12.

Filed bug 621205 on this.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

http://hg.mozilla.org/tracemonkey/rev/74c70b60cf8b
http://hg.mozilla.org/tracemonkey/rev/acb3e091eb25

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

http://hg.mozilla.org/mozilla-central/rev/15479f7be9d8
http://hg.mozilla.org/mozilla-central/rev/49ed986cf05b