Last Comment Bug 777537 - JM/IonMonkey: Ensure that CompilerOuput status mirror validity without output pointers
: JM/IonMonkey: Ensure that CompilerOuput status mirror validity without output...
Status: RESOLVED FIXED
[ion:p1:fx18]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: Nicolas B. Pierron [:nbp]
:
:
Mentors:
: 782024 782103 (view as bug list)
Depends on: 772509 777788
Blocks: IonGreen
  Show dependency treegraph
 
Reported: 2012-07-25 15:53 PDT by Nicolas B. Pierron [:nbp]
Modified: 2012-08-21 02:19 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Ensure validity of CompilerOutput without a dandling pointer. (31.54 KB, patch)
2012-08-10 14:39 PDT, Nicolas B. Pierron [:nbp]
bhackett1024: review+
nicolas.b.pierron: checkin+
Details | Diff | Splinter Review
fix non-mjit builds (785 bytes, patch)
2012-08-19 06:18 PDT, Landry Breuil (:gaston)
bhackett1024: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2012-07-25 15:53:46 PDT
The current implementation CompilerOutput can still own a dandling pointer to a compiler output which has been invalidated.  The current implementation is not supposed to use this pointer because we ensure that everything is valid before making any changes.

Making the CompilerOutput mirror the validity would be nicer.  This imply to modify addPendingRecompiles(cx, script, pc) to reuse the current CompilerOutputs instead of forging new one from scratch.

With such modifications we can ensure that a compilation output can be invalidated only once and thus remove the output pointer because the script is already holding it.
Comment 1 Nicolas B. Pierron [:nbp] 2012-08-10 14:39:33 PDT
Created attachment 651002 [details] [diff] [review]
Ensure validity of CompilerOutput without a dandling pointer.

Add a constraint index on each result of a compilation.  This index correspond to the CompilerOutput structure which identify the result of one compilation in a JSScript.

This patch definitively fix unrelated invalidation.

Currently this patch is block by some marking issue in IonMonkey, but a subset of it can be used for mozilla-central.
Comment 2 Nicolas B. Pierron [:nbp] 2012-08-10 16:20:42 PDT
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #1)
> Currently this patch is block by some marking issue in IonMonkey, but a
> subset of it can be used for mozilla-central.

Update: it works fine against the HEAD of IonMonkey.
& Rename the Title to mention that this Bug targets both IonMonkey and central.
Comment 3 Brian Hackett (:bhackett) 2012-08-12 17:43:50 PDT
Comment on attachment 651002 [details] [diff] [review]
Ensure validity of CompilerOutput without a dandling pointer.

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

::: js/src/ion/CodeGenerator.cpp
@@ +1231,5 @@
>  
> +#if defined(DEBUG) && (defined(JS_CPU_X64) || defined(JS_CPU_X86))
> +            // No-op used to index the LIR instructions in IonGraph output.
> +            masm.store16(Imm32(iter->id()), Address(StackPointer, -8));
> +#endif

What is this change for?

::: js/src/ion/IonCode.h
@@ +206,5 @@
>  
>      // Number of times this function has tried to call a non-IM compileable function
>      uint32 slowCallCount;
>  
> +    types::RecompileInfo constraintIndex_;

Needs a comment, and a better name (recompileInfo_?)

