880640 - [PJS] Modify transitive compilation logic not to attempt ion compilation until baseline has completed

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

As part of bug 875276, we no longer gather type information during interpreter runs. The current transitive compilation logic does not consider whether baseline compilation has succeeded or not, leading to numerous problems where we attempt ion compilation without any type information. This in turn can lead to scripts being disqualified from further compilation because they hit odd corner cases in ion code. Bad.

This bug updates the logic in the main transitive compilation loop so that this loop will continue to run warmup iterations so long as there are transitive callees that (1) have no baseline information and (2) continue to be invoked. That is, for callees without baseline, we monitor the use count. If running warmups does not cause it to increase, we give up on compiling that callee, as they are considered unlikely to be called.

This allows us to remove some ad hoc checks we had in place:

- We no longer need to check the use count when we initially enqueue the main function. If it 
is too low, the general mechanism will handle it.

- We no longer need to be check for a 0 use count toi avoid enqueueing callees that are not, in fact, ever called (which has caused us trouble in the past). The callee will simply be considered a stalled script.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Change worklist to monitor for baseline info and use count progress.

Comment 2

[Kannan Vijayan [:djvj]]

Comment on attachment 759689 [details] [diff] [review]
Change worklist to monitor for baseline info and use count progress.

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

::: js/src/vm/ForkJoin.cpp
@@ +244,5 @@
>  
> +    struct WorklistData {
> +        bool calleesEnqueued;
> +        uint32_t useCount;
> +        bool stalled;

You may want to consider adding a |int stallCount| here.  Which can be incremented each time the useCount looks stalled.  When stallCount reaches a threshold value, then the run can be treated as stalled.

This can help avoid situations where some callees are inside conditionals and don't get execute at every run, and get eagerly marked as stalled.

@@ +555,5 @@
>      //       - Invalidate any scripts that may need to be invalidated
>      //       - Re-enqueue main script and any uncompiled scripts that were called
>      // - Too many bailouts: Fallback to sequential
>  
> +    if (!ion::IsBaselineEnabled(cx_) || !ion::IsEnabled(cx_))

!IsBaselineEnabled(cx_) should imply !ion::IsEnabled(cx_) now.  This can be made into a:

JS_ASSERT_IF(!IsBaselineEnabled(cx), !ion::IsEnabled(cx));

before the conditional.  And the conditional can remain the same.

@@ +702,5 @@
> +                         "but use count grew from %d to %d",
> +                         script.get(), script->filename(), script->lineno,
> +                         previousUseCount, currentUseCount);
> +                } else {
> +                    worklistData_[i].stalled = true;

See above comment about stallCount.  Here, stallCount can be incremented, and stalled = true only set when it gets past some threshold.

@@ +887,5 @@
>  bool
>  js::ParallelDo::addToWorklist(HandleScript script)
>  {
> +    uint32_t len = worklist_.length();
> +    for (uint32_t i = 0; i < len; i++) {

Why the caching of worklist_.length()?  Keeping it in the loop context is clearer, and the compiler should lift it anyway.

@@ +911,2 @@
>          return false;
> +    worklistData_[len].reset();

Oh, I see.  That's why :)
Still, it would be clear to do:
worklistData_[workListData_.length() - 1].reset();

The "len" variable is far removed from its usage above, and opens up the possibility of getting out of sync if the code in between changes.

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Change worklist to monitor for baseline info and use count progress.

Incorporate djvj's suggestion. In particular, wait for 3 continuous stalls before considered a script to be "stalled"

Comment 4

[Niko Matsakis [:nmatsakis]]

Try run with new patch: https://tbpl.mozilla.org/?tree=Try&rev=4ab95b68dd4a

Comment 5

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ec850cf19e60

Comment 7

[Ed Morley [:emorley]]

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