The default bug view has changed. See this FAQ.

IonMonkey: Previous constraints can invalidate recompiled functions.

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

unspecified
mozilla17
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
This bug appear on v8-raytrace.js without the fix for Bug 766805 (should be masked when applied).  The bug manifest it-self with the following schema:

F calls G, G calls H.
F is compiled and inline H through G.
F got invalidated and the counter of G got reset.
F is recompiled without inlining neither G nor H
G is compiled.
H is compiled.
F got invalidated by some modification on H. <-- *error*

in v8-raytrace.js, the invalidations are caused by Bug 766805 and F, G and H are respectively located at lines 681, 427 and 269.

It seems that the freeze added during one compilation are not removed when the result of the compilation are invalidated.  Which means that the second compilation can be invalidated by freeze added during the first compilation.
(Assignee)

Comment 1

5 years ago
I suggest to replace the RecompileInfo strucure which is duplicated in many TypeConstraint classes by a CompileOutput pointer.  The CompileOutput structure should be allocated at the beginning of the Compilation and used when registering constraints.  At the end of the compilation, an identifier — to identify the compilation result — is set.  When we invalidate, we reset the identifier to an unambiguous flag. This flag mark that the compilation output is invalidated, and thus there is need to add a pending recompilation.

In cases where we cannot allocate a CompileOutput (1), we can still use the RecompileInfo structure even if this may cause unwanted invalidations.

What do you think?

(1) TypeCompartment::addPendingRecompile has 2 interfaces, one with a RecompileInfo and one with pc & script.  If it is hard to convert then to all use the CompilationOutput, we can keep the RecompileInfo and convert the CompilationOutput to a RecompileInfo when the jit-code is still valid.
This sounds good, I'm not quite following how the allocation and lifetime for the CompileOutput pointer works, but here is a similar approach that will work fine:

- Keep a vector of RecompileInfo on the compartment, where an entry is added every time a script is compiled by IM or JM.  Each entry in this vector has a bit indicating whether the compiled code has been invalidated.

- Instead of adding RecompileInfo directly to freeze constraints, add an index into the compartment's compilation vector.  Then only addPendingRecompile when a freeze constraint is triggered on a RecompileInfo which has not already been marked as invalidated.

- Purge the vector of RecompileInfo data whenever analysis data and jitcode are purged, i.e. on most GCs.

The pc/script form of addPendingRecompile should be left alone, this is for triggering recompilations in cases where there is no explicitly associated freeze constraint, e.g. when a type barrier suddenly appears at some random opcode.
(Assignee)

Comment 3

