584860 - TM: Move TM into compartment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Right now the TraceMonitor is per-thread, which doesn't make much sense in our new compartment world, especially since 1) traces don't cross global objects 2) scripts will be per-compartment.

From there we can revive a JM1 change. JSOP_TRACE can precede a 16-bit integer that keys into a TraceTree vector attached to each script. This will make lookup lightning fast and fragment cache clearing trivial.

Comment 2

[Andreas Gal :gal]

Attachment: patch

parital patch for billm to take over

Comment 3

[Andreas Gal :gal]

I think we can split this bug. I need the trace cache to be associated with the compartment for per-compartment GC. The vmfragment stuff can be killed later.

Comment 4

[Andreas Gal :gal]

In jsd we recover the current thread when we are given a runtime and disable something in the trace monitor there. This is necessary because the API takes a runtime pointer, instead of a constant. That needs fixing.

Comment 5

[Andreas Gal :gal]

This blocks per-compartment GCs.

Comment 6

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

Attachment: updated patch

This passes trace-tests and jstests. Currently on tryserver.

Comment 7

[Andreas Gal :gal]

maxCodeCacheBytes should probably keep living in the thread since the name as PerThread parameters.

Comment 8

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

Just for the record, the scary part here is the removal of LeaveTraceRT.

Comment 9

[David Mandelin [:dmandelin]]

This bug is currently minused for 2.0, but it blocks bug 562455, which is final+. We need to make a decision and get this consistent again. See also bug 615723 comment 0 for more on the underlying issues.

Comment 11

[Paul Wright]

This bug also blocks bug 605662, enabling compartmental GC (per comment 5), which is a beta 9 blocker.  So, there are inconsistencies there as well.

Comment 12

[David Mandelin [:dmandelin]]

Reactivating as a blocker after discussion with Andreas.

Comment 13

[Andreas Gal :gal]

I don't need this for per-compartment, but we should do it for b9 anyway.

Comment 14

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

Attachment: updated patch

Here's a rebased patch. It's green on the tryserver. I had to move the eval cache to the compartment as well; this seems like the right place for it. I also moved maxCodeCacheBytes to the thread data, as requested.

Comment 15

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

Attachment: updated patch

Oops, I made a small mistake. Here's a fixed version.

Comment 16

[Andreas Gal :gal]

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

This is no longer true. Just remove the comment.

 1/16 of this
+     * size is used as threshold for the regular expression code cache.

Is it ok to no longer do LeaveTraceRT?

We used to have 16MB code cache per thread. We now have 16MB per compartment. That might become a problem. We should file a bug to flush the compartment code cache when the page goes into the BF cache.

Comment 17

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

I talked to dvander about the LeaveTraceRT thing and he seemed confident that it was okay.

http://hg.mozilla.org/tracemonkey/rev/2d7468b6054f

Comment 18

[Igor Bukanov]

