Closed Bug 1395900 Opened 7 years ago Closed 7 years ago

for-in loops can trigger lots of IonBuilder loop restarts

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

The function below triggers 25 (!) loop restarts in IonBuilder. This is because we don't know the IteratorMore type and also because we don't know "x" has type MIRType::String in the loop.

I noticed this first on Speedometer. A patch for this reduces the number of restarts from ~1850 to ~650 so it seems pretty effective.

var o = {y: 1};
function f() {
    for (var i = 0; i < 10000; i++) {
        for (var x in o) {}
        for (var x in o) {}
        for (var x in o) {}
        for (var x in o) {}
        for (var x in o) {}
        for (var x in o) {}
        for (var x in o) {}
        for (var x in o) {}
        for (var x in o) {}
        for (var x in o) {}
        for (var x in o) {}
        for (var x in o) {}
    }
}
f();
(In reply to Jan de Mooij [:jandem] from comment #0)
> The function below triggers 25 (!) loop restarts in IonBuilder.

FWIW my patches get rid of all of these loop restarts for the function in comment 0.
This does not affect Speedometer much, but I noticed it on my micro-benchmarks.

In analyzeNewLoopTypes, we first look for a pre-existing loop header so we can reuse its phi types. If we can't use the existing loop header because oldEntry->isDead(), we happily return from analyzeNewLoopTypes.

Instead, I think we should just fall through to the code after the loop, in this case.
Attachment #8903543 - Flags: review?(tcampbell)
This teaches analyzeNewLoopTypes about MIteratorMore's MIRType::Value type, so we don't trigger a loop abort for each for-in loop.
Attachment #8903545 - Flags: review?(tcampbell)
nonStringIteration_ is dead.

We used it to unbox |x| to string in |for (x in y)|, but I think I broke that a long time ago when I removed JSOP_ITERNEXT (it was merged with JSOP_ITERMORE).
Attachment #8903546 - Flags: review?(tcampbell)
This adds JSOP_ITERNEXT back. It's a NOP in the interpreter and Baseline, but IonBuilder can use it to unbox the value as string.

Unfortunately legacy generators can return non-strings, in that case we bail and forbid Ion compilation of the script. Legacy generators will likely be removed soon and shouldn't be used on the web.
Attachment #8903549 - Flags: review?(tcampbell)
Comment on attachment 8903545 [details] [diff] [review]
Part 2 - Add MIteratorMore type to analyzeNewLoopTypes

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

::: js/src/jit/IonBuilder.cpp
@@ +563,5 @@
>      }
>      if (!loopHeaders_.append(LoopHeader(cfgBlock->startPc(), entry)))
>          return abort(AbortReason::Alloc);
>  
> +    if (loopEntry->isForIn()) {

Good find :)
Attachment #8903545 - Flags: review?(tcampbell) → review+
Comment on attachment 8903549 [details] [diff] [review]
Part 4 - Add JSOP_ITERNEXT

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

Please update jsopcodes.cpp so dis() doesn't show |<unknown>|
https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/js/src/jsopcode.cpp#678-701

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7508,5 @@
>          auto loopDepth = this->stackDepth;
>  #endif
>          MOZ_ASSERT(loopDepth >= 2);
>  
> +        if (iflags == JSITER_ENUMERATE) {

Add stack comment
Attachment #8903549 - Flags: review?(tcampbell) → review+
Attachment #8903546 - Flags: review?(tcampbell) → review+
Comment on attachment 8903543 [details] [diff] [review]
Part 1 - Fix issue in analyzeNewLoopTypes

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

If I'm reading this right, your patch will allow us to retry seeding types through |GETLOCAL; SETLOCAL| sequences (the type of other ops wont be different than first time we saw this loop header). If your micro-benchmarks show this gaining us new type info, patch seems good to me.

One side-effect is that loopHeaders_ adds new entries. An alternative would be |break| instead of |continue| and updating the matching entry instead of allocating a new one. I'm fine with leaving as is, but might be clearer to reuse the entry.
Attachment #8903543 - Flags: review?(tcampbell) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f719e2cf81ea
part 1 - Try to get new loop types in analyzeNewLoopTypes even if the previous header is dead. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc73e6b6a21
part 2 - Teach analyzeNewLoopTypes about for-in iterator value slot. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/456b4480fdbf
part 3 - Remove unused IonBuilder::nonStringIteration_. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/57a63c3f76fb
part 4 - Add JSOP_ITERNEXT to improve iterator key type information in Ion. r=tcampbell
(In reply to Ted Campbell [:tcampbell] from comment #8)
> One side-effect is that loopHeaders_ adds new entries. An alternative would
> be |break| instead of |continue| and updating the matching entry instead of
> allocating a new one. I'm fine with leaving as is, but might be clearer to
> reuse the entry.

Yeah, fair enough. I changed it to a break, updated the existing entry, and added a foundEntry bool so we don't add a new entry after the loop in this case.
This seems to have improved sunspider tagcloud by 3-4%.

Sunspider now takes 185-187 ms on the Mac x64 slave. Back in March, this was ~216 ms. So we "accidentally" improved sunspider by more than 10% - simply by optimizing performance all over the place :)
You need to log in before you can comment on or make changes to this bug.