Closed Bug 777537 Opened 12 years ago Closed 12 years ago

JM/IonMonkey: Ensure that CompilerOuput status mirror validity without output pointers

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: nbp, Assigned: nbp)

References

Details

(Whiteboard: [ion:p1:fx18])

Attachments

(2 files)

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.
Depends on: 777788
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.
Attachment #651002 - Flags: review?(bhackett1024)
(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.
Status: NEW → ASSIGNED
Summary: IonMonkey: Ensure that CompilerOuput status mirror validity without output pointers → JM/IonMonkey: Ensure that CompilerOuput status mirror validity without output pointers
Blocks: 782024
Whiteboard: [ion:t] → [ion:p1:fx18]
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.
Attachment #651002 - Flags: review?(bhackett1024) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/1c3efa824966
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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 ?
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.
Attachment #653175 - Flags: review?(bhackett1024)
Attachment #653175 - Flags: review?(bhackett1024) → review+
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
Doh! My bad, sorry..
You need to log in before you can comment on or make changes to this bug.