(In reply to comment #17)
> I talked to dvander about the LeaveTraceRT thing and he seemed confident that
> it was okay.
> 
> http://hg.mozilla.org/tracemonkey/rev/2d7468b6054f

There are stalled comments that continue to refer to threads, not compartments:

http://hg.mozilla.org/tracemonkey/rev/2d7468b6054f#l6.26 : 

    6.27 + * Trace monitor. Every JSThread (if JS_THREADSAFE) or JSRuntime (if not
    6.28 + * JS_THREADSAFE) has an associated trace monitor that keeps track of loop
    6.29 + * frequencies for all JavaScript code loaded into that runtime.
    6.30 + */
    6.31 +struct TraceMonitor {
    6.32 +    /*
    6.33 +     * The context currently executing JIT-compiled code on this thread, or
    6.34 +     * NULL if none. Among other things, this can in certain cases prevent
    6.35 +     * last-ditch GC and suppress calls to JS_ReportOutOfMemory.

Comment 19

[Igor Bukanov]

More notes on the patch:

TraceMonotor::sweep should be called from JSCompartment::sweep, no via an explicit loop from GC.

DestroyScript should not take an extra compartment argument as script's compartment is available via script->compartment.

We need to do something with maxCodeCacheBytes. Due to thread migration for web workers that value may have no relevance to the current compartment. Perhaps we should have one per-runtime counter? The counter can be lockless with update races tolerated and which can be re-adjusted during the GC.

Comment 20

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

Attachment: small changes

OK, here's a patch for these remaining items. I left maxCodeCacheBytes alone. Right now it's basically a constant: 16MB for normal stuff, 1MB for DOM workers. I suspect that bug 620833 will mostly obsolete this value anyway.

Comment 21

[Igor Bukanov]

Comment on attachment 499374 [details] [diff] [review]
small changes

Thanks!

Comment 22

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

http://hg.mozilla.org/tracemonkey/rev/0be4f01086ea

Comment 23

[Igor Bukanov]

One more issue: the tracer still embeds the pointer to JSThreadData fields directly into the traced code. This is wrong as the web workers compartment can move across threads and even outlive the thread the jit where generated on. In particular, we have the problems with

JSThreadData::interruptFlags, JSThreadData::iterationCounter and JSThreadData::mathCache.

For JSThreadData::interruptFlags the patch should do the same what the method jit does and update JSRuntime::interruptCounter, see mjit::Compiler::interruptCheckHelper()

For mathCache we should just move it to the compartment.

For iterationCounter it seems the same treatment of moving it into the compartment should be used.

Comment 25

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

I happened to notice the function ProhibitFlush in the tracer today. It avoids doing a flush if any context associated with the current thread is running a trace. It seems like this code should maybe be removed, or else it needs to change somehow. I don't understand enough to know, though. Can anyone help?

Comment 26

[Igor Bukanov]

(In reply to comment #25)
> I happened to notice the function ProhibitFlush in the tracer today. It avoids
> doing a flush if any context associated with the current thread is running a
> trace. 

This should be changed to avoid the flush if any trace related to this compartment is running. But it looks like the whole implementation related to JS_FlushCaches and ResetJIT should be changed to flush either the trace compartment (cases like ResetJIT(cx, FR_DEEP_BAIL)) or all compartments in the given runtime (the case of ResetJIT(cx, FR_OOM)).

Comment 27

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

(In reply to comment #26)
> This should be changed to avoid the flush if any trace related to this
> compartment is running.

How would this work? Would we have to worry about contexts other than cx? Is it possible for one context to be associated with a compartment, start running a trace, get suspended while on trace somehow, and then for another context to enter the same compartment?

> But it looks like the whole implementation related to
> JS_FlushCaches and ResetJIT should be changed to flush either the trace
> compartment (cases like ResetJIT(cx, FR_DEEP_BAIL)) or all compartments in the
> given runtime (the case of ResetJIT(cx, FR_OOM)).

Currently, we are flushing just the compartment. In the case of an actual OOM, it probably does make sense to flush the jit for all compartments. However, ResetJIT(cx, FR_OOM) is also called when we hit OverfullJITCache(cx, tm); in this case, we just want to flush the compartment.

So we might want to add a case to ResetJIT so that if tm->outOfMemory() is true, then we should flush all the compartments.

Comment 28

[Igor Bukanov]

(In reply to comment #27)
> How would this work? Would we have to worry about contexts other than cx? Is it
> possible for one context to be associated with a compartment, start running a
> trace, get suspended while on trace somehow, and then for another context to
> enter the same compartment?

I do not know - somebody with more knowledge about traces should comment.

> Currently, we are flushing just the compartment. In the case of an actual OOM,
> it probably does make sense to flush the jit for all compartments.

I think we need 2 bugs: the first is about flushing the current compartment and the second about flushing everything possible on OOM.

Comment 29

[Igor Bukanov]

The landed patch has not removed the vmfragment table - I filed that as bug 621998.

Comment 30

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

http://hg.mozilla.org/mozilla-central/rev/c9682df3daf8

Comment 31

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

http://hg.mozilla.org/mozilla-central/rev/f66eefcf11ff

Comment 32

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)