Closed Bug 1451443 Opened 2 years ago Closed 2 years ago

Remove CompilerOutput, simplify RecompileInfo

Categories

(Core :: JavaScript Engine: JIT, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Attached patch PatchSplinter Review
We have a very complicated mechanism to track compilations affected by type constraints. There's a Vector of CompilerOutputs; RecompileInfo stores an index into that vector. These CompilerOutputs and RecompileInfos can be swept during incremental sweeping and this requires a second Vector and a lot of code for dealing with this.

The attached patch rewrites this as follows:

* Each IonScript has a unique IonCompilationId (uint64_t).
* RecompileInfo stores a JSScript* and IonCompilationId.
* Invalidating a RecompileInfo will trigger invalidation of the IonScript iff the IonCompilationId matches.
* A RecompileInfo can be swept if its script no longer has an IonScript with a matching IonCompilationId.

It's much simpler: we no longer need CompilerOutput + the Vectors in TypeZone. RecompileInfo no longer needs a generation field.

The only downside is that RecompileInfo is slightly bigger now but that should be okay I think.

 10 files changed, 146 insertions(+), 299 deletions(-)
Attachment #8965029 - Flags: review?(tcampbell)
This is green on Try (Linux64).
Comment on attachment 8965029 [details] [diff] [review]
Patch

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

Much clearer than the current code :)

It would be nice if there were a way to assert there are no live RecompileInfos when we are about to compact scripts. I'm not sure an easy way to do it though. These bare heap pointers to GCThings make me uneasy.

::: js/src/jit/Ion.cpp
@@ +2973,5 @@
>  
>      // Add an invalidation reference to all invalidated IonScripts to indicate
>      // to the traversal which frames have been invalidated.
>      size_t numInvalidations = 0;
>      for (size_t i = 0; i < invalid.length(); i++) {

Might as well use a range-based loop here too.
Attachment #8965029 - Flags: review?(tcampbell) → review+
Priority: -- → P3
(In reply to Ted Campbell [:tcampbell] from comment #2)
> It would be nice if there were a way to assert there are no live
> RecompileInfos when we are about to compact scripts. I'm not sure an easy
> way to do it though. These bare heap pointers to GCThings make me uneasy.

Note that IsAboutToBeFinalized (called by RecompileInfo::shouldSweep) also does update-when-compacting:

https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/js/src/gc/Marking.cpp#3386-3389

So after we compact we do a final "sweep" to fix up pointers.

(In reply to Ted Campbell [:tcampbell] from comment #2)
> >      for (size_t i = 0; i < invalid.length(); i++) {
> 
> Might as well use a range-based loop here too.

Done. I didn't do this initially because I was worried the Vector could change while we're iterating over it, but I just double checked the callers/callees and that can't happen.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67ff53988f4d
Remove CompilerOutput and simplify Ion code invalidation. r=tcampbell
Here's a bonus patch to use Vector's move constructor in TypeZone::processPendingRecompiles instead of using a for-loop to copy.
Attachment #8965620 - Flags: review?(tcampbell)
Attachment #8965620 - Attachment is obsolete: true
Attachment #8965620 - Flags: review?(tcampbell)
Attachment #8965622 - Flags: review?(tcampbell)
Fun:

(1) I didn't get this on Try and it also didn't show up on my inbound push - e.g. it looks like the hazard build is non-deterministic. 

(2) This hazard is not a real hazard - the function it's complaining about is only called in suppress-GC contexts.

I'll see if I can work around it somehow.
Flags: needinfo?(sphink)
So looking at gcFunctions.txt for both a green and red build, the analysis thinks:

(1) js::FinishCompilation *cannot* GC.
(2) generateTypeConstraint *can* GC.

(1) is the only caller of (2), so I'm not sure why it treats (2) as a GC function. Also I don't understand why it does not always report a hazard here if it really believes (2) to be true.
Retriggering the shell hazard build on my Try push gives me 11 Green, 5 Red:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f91a39cf3a83b9e1f44cecbc0c338ec442db8d5f&filter-searchStr=hazard

It fails reliably if I add AutoCheckCannotGC to generateTypeConstraint. Adding AutoSuppressGCAnalysis to generateTypeConstraint would probably fix this, but I don't want to do that if it's a bug in the rooting analysis.
Attached patch Hazard fixSplinter Review
This seems to work.

I'll leave it up to you to decide whether this is an appropriate fix :)
Attachment #8965742 - Flags: review?(sphink)
Comment on attachment 8965742 [details] [diff] [review]
Hazard fix

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

Thanks for pointing this out. It reminds a lot of another recent regression that came with the upgrade to gcc 6.1, which turned out to be sixgill not handling a particular construct that apparently gcc 4.9 wasn't using (bug 1450379). But I didn't notice non-determinism with that. This is a great test case.

There's a small amount of nondeterminism expected in the hazard analysis, but it should only affect functions with identical names and types that are compiled in different compilation units, eg main() when you're compiling multiple final binaries. I would not expect that to be happening here (normally you'd get link errors if these were present, unless one was weak or something.)

Since I'm going to be on PTO for a week, I'm ok with landing this temporarily to get the hazard out of the way, but I've filed bug 1452171 for removing it. The analysis needs to be fixed.
Attachment #8965742 - Flags: review?(sphink) → review+
Flags: needinfo?(sphink)
Attachment #8965622 - Flags: review?(tcampbell) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dae25f5b42df
Remove CompilerOutput and simplify Ion code invalidation. r=tcampbell
Thanks, Steve!

I folded all patches in this bug and pushed. Fingers crossed.
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/dae25f5b42df
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1452581
You need to log in before you can comment on or make changes to this bug.