1451443 - Remove CompilerOutput, simplify RecompileInfo

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

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

Comment 1

[Jan de Mooij [:jandem]]

This is green on Try (Linux64).

Comment 2

[Ted Campbell [:tcampbell]]

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.

Comment 3

[Jan de Mooij [:jandem]]

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67ff53988f4d
Remove CompilerOutput and simplify Ion code invalidation. r=tcampbell

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Use move constructor in processPendingRecompiles

Here's a bonus patch to use Vector's move constructor in TypeZone::processPendingRecompiles instead of using a for-loop to copy.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Use move constructor in processPendingRecompiles

Comment 7

[Andreea Pavel [:apavel]]

Backed out for hazard failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6f79a4a6772451c6120d8eae24bfabe8530c068c

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=172260864&repo=mozilla-inbound&lineNumber=36381

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/a040a354d2960d80b8264e4a290cc16e8641a33d

Comment 8

[Jan de Mooij [:jandem]]

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.

Comment 9

[Jan de Mooij [:jandem]]

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.

Comment 10

[Jan de Mooij [:jandem]]

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.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Hazard fix

This seems to work.

I'll leave it up to you to decide whether this is an appropriate fix :)

Comment 12

[Steve Fink [:sfink] [:s:]]

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dae25f5b42df
Remove CompilerOutput and simplify Ion code invalidation. r=tcampbell

Comment 14

[Jan de Mooij [:jandem]]

Thanks, Steve!

I folded all patches in this bug and pushed. Fingers crossed.

Comment 15

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/dae25f5b42df