Last Comment Bug 1233818 - Make implicit interrupt checks work with W^X JIT code
: Make implicit interrupt checks work with W^X JIT code
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine: JIT (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla46
Assigned To: Jan de Mooij [:jandem]
:
: Hannes Verschore [:h4writer]
Mentors:
Depends on:
Blocks: 1215479
  Show dependency treegraph
 
Reported: 2015-12-18 12:23 PST by Jan de Mooij [:jandem]
Modified: 2016-03-14 04:31 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Part 1 - Explicit interrupt checks for GC (13.00 KB, patch)
2015-12-21 06:24 PST, Jan de Mooij [:jandem]
bhackett1024: review+
Details | Diff | Splinter Review
Part 2 - Remove ExecutableAllocator destroyCallback (2.14 KB, patch)
2015-12-21 06:27 PST, Jan de Mooij [:jandem]
nicolas.b.pierron: review+
Details | Diff | Splinter Review
Part 3 - Out-of-line ExecutableAllocator methods (17.63 KB, patch)
2015-12-21 06:29 PST, Jan de Mooij [:jandem]
nicolas.b.pierron: review+
Details | Diff | Splinter Review
Part 4 - Make implicit interrupts with work --non-writable-jitcode (25.63 KB, patch)
2015-12-21 06:34 PST, Jan de Mooij [:jandem]
bhackett1024: review+
Details | Diff | Splinter Review
Part 5 - Rename rt->handlingSignal (6.37 KB, patch)
2015-12-22 12:34 PST, Jan de Mooij [:jandem]
luke: review+
Details | Diff | Splinter Review
Part 6 - Make InterruptRunningJitCode non-reentrant (6.14 KB, patch)
2015-12-22 12:36 PST, Jan de Mooij [:jandem]
luke: review+
Details | Diff | Splinter Review
Part 7 - Fix AutoPreventBackedgePatching (11.33 KB, patch)
2015-12-22 12:37 PST, Jan de Mooij [:jandem]
luke: review+
Details | Diff | Splinter Review
Part 8 - Don't patch if the backedge state is the same (4.70 KB, patch)
2015-12-24 04:35 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Part 8 - Don't patch if the backedge state is the same (5.27 KB, patch)
2015-12-24 08:36 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Part 8 - Don't patch if the backedge state is the same (5.81 KB, patch)
2015-12-24 16:03 PST, Jan de Mooij [:jandem]
luke: review+
Details | Diff | Splinter Review

Description User image Jan de Mooij [:jandem] 2015-12-18 12:23:32 PST
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 User image Jan de Mooij [:jandem] 2015-12-21 06:24:56 PST
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.
Comment 2 User image Jan de Mooij [:jandem] 2015-12-21 06:27:07 PST
Created attachment 8700598 [details] [diff] [review]
Part 2 - Remove ExecutableAllocator destroyCallback

It's not used.
Comment 3 User image Jan de Mooij [:jandem] 2015-12-21 06:29:21 PST
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.
Comment 4 User image Jan de Mooij [:jandem] 2015-12-21 06:34:38 PST
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.
Comment 5 User image Jan de Mooij [:jandem] 2015-12-21 06:50:34 PST
(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 User image Nicolas B. Pierron [:nbp] 2015-12-21 08:30:59 PST
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 User image Nicolas B. Pierron [:nbp] 2015-12-21 08:54:57 PST
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 9 User image Brian Hackett (:bhackett) 2015-12-22 03:23:06 PST
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 User image Brian Hackett (:bhackett) 2015-12-22 04:04:02 PST
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 User image Jan de Mooij [:jandem] 2015-12-22 10:50:43 PST
(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 User image Jan de Mooij [:jandem] 2015-12-22 12:34:13 PST
Created attachment 8701179 [details] [diff] [review]
Part 5 - Rename rt->handlingSignal

Renames handlingSignal to handlingSegFault as suggested.
Comment 13 User image Jan de Mooij [:jandem] 2015-12-22 12:36:21 PST
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.
Comment 14 User image Jan de Mooij [:jandem] 2015-12-22 12:37:48 PST
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.
Comment 15 User image Luke Wagner [:luke] 2015-12-22 13:19:08 PST
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 User image Luke Wagner [:luke] 2015-12-22 13:34:43 PST
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 18 User image Jan de Mooij [:jandem] 2015-12-23 02:49:55 PST
(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 20 User image Jan de Mooij [:jandem] 2015-12-23 04:41:44 PST
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 User image Jan de Mooij [:jandem] 2015-12-24 04:35:56 PST
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_.
Comment 22 User image Jan de Mooij [:jandem] 2015-12-24 08:36:45 PST
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.
Comment 23 User image Jan de Mooij [:jandem] 2015-12-24 16:03:04 PST
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.
Comment 26 User image Carsten Book [:Tomcat] 2015-12-29 03:00:56 PST
https://hg.mozilla.org/mozilla-central/rev/426e2e8f1ff6

Note You need to log in before you can comment on or make changes to this bug.