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.

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.

(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.

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.

Same patch, with IonMonkey modifications.

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.

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.

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.

(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.

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.

Comment on attachment 641937 [details] [diff] [review] [JM] Add CompilerOutput indirection for RecompileInfo. https://tbpl.mozilla.org/?tree=Try&rev=d470bd62bcf6

Comment on attachment 641937 [details] [diff] [review] [JM] Add CompilerOutput indirection for RecompileInfo. https://hg.mozilla.org/integration/mozilla-inbound/rev/4a25a6c994fd