Make implicit interrupt checks work with W^X JIT code

RESOLVED FIXED in Firefox 46

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(8 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8700594 [details] [diff] [review]
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.
Attachment #8700594 - Flags: review?(bhackett1024)
(Assignee)

Comment 2

a year ago
Created attachment 8700598 [details] [diff] [review]
Part 2 - Remove ExecutableAllocator destroyCallback

It's not used.
Attachment #8700598 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 3

a year ago
Created attachment 8700600 [details] [diff] [review]
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.
Attachment #8700600 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 4

a year ago
Created attachment 8700601 [details] [diff] [review]
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.
Attachment #8700601 - Flags: review?(bhackett1024)
(Assignee)

Comment 5

a year ago
(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 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.
Attachment #8700598 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8700600 - Flags: review?(nicolas.b.pierron) → review+

Comment 8

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccab6d1ac5dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/44a9d27a671b
(Assignee)

Updated

a year ago
Keywords: leave-open
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.
Attachment #8700594 - Flags: review?(bhackett1024) → review+
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()?
Attachment #8700601 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 11

a year ago
(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.
(Assignee)

Comment 12

a year ago
Created attachment 8701179 [details] [diff] [review]
Part 5 - Rename rt->handlingSignal

Renames handlingSignal to handlingSegFault as suggested.
Attachment #8701179 - Flags: review?(luke)
(Assignee)

Comment 13

a year ago
Created attachment 8701180 [details] [diff] [review]
Part 6 - Make InterruptRunningJitCode non-reentrant

Adds an Atomic<bool> to JSRuntime, to make sure only one thread is active in InterruptRunningJitCode.
Attachment #8701180 - Flags: review?(luke)
(Assignee)

Comment 14

a year ago
Created attachment 8701182 [details] [diff] [review]
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.
Attachment #8701182 - Flags: review?(luke)

Updated

a year ago
Attachment #8701179 - Flags: review?(luke) → review+
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?
Attachment #8701180 - Flags: review?(luke) → review+
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.
Attachment #8701182 - Flags: review?(luke) → review+

Comment 17

a year ago
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
(Assignee)

Comment 18

a year ago
(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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ccab6d1ac5dc
https://hg.mozilla.org/mozilla-central/rev/44a9d27a671b
(Assignee)

Comment 20

a year ago
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.
(Assignee)

Comment 21

a year ago
Created attachment 8701815 [details] [diff] [review]
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_.
Attachment #8701815 - Flags: review?(luke)
(Assignee)

Comment 22

a year ago
Created attachment 8701852 [details] [diff] [review]
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.
Attachment #8701815 - Attachment is obsolete: true
Attachment #8701815 - Flags: review?(luke)
Attachment #8701852 - Flags: review?(luke)
(Assignee)

Comment 23

a year ago
Created attachment 8701886 [details] [diff] [review]
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.
Attachment #8701852 - Attachment is obsolete: true
Attachment #8701852 - Flags: review?(luke)
Attachment #8701886 - Flags: review?(luke)

Comment 24

a year ago
bugherder
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

Updated

a year ago
Attachment #8701886 - Flags: review?(luke) → review+

Comment 25

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/426e2e8f1ff6
(Assignee)

Updated

a year ago
Keywords: leave-open

Comment 26

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/426e2e8f1ff6
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.