1233818 - Make implicit interrupt checks work with W^X JIT code

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

We currently disable implicit interrupt checks when W^X JIT code is enabled. We need to fix this if we want to enable non-writable JIT code by default because it affects performance.

The plan of attack is as follows:

(1) Add an Atomic flag to JSRuntime and set it whenever we're in an AutoWritableJitCode scope. In the signal handler, if that flag is set, do nothing, to avoid racing with code currently executing.

(2) Move ExecutableAllocator and the backedge list from JitRuntime to JitZone.

(3) In the signal handler, if the current activation is a JitActivation, get its JitZone (via activation->compartment->zone->jitZone) and reprotect pools that contain Ion code (pool->m_ionCodeBytes > 0) before and after patching backedges.

Making the executable allocator per-Zone is not strictly necessary, but it's nice for other reasons (cache locality, fragmentation) and should reduce the number of pages we have to reprotect in the signal handler.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Explicit interrupt checks for GC

Backedge patching will get slower with W^X JIT code, that might be a problem for the interrupts triggered by the GC, because they can happen fairly frequently.

This patch uses explicit interrupts in Ion for loops that have post barriers or might trigger GC, so the GC interrupts don't need to do backedge patching. What's nice about this is that it also reduces the length of the patchable backedge list.

I tested the benchmarks where implicit interrupts help the most (Kraken ai-astar, Octane-crypto, empty loop microbenchmarks), and we still use implicit interrupts there.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Remove ExecutableAllocator destroyCallback

It's not used.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Out-of-line ExecutableAllocator methods

ExecutableAllocator.h has some huge inline methods, let's just move them to the cpp file as none of this is super hot.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Make implicit interrupts with work --non-writable-jitcode

This patch uses a separate exec-alloc for Ion code with patchable backedges. When patching backedges, we makes this code writable before patching, and executable again when we're done.

Also some minor changes to make sure we don't do this in the signal handler while we're in an AutoWritableJitCode scope or messing with the exec-allocator.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> Making the executable allocator per-Zone is not strictly necessary, but it's
> nice for other reasons (cache locality, fragmentation) and should reduce the
> number of pages we have to reprotect in the signal handler.

I decided not to do this. We discard JIT code pretty aggressively, so fragmentation is not a big concern at the moment. A separate per-runtime allocator for just the Ion code with patchable backedges seemed simpler and safer.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8700598 [details] [diff] [review]
Part 2 - Remove ExecutableAllocator destroyCallback

Review of attachment 8700598 [details] [diff] [review]:
-----------------------------------------------------------------

This apparently got added by Bug 645887, and has not been maintained beyond JM ICs, from what I understand.
Also, it seems to me that this is probably the wrong location for such instrumentation now.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8700600 [details] [diff] [review]
Part 3 - Out-of-line ExecutableAllocator methods

Review of attachment 8700600 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/ExecutableAllocator.cpp
@@ +87,5 @@
> +void
> +ExecutablePool::addRef()
> +{
> +    MOZ_ASSERT(m_refCount);
> +    MOZ_ASSERT(m_refCount < UINT_MAX);

nit: Move the comment which claims that we cannot rollover into this function.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccab6d1ac5dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/44a9d27a671b

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 8700594 [details] [diff] [review]
Part 1 - Explicit interrupt checks for GC

Review of attachment 8700594 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, this is pretty unfortunate (some short loops will still be affected by this) but it does seem like the best option.

Comment 10

[Brian Hackett [Laid off!]]

Comment on attachment 8700601 [details] [diff] [review]
Part 4 - Make implicit interrupts with work --non-writable-jitcode

Review of attachment 8700601 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/Ion.cpp
@@ +375,5 @@
>          else
>              PatchBackedge(patchableBackedge->backedge, patchableBackedge->interruptCheck, target);
>      }
> +
> +    ionExecAlloc_.makeAllExecutable();

I'm not sure this function still behaves correctly.

With writable jitcode calls to patchIonBackedges effectively behave in an idempotent fashion.  Suppose the main thread is interrupted by the watchdog thread and calls patchIonBackedges(BackedgeInterruptCheck) (or if it calls this on its own; this isn't asserted anywhere but doesn't seem currently possible).  If it is in the middle of patching the backedges and is interrupted again, there is nothing to prevent the inner interrupt handler from repatching the backedges (since !preventBackedgePatching_) but before this change the outer interrupt handler will finish fine and leave things in the correct state.

If the same thing happens with this change, then after the inner interrupt handler finishes the code will be marked as executable, and when the outer interrupt resumes it will crash trying to patch the backedges.

If this understanding is correct, it should work if there is an AutoPreventBackedgePatching in this function which surrounds the makeAllWritable/Executable calls.