::: js/src/jsinfer.cpp
@@ +2201,5 @@
> +            mjit::JITScript *jit = script->getJIT((bool) constructing, (bool) barriers);
> +            if (!jit)
> +                continue;
> +
> +            for (unsigned int c = 0; c < jit->nchunks; c++) {

This needs to only invalidate the chunk containing pc, as in the original code.

::: js/src/methodjit/Compiler.cpp
@@ +1034,5 @@
>  
>          if (desc.chunk) {
> +            // Look for the CompilerOutput of the chunk and invalidate it.
> +            types::TypeCompartment &types = cx->compartment->types;
> +            desc.chunk->constraintIndex.compilerOutput(types)->invalidate();

Can you put this logic inside JITScript::destroyChunk itself?  Having to call both destroyChunk and invalidate() together is error prone.
Comment 4 Nicolas B. Pierron [:nbp] 2012-08-14 16:05:00 PDT
(In reply to Brian Hackett (:bhackett) from comment #3)
> Comment on attachment 651002 [details] [diff] [review]
> Ensure validity of CompilerOutput without a dandling pointer.
> 
> Review of attachment 651002 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/CodeGenerator.cpp
> @@ +1231,5 @@
> >  
> > +#if defined(DEBUG) && (defined(JS_CPU_X64) || defined(JS_CPU_X86))
> > +            // No-op used to index the LIR instructions in IonGraph output.
> > +            masm.store16(Imm32(iter->id()), Address(StackPointer, -8));
> > +#endif
> 
> What is this change for?

This is mostly for tracking issues in debug builds, this is supposed to be a no-op because we should never look below the stack pointer (due to an ARM restriction).  So we can safely erase the space below the stack pointer on arch other than ARM to track the LIR index listed in IonGraph output.

This is unrelated to the current patch.

> ::: js/src/ion/IonCode.h
> @@ +206,5 @@
> >  
> >      // Number of times this function has tried to call a non-IM compileable function
> >      uint32 slowCallCount;
> >  
> > +    types::RecompileInfo constraintIndex_;
> 
> Needs a comment, and a better name (recompileInfo_?)

I feel like the constraint mechanism is orthogonal to the type inference, and that we should move it out-side the TypeCompartment.  The RecompileInfo is only used to index a compiler output in the list of constrained outputs.  This explain why I would prefer to do the opposite renaming.  But in the mean time I will apply your nit and this feeling can potentially be part of another bug later.
Comment 5 Nicolas B. Pierron [:nbp] 2012-08-14 18:41:03 PDT
Comment on attachment 651002 [details] [diff] [review]
Ensure validity of CompilerOutput without a dandling pointer.

ionmonkey: https://hg.mozilla.org/projects/ionmonkey/rev/cfc77da79f9f
inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3efa824966
Comment 6 David Anderson [:dvander] 2012-08-14 19:24:31 PDT
*** Bug 782024 has been marked as a duplicate of this bug. ***
Comment 7 Ed Morley [:emorley] 2012-08-15 09:48:29 PDT
https://hg.mozilla.org/mozilla-central/rev/1c3efa824966
Comment 8 Nicolas B. Pierron [:nbp] 2012-08-15 11:39:42 PDT
*** Bug 782103 has been marked as a duplicate of this bug. ***
Comment 9 Landry Breuil (:gaston) 2012-08-19 04:19:33 PDT
This broke non-methodjit builds :

js/src/jsinfer.h:1021: error: 'mjit' has not been declared
js/src/jsinfer.h:1021: error: ISO C++ forbids declaration of 'JITScript' with no type
js/src/jsinfer.h:1021: error: expected ';' before '*' token

See http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/382/steps/build/logs/stdio

Dunno what's the best fix here, put the mjit namespace declaration at the beginning of jsinfer.h outside #ifdef JS_METHODJIT or add more #ifdefs to the decl of ::mjit() and callers.

I'd go for the first solution, anyone has an advice on it ?
Comment 10 Landry Breuil (:gaston) 2012-08-19 06:18:09 PDT
Created attachment 653175 [details] [diff] [review]
fix non-mjit builds

This fixes that build failure for me on OpenBSD/sparc64. The build now stumbles upon gcc 4.2 (see https://bugzilla.mozilla.org/show_bug.cgi?id=777174#c18) but that's another issue.
Comment 12 Ed Morley [:emorley] 2012-08-20 09:34:30 PDT
The bug number for this commit was typoed, causing the merge tool to close the wrong bug (and future hg blame to be wrong) :-(

https://hg.mozilla.org/mozilla-central/rev/9b6f5ce1065a
Comment 13 Landry Breuil (:gaston) 2012-08-21 02:19:06 PDT
Doh! My bad, sorry..

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