623050 - TM: ProhibitFlush is incorrect now that TM is in compartment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

When the TraceMonitor moved into the compartment in bug 584860, I didn't change ProhibitFlush so that we don't flush the trace cache of a compartment when some other thread is running code from the same compartment. Andreas says this is necessary.

Comment 1

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

Attachment: patch

This patch moves tracerState and bailExit from JSContext to the TraceMonitor.

Nick, could you look over the code in Writer.{cpp,h}? I added ACCSET_TM for access inside the TraceMonitor. The intention is that you will always be accessing a baked-in address. In this case, I'm accessing bailExit. I'm pretty sure that this does not need to be a volatile access. The only place bailExit is changed outside of tracejit-generated code is in DeepBail, and that's only after we've already done LeaveTree.

Comment 2

[Andreas Gal :gal]

Comment on attachment 501513 [details] [diff] [review]
patch

>+ProhibitFlush(TraceMonitor *tm)
>+{
>+    return !!tm->tracerState; // early out if the given is in native code

comment doesn't make sense (early out? given?)

compartment and thus traceMonitor should be now constant for a given script, right? so we can just embed direct addresses instead of loading them?

Comment 3

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

(In reply to comment #2)
> comment doesn't make sense (early out? given?)

I guess it's supposed to mean "don't flush if we're in native code". It's a stupid comment; I'll take it out.

> compartment and thus traceMonitor should be now constant for a given script,
> right? so we can just embed direct addresses instead of loading them?

The patch embeds the address of bailExit. I didn't see any other loads from tm on trace. What were you thinking of?

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 501513 [details] [diff] [review]
patch

Most important thing:  You need to update accNames[] in jstracer.cpp.  Once
you've done that, do a run with TMFLAGS=afterdce and check that the accset
annotations on the loads and stores look ok.


>+    nj::LIns *stTraceMonitorField(nj::LIns *value, void *dest, const char *destName) const {
>+        return lir->insStore(value, name(lir->insImmP(dest), destName), 0, ACCSET_TM);
>+    }
>+    #define stTraceMonitorField(value, tm, fieldname) \
>+        stTraceMonitorField(value, &(tm)->fieldname, #fieldname)

You could remove the 2nd arg and hardwire the macro to use 'traceMonitor',
like stContextField() does with 'cx_ins'.  Not crucial, though.


>+    nj::LIns *ldpVolatile(nj::LIns *base) const {
>+        return lir->insLoad(nj::LIR_ldp, base, 0, nj::ACCSET_LOAD_ANY, nj::LOAD_VOLATILE);
>+    }
>+
>+    nj::LIns *stpVolatile(nj::LIns *value, nj::LIns *base) const {
>+        return lir->insStore(nj::LIR_stp, value, base, 0, nj::ACCSET_STORE_ANY);
>+    }

These aren't used, take them out.


f=me with these fixed.

Comment 5

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

(In reply to comment #4)
> Most important thing:  You need to update accNames[] in jstracer.cpp.  Once
> you've done that, do a run with TMFLAGS=afterdce and check that the accset
> annotations on the loads and stores look ok.

What do you think of the idea of making accNames public in LInsPrinter so that we can JS_STATIC_ASSERT that it has the right size? I.e.,
  JS_STATIC_ASSERT(JS_ARRAY_LENGTH(nanojit::LInsPrinter::accNames) ==
                   TM_NUM_USED_ACCS + 1);

Comment 6

[Brendan Eich [:brendan]]

Could use a list-generating macro like FUNCTION_KIND_METER_LIST to avoid all duplication and potential loss of array parallelism maintenance hazards.

/be

Comment 7

[Andreas Gal :gal]

Comment on attachment 501513 [details] [diff] [review]
patch

r=me, nice work, as usual

Comment 8

[David Mandelin [:dmandelin]]

Is this ready to land?

Comment 9

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

(In reply to comment #8)
> Is this ready to land?

As soon as the try server is ready to reveal its precious results. It's been "Loading 75%" for about an hour.

Comment 10

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

http://hg.mozilla.org/tracemonkey/rev/5d3b8f835242

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/5d3b8f835242