Closed Bug 983477 Opened 6 years ago Closed 6 years ago

Preserve scripts and the JIT code of scripts with compiled parallel JIT code across GCs for some maximum N GCs

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: shu, Assigned: shu)

References

Details

Attachments

(1 file, 2 obsolete files)

Not discarding PJS JIT code is very important for performance -- moreso for PJS
than sequential execution, since we have to do a compilation of the entire call
graph from the PJS entry point to do parallel execution.
Preserves JSScripts and their respective JIT code for scripts that have been
used for PJS in the last 1s.
Attachment #8390917 - Flags: review?(terrence)
Attachment #8390917 - Flags: feedback?(nmatsakis)
Depends on: 966567
Blocks: 966567
No longer depends on: 966567
Comment on attachment 8390917 [details] [diff] [review]
Preserve scripts and the JIT code of scripts with compiled parallel JIT code across GCs, provided <1s has passed since last parallel activity

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

This approach is going to lead to all sorts of horrors down the road when the fuzzers start finding bugs that only repro after doing 1s of work on some specific machine or when simple tests start failing unreproducibly on TBPL. Instead, you probably want to use the same heuristic we use for chunk retirement: number of major GC's survied without usage. Please also make it tweakable from the shell so that the fuzzers can poke the parameter directly to sus out problems quicker.
Attachment #8390917 - Flags: review?(terrence) → review-
(In reply to Terrence Cole [:terrence] from comment #2)
> Comment on attachment 8390917 [details] [diff] [review]
> Preserve scripts and the JIT code of scripts with compiled parallel JIT code
> across GCs, provided <1s has passed since last parallel activity
> 
> Review of attachment 8390917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This approach is going to lead to all sorts of horrors down the road when
> the fuzzers start finding bugs that only repro after doing 1s of work on
> some specific machine or when simple tests start failing unreproducibly on
> TBPL. Instead, you probably want to use the same heuristic we use for chunk
> retirement: number of major GC's survied without usage. Please also make it
> tweakable from the shell so that the fuzzers can poke the parameter directly
> to sus out problems quicker.

The problem is that nothing in the user script roots the parallel methods like Array.prototype.mapPar. How can I root those with a heuristic not based on time?
Implementing Terrence's suggested heuristic: surviving some # of major GCs
instead of a period of time.

Terrence, how does this one look?
Attachment #8391792 - Flags: feedback?(terrence)
Attachment #8391792 - Flags: feedback?(nmatsakis)
Attachment #8390917 - Attachment is obsolete: true
Attachment #8390917 - Flags: feedback?(nmatsakis)
Comment on attachment 8391792 [details] [diff] [review]
Preserve scripts and the JIT code of scripts with compiled parallel JIT code across GCs for some maximum N GCs

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

Great! I'm glad it worked so nicely.

::: js/src/jit/IonCode.h
@@ +567,5 @@
> +    }
> +    uint32_t parallelAge() const {
> +        return parallelAge_;
> +    }
> +    uint32_t parallelAgeInc() {

I'd put the verb first and not abbreviate: increaseParallelAge or somesuch.
Attachment #8391792 - Flags: feedback?(terrence) → feedback+
(In reply to Terrence Cole [:terrence] from comment #5)
> Comment on attachment 8391792 [details] [diff] [review]
> Preserve scripts and the JIT code of scripts with compiled parallel JIT code
> across GCs for some maximum N GCs
> 
> Review of attachment 8391792 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great! I'm glad it worked so nicely.
> 
> ::: js/src/jit/IonCode.h
> @@ +567,5 @@
> > +    }
> > +    uint32_t parallelAge() const {
> > +        return parallelAge_;
> > +    }
> > +    uint32_t parallelAgeInc() {
> 
> I'd put the verb first and not abbreviate: increaseParallelAge or somesuch.

I have it as a suffix because it is a postfix increment.
Comment on attachment 8391792 [details] [diff] [review]
Preserve scripts and the JIT code of scripts with compiled parallel JIT code across GCs for some maximum N GCs

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

::: js/src/jit/Ion.cpp
@@ +521,5 @@
> +            return false;
> +    }
> +
> +    if (script->hasParallelIonScript())
> +        script->parallelIonScript()->resetParallelAge();

I'd like to avoid needing to do a hash lookup every time we enter a parallel operation. Perhaps we could do something like this:

1. When we begin parallel execution for a script S, we add it to the set.
2. Therefore, when we add a parallel ion script to S, it is guaranteed to be in the set.
3. Therefore, if S->hasParallelIonScript(), then S is known to be in the set, and we can simply reset the parallel age and return true.
4. We remove from the set when the parallel ion script is collected.

What do you think?
Attachment #8391792 - Flags: feedback?(nmatsakis) → feedback+
Summary: Preserve scripts and the JIT code of scripts with compiled parallel JIT code across GCs, provided <1s has passed since last parallel activity → Preserve scripts and the JIT code of scripts with compiled parallel JIT code across GCs for some maximum N GCs
Flagging you both for review. The boundaries for who should look at which file
are hopefully obvious. :)

Now that I've fixed all the jit-test crashes, the integration is not as clean
as I had originally hoped in that I had to muck with TypeZone::sweep as well.

Changes:

- Caches the member-of-activeParallelEntryScripts_ property on the IonScript
  itself to avoid rehashing on the ForkJoin fast path.

- Fix crashes.
Attachment #8392621 - Flags: review?(terrence)
Attachment #8392621 - Flags: review?(nmatsakis)
Attachment #8391792 - Attachment is obsolete: true
Comment on attachment 8392621 [details] [diff] [review]
Preserve scripts and the JIT code of scripts with compiled parallel JIT code across GCs for some maximum N GCs v2

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

r=me
Attachment #8392621 - Flags: review?(terrence) → review+
Comment on attachment 8392621 [details] [diff] [review]
Preserve scripts and the JIT code of scripts with compiled parallel JIT code across GCs for some maximum N GCs v2

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

This looks good. It's not quite what I expected, but I think it is better.

I expected to add all scripts (regardless of whether they are the entry script or not) to the set before we started compiling them in parallel, and then just using the "hasParallelIonScript()" flag a queue that they must be the in the set. Essentially the set would be "all parallel ion scripts". Your solution leads to a smaller set, my solution saves a flag. On balance, I imagine your solution saves a lot more memory.

The other advantage of the separate flag solution is that the invariant that "if the bit is set, no need to check the hashtable" is self-contained and maintained purely by the routine that adds the entry to the hashtable, rather than being coordinated across ForkJoin.cpp's compilation routines and so forth. So yeah, nice.

::: js/src/jit/Ion.cpp
@@ +525,5 @@
> +{
> +    // Fast path. The isParallelEntryScript bit guarantees that the script is
> +    // already in the set.
> +    if (script->parallelIonScript()->isParallelEntryScript()) {
> +        MOZ_ASSERT(isActiveParallelEntryScript(script));

If that helper is only used here (I didn't search all files), maybe just inline it into the assert.
Attachment #8392621 - Flags: review?(nmatsakis) → review+
https://hg.mozilla.org/mozilla-central/rev/b081f280543b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.