5 years ago
(In reply to Brian Hackett (:bhackett) from comment #2)
> This sounds good, I'm not quite following how the allocation and lifetime
> for the CompileOutput pointer works, but here is a similar approach that
> will work fine:
> 
> - Keep a vector of RecompileInfo on the compartment, where an entry is added
> every time a script is compiled by IM or JM.  Each entry in this vector has
> a bit indicating whether the compiled code has been invalidated.
> 
> - Instead of adding RecompileInfo directly to freeze constraints, add an
> index into the compartment's compilation vector.  Then only
> addPendingRecompile when a freeze constraint is triggered on a RecompileInfo
> which has not already been marked as invalidated.
> 
> - Purge the vector of RecompileInfo data whenever analysis data and jitcode
> are purged, i.e. on most GCs.

I think we can store it in the LifoAllocator used for the type constraints as the jit code would be invalidated when its constraints are removed.
(Assignee)

Comment 4

5 years ago
Created attachment 641937 [details] [diff] [review]
[JM] Add CompilerOutput indirection for RecompileInfo.

Add CompilerOutput structure which got invalidated once it is added to the list of pending recompilation.  Modify RecompileInfo to be an index in the Vector of CompilerOutput hosted in the TypeCompartment.
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Attachment #641937 - Flags: review?(bhackett1024)
(Assignee)

Comment 5

5 years ago
Created attachment 641938 [details] [diff] [review]
[Ion] Add CompilerOutput indirection for RecompileInfo.

Same patch, with IonMonkey modifications.
Attachment #641938 - Flags: review?(bhackett1024)
(Assignee)

Comment 6

5 years ago
Created attachment 641939 [details]
Patch Diff [JM] [Ion]
(Assignee)

Updated

5 years ago
Duplicate of this bug: 766805
(Assignee)

Comment 8

5 years ago
Created attachment 642041 [details] [diff] [review]
[Ion] Add CompilerOutput indirection for RecompileInfo.

Update Ion patch to select correctly the compiled script at the finalization of AutoEnterCompilation and to select the correct invalidator based on the compiled output which is invalidated.

This fix the case of invalidation reported in Bug 766805 where JM froze some Typesets which caused Ion invalidations.
Attachment #641938 - Attachment is obsolete: true
Attachment #641939 - Attachment is obsolete: true
Attachment #641938 - Flags: review?(bhackett1024)
Attachment #642041 - Flags: review?(bhackett1024)
Comment on attachment 641937 [details] [diff] [review]
[JM] Add CompilerOutput indirection for RecompileInfo.

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

Nice, r+ with comments fixed.

::: js/src/jsinfer.h
@@ +1117,5 @@
>      bool barriers : 1;
>      uint32_t chunkIndex:30;
>  
> +#ifdef JS_METHODJIT
> +    js::mjit::JITScript *mjit;       /* Information attached by JM */

Keeping the JITScript pointer in here shouldn't be necessary, a one bit field isValid should suffice.  This will simplify the logic and cut memory, and I don't think having the JITScript is useful for distinguishing things.  The only times we will need to recompile are after purging jitcode (which will purge the CompilerOutputs) and after invalidating the previous JITScript, which will mark the latest CompilerOutput for the script as invalid.

@@ +1132,5 @@
> +
> +struct RecompileInfo
> +{
> +    static const uint16_t NoCompilerRunning = 0xffff;
> +    uint16_t outputIndex;

outputIndex should be a uint32, no memory will be saved by using a smaller integer and it is possible that we could hit 2^16 compilations between purges, especially now that it is possible to retain jitcode and analysis data across GCs.
Attachment #641937 - Flags: review?(bhackett1024) → review+
Comment on attachment 642041 [details] [diff] [review]
[Ion] Add CompilerOutput indirection for RecompileInfo.

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

::: js/src/ion/Ion.cpp
@@ +876,5 @@
>      if (!oracle.init(cx, script))
>          return false;
>  
> +    types::AutoEnterCompilation enterCompiler(cx, types::AutoEnterCompilation::Ion);
> +    enterCompiler.init(script, false, 0);

Need to check return value here.

::: js/src/jsinfer.h
@@ +1131,5 @@
> +    /* Result of the compilation */
> +#ifdef JS_METHODJIT
> +    mjit::JITScript *mjit;
> +#endif
> +    ion::IonScript *ion;

As with the other patch, an isValid bit should suffice here instead of JITScript/IonScript pointers, though you'll also I think want an isIon or isJM bit too to distinguish between the two compilers.
Attachment #642041 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 11

5 years ago
(In reply to Brian Hackett (:bhackett) from comment #9)
> Comment on attachment 641937 [details] [diff] [review]
> [JM] Add CompilerOutput indirection for RecompileInfo.
> 
> Review of attachment 641937 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice, r+ with comments fixed.
> 
> ::: js/src/jsinfer.h
> @@ +1117,5 @@
> >      bool barriers : 1;
> >      uint32_t chunkIndex:30;
> >  
> > +#ifdef JS_METHODJIT
> > +    js::mjit::JITScript *mjit;       /* Information attached by JM */
> 
> Keeping the JITScript pointer in here shouldn't be necessary, a one bit
> field isValid should suffice.  This will simplify the logic and cut memory,
> and I don't think having the JITScript is useful for distinguishing things. 

In such case we would need to iterate over the compiledOutputs in addPendingRecompile(cx, script, pc) to change the isValid flag of the remaining compilerOutputs, otherwise they may stay with a valid flag which refer to a previous compilation — which is invalid.

This would be the case with addTypeBarrier and addSingletonTypeBarrier which are causing a recompilation if the analysis had no type barrier for the pc.

Otherwise, only one pointer and a flag can be used for the both JIT.
(In reply to Nicolas B. Pierron [:pierron] from comment #11)
> In such case we would need to iterate over the compiledOutputs in
> addPendingRecompile(cx, script, pc) to change the isValid flag of the
> remaining compilerOutputs, otherwise they may stay with a valid flag which
> refer to a previous compilation — which is invalid.
> 
> This would be the case with addTypeBarrier and addSingletonTypeBarrier which
> are causing a recompilation if the analysis had no type barrier for the pc.

Doing such looping doesn't seem terrible, if you see there is jitcode there you know there is a valid CompilerOuput somewhere in the list of them and can go invalidate it.  Thinking again, what this patch will do now in such cases is leave dangling pointers in the CompilerOutput, since when we hit addPendingRecompile(script, pc) only a fake version of the CompilerOutput is invalidated and not the one in the list.  The behavior is still correct, but this could lead to non-deterministic unnecessary compilation if an IonScript or JITChunk is allocated to the same address as a previous one.  Plus valgrind will probably be angry.

Another option is to store the compilation index in the IonScript or JITChunk (btw you want to use this one for JM rather than JITScript, as JITChunk is the unit of granularity for JM compilation), and invalidate the CompilerOutput when discarding that code for any reason.
(Assignee)

Comment 13

5 years ago
Comment on attachment 642041 [details] [diff] [review]
[Ion] Add CompilerOutput indirection for RecompileInfo.

Ion part: https://hg.mozilla.org/projects/ionmonkey/rev/8ea86b9020a2

It contains additional modification reviewed by dvander.
Attachment #642041 - Flags: checkin+
(Assignee)

Comment 14

5 years ago
Comment on attachment 641937 [details] [diff] [review]
[JM] Add CompilerOutput indirection for RecompileInfo.

https://tbpl.mozilla.org/?tree=Try&rev=d470bd62bcf6
(Assignee)

Comment 15

5 years ago
Comment on attachment 641937 [details] [diff] [review]
[JM] Add CompilerOutput indirection for RecompileInfo.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a25a6c994fd
Attachment #641937 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/4a25a6c994fd
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 776748
(Assignee)

Updated

5 years ago
Blocks: 777537
(Assignee)

Updated

5 years ago
Blocks: 780274
(Assignee)

Updated

5 years ago
No longer blocks: 780274
Depends on: 780274
Depends on: 780549
Depends on: 783464
You need to log in before you can comment on or make changes to this bug.