::: js/src/jit/JitCompartment.h
@@ +82,5 @@
>  class JitRuntime
>  {
>      friend class JitCompartment;
>  
>      // Executable allocator for all code except asm.js code.

This comment is wrong now.

@@ +86,5 @@
>      // Executable allocator for all code except asm.js code.
>      ExecutableAllocator execAlloc_;
>  
> +    // Executable allocator for Ion scripts with patchable backedges.
> +    ExecutableAllocator ionExecAlloc_;

backedgeExecAlloc_?

@@ +207,5 @@
>  
>      ExecutableAllocator& execAlloc() {
>          return execAlloc_;
>      }
> +    ExecutableAllocator& ionExecAlloc() {

backedgeExecAlloc()?

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #10)
> If this understanding is correct, it should work if there is an
> AutoPreventBackedgePatching in this function which surrounds the
> makeAllWritable/Executable calls.

That's a great find, thanks!

I discussed the signal handler stuff with Luke, and this signal handler is not reentrant, but on Windows we use a different mechanism (SuspendThread) and it's possible (in theory) to have 2 (watchdog) threads race here.

I'll post a patch to make sure InterruptRunningJitCode and the signal handler stuff is atomic, and then patchIonBackedges can assert we're handling an interrupt.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Rename rt->handlingSignal

Renames handlingSignal to handlingSegFault as suggested.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Make InterruptRunningJitCode non-reentrant

Adds an Atomic<bool> to JSRuntime, to make sure only one thread is active in InterruptRunningJitCode.

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Fix AutoPreventBackedgePatching

This fixes the asm.js assert I got (no JitRuntime when executing asm.js), and also asserts AutoPreventBackedgePatching is only used on the main thread.

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 8701180 [details] [diff] [review]
Part 6 - Make InterruptRunningJitCode non-reentrant

Review of attachment 8701180 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Runtime.h
@@ +850,5 @@
> +    // InterruptRunningJitCode.
> +    mozilla::Atomic<bool> handlingJitInterrupt_;
> +
> +  public:
> +    bool startJitInterrupt() {

How about (start,finish)HandlingJitInterrupt?

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 8701182 [details] [diff] [review]
Part 7 - Fix AutoPreventBackedgePatching

Review of attachment 8701182 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/JitCompartment.h
@@ +220,5 @@
>          bool prev_;
>        public:
> +        // This two-arg constructor is provided for JSRuntime::createJitRuntime,
> +        // where we have a JitRuntime but didn't set rt->jitRuntime_ yet.
> +        AutoPreventBackedgePatching(JSRuntime* rt, JitRuntime* jrt)

An alternative to this is to have JitRuntime store its JSRuntime* ctor arg in a field so that you could MOZ_ASSERT(CurrentThreadCanAccessRuntime(jrt->runtime())) and then AutoPreventBackedgePatching() could take only a JitRuntime.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63cfd96c94fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/971decb70384
https://hg.mozilla.org/integration/mozilla-inbound/rev/2267d84b2a9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/011ba20fcace
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed06bc78715d

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #15)
> > +    bool startJitInterrupt() {
> 
> How about (start,finish)HandlingJitInterrupt?

Ah yes that's more consistent, done.

(In reply to Luke Wagner [:luke] from comment #16)
> An alternative to this is to have JitRuntime store its JSRuntime* ctor arg
> in a field so that you could
> MOZ_ASSERT(CurrentThreadCanAccessRuntime(jrt->runtime())) and then
> AutoPreventBackedgePatching() could take only a JitRuntime.

I considered this and we already discussed this, but for posterity: some callers can pass in a nullptr rt->jitRuntime (MutateCode in Odin for instance), and in that case we wouldn't have a JSRuntime to assert we're on the main thread. We could use a TLS thing maybe, but I think it doesn't matter too much.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ccab6d1ac5dc
https://hg.mozilla.org/mozilla-central/rev/44a9d27a671b

Comment 20

[Jan de Mooij [:jandem]]

AWFY confirms that with these patches, W^X is < 1% slower on Kraken. On Sunspider and Octane, the gap is a little bigger but still pretty small.

On Octane the main problem is Octane-TypeScript still (it has some worst-case behavior), but there's more we can optimize there.

So on Mac we're in pretty good shape, after I land some more optimizations I'll test Windows and Linux.

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Don't patch if the backedge state is the same

jit::InterruptCheck (can be called hundreds of times during a single Octane test) currently patches backedges to go to the loop header. This patching is actually only needed if we triggered an urgent interrupt and made them jump to the interrupt check, which is very rare and shouldn't be happening on the benchmarks at all.

This patch adds a backedgeTarget_ value to JitRuntime so JitRuntime::patchIonBackedges can just return if target == backedgeTarget_.

Comment 22

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Don't patch if the backedge state is the same

Woops, try caught a bug in the previous version; see comment for an explanation.

Comment 23

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Don't patch if the backedge state is the same

This patch is much nicer actually. It also fixes copyPatchableBackedges to match the state of the other backedges in the runtime. The hasPendingInterrupt check we did there is no longer really correct.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63cfd96c94fc
https://hg.mozilla.org/mozilla-central/rev/971decb70384
https://hg.mozilla.org/mozilla-central/rev/2267d84b2a9c
https://hg.mozilla.org/mozilla-central/rev/011ba20fcace
https://hg.mozilla.org/mozilla-central/rev/ed06bc78715d

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/426e2e8f1ff6

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/426e2e8f1ff6