Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

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

(Whiteboard: [ion:p1:fx18])

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 777788
Whiteboard: [ion:t]
(Assignee)

Comment 1

5 years ago
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.
Attachment #651002 - Flags: review?(bhackett1024)
(Assignee)

Comment 2

5 years ago
(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+
(Assignee)

Comment 4

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
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
Attachment #651002 - Flags: checkin+
No longer blocks: 782024
Duplicate of this bug: 782024
Blocks: 747221

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/1c3efa824966
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(Assignee)

Updated

5 years ago
Duplicate of this bug: 782103
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 ?
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.
Attachment #653175 - Flags: review?(bhackett1024)
Attachment #653175 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b6f5ce1065a
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.