1487366 - Rare input argument types cause us to generate pessimistic code

Status: NEW

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

Description

[Paul Bone [:pbone]]

STR:
1. Navigate to http://paul.bone.id.au/pub/pbone-2018-ast2wasm-slides/demo-wasm.html
2. Enter "39" in the input box.
3. Click "Fib (JS)", this runs a naive fib program in JavaScript.
4. Observe the time it takes, (I get about 450ms)
5. Click the button again,
6. Observe the time it takes, (I get about 1100ms)

Expected output: I expected these to be consistent.  Also the 450ms is about the time it takes for the webassembly program to run (consistently).

Comment 1

[Luke Wagner [:luke]]

If I read comment 0 correctly, wasm is the stable one; JS is the one that is strangely changing behavior.

Comment 2

[Paul Bone [:pbone]]

Luke is right, it's the JS code that's slowing down on 2nd and later runs.  I assume it's totally unrealted to the WASM on that page but I dono for sure.  I wrote it to demo some wasm and added the JS as a comparison.

Comment 3

[Nicolas B. Pierron [:nbp]]

Iain, can you take a look at this issue.

One of the thing which surprises me are is the large quantity of leaf-addresses when running the Gecko profiler addon.
However, I was not able to find any IonBuilder / Codegenerator::link associated to the `fib` function in the profile.

I tried disabling IonMonkey from about:config but this slow down the performance, worse than observed without disabling it. I once observed that disabling off-thread compilation delayed the slow down.

I suggest to convert this test case in a JS Shell test case and see if this can be reproduced.

Comment 4

[Iain Ireland [:iain]]

I've managed to reproduce this in the browser. (Profile: https://perfht.ml/2QKTzQa)

I can't seem to replicate the same behaviour in the shell, unfortunately.

I'll keep poking at it.

Comment 5

[Iain Ireland [:iain]]

Figured it out. Here's a testcase:

```
function fib(n) {
    if (n < 2) {
        return 1;
    } else {
        return fib(n-1) + fib(n-2);
    }
}
function benchmark(f) {
    var before = Date.now();
    f();
    var after = Date.now();
    print(global + ": " + (after - before) + " ms");
}
var output;
var input = "39"; // *** NOTE: input is a string here ***
benchmark(() => { global = fib(input); })
benchmark(() => { global = fib(input); })
```

Example output:
102334155: 392 ms
102334155: 928 ms

The key is that fib is *almost* always called with an int32 argument, but when the button is clicked it is called with input.value, which is a string. We don't record the type for the very first call to foo, so the code for the first click is compiled under the assumption that the argument is always an int32. The second click invalidates that assumption, so we have to bail out and recompile a slower version of the code.

I managed to extract iongraphs of the before and after cases. Compared to the fast case, the slow case has:
- two extra valuetoint32
- two places where subi becomes binaryvaluecache
- two places where compareandbranch becomes binaryboolcache

This all makes a lot of sense in retrospect.

I'm not sure that anything needs to be done here. I can think of a couple of options (for example, we could detect recursive functions that call themselves with specific types and compile a separate "inner" version that relies on the "outer" version to handle the first iteration and get the types right) but I doubt it's worth the effort, unless we have some reason to believe that heavily recursive functions are really important in real web code.

Comment 6

[Nicolas B. Pierron [:nbp]]

Iain, thanks for investigating and figuring out this issue. It sounds to me similar to something that arai started to investigate in the past, about having different specializations based on the arguments type.

While this test case seems artificial, it might be worth to apply these learning to see if similar invalidation happens in the real world, i-e degraded performances because of a low-frequency event.

Arai, do you recall the attempt of specializing the compilation based on the input types of functions and what was the bug to follow progress on this topic?

Comment 7

[Tooru Fujisawa [:arai]]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> Arai, do you recall the attempt of specializing the compilation based on the
> input types of functions and what was the bug to follow progress on this
> topic?

it's bug 1375495.
currently it's suspended, and I'm planning to look into it again after shypes (bug 1038917), since it simplifies the handling of object case.

Comment 8

[Iain Ireland [:iain]]

Bumping this down in priority and unassigning myself.

Comment 9

[Paul Bone [:pbone]]

Thanks for figuring this out, I'll try to write better JS next time ;-)

Nevertheless, it could be important to handle this incase someone is doing some random benchmarking like this and starts reporting "OMG Firefox is weird".  Maybe there's not much real world code that does this, but my guess is that it's easy to stumble upon in testing.

Comment 10

[Mayank Bansal]

I got consistent numbers each time I run this : https://share.firefox.dev/45rj5By
Bug 1375495 is resolved as WORKSFORSOME.

:pbone , :iain : is there anything more to do here?

Comment 11

[Iain Ireland [:iain]]

With WarpBuilder, this testcase ends up compiling an IonScript that only handles Int32 arguments to fib, and bailing out on a string input. The first 9 bailouts are ignored. Once numFixableBailouts hits 10, we invalidate and recompile with slightly worse code. The cliff is smaller than before (~1200 -> 1450 instead of 400 -> 900), although it might be because we're generating slightly worse code in the fast case than we did with IonBuilder.

In some cases, trial inlining could deal with this kind of pattern. That's basically equivalent to the solution proposed in bug 1375495. In this particular case, we don't inline because the function is recursive.

The reason the pessimized codegen feels bad here is that we only need it in a tiny fraction of the invocations of fib. Right now we have a static threshold for the number of bailouts before invalidating. If we could scale that threshold to be a fraction of the overall invocations, we could avoid this problem, at the cost of having to track the total number of invocations for Ion functions. It's not obvious to me that this problem is common enough to justify the overhead.