1850305 - A js source compressor is 2.5x faster in Chrome compared to Nightly (and we spend a lot of time in Baseline)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Mayank Bansal]

Go to https://codegolf.github.io/zpng/
Set the number of iterations to 1
Copy and paste the sample .js file in the input box
Press "Compile" button

Nightly: https://share.firefox.dev/3QXkgU5 (86 seconds)
Chrome: https://share.firefox.dev/45KSCxF (34 seconds)

ER: Maybe faster.

Comment 1

[Mayank Bansal]

Attachment: Sample js file (unminified d3.js).txt

Comment 2

[Jan de Mooij [:jandem]]

That ke function is pretty large but mostly runs in Baseline. Its size might have something to do with that, although there are some Ion samples of it so we probably do end up compiling it if the profile is correct.

NI myself to see if there's something weird going on with this function.

Comment 3

[Jan de Mooij [:jandem]]

This is a really good find, thanks for reporting this! It's a bailout issue caused by the type policy for MBooleanToInt32.

I have a fix, but I'm still looking into adding some assertions to the type policy code to catch similar issues.

I also have a shell version of this website that I'll attach to this bug. The fix improves that from ~85 seconds to ~16 seconds, almost exactly the same as what I get with an old V8 shell build.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Shell version

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1850305 part 1 - Fix a bailout performance cliff with GuardBooleanToInt32. r?iain!

We were relying on MBooleanToInt32's type policy to insert a fallible unbox
for the to-boolean conversion. This caused a performance cliff because for
frequent TypePolicy bailouts we disable compilation instead of invalidating
the script.

The fix is to add the unbox instruction in the transpiler.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1850305 part 2 - Add some infallible type conversions in the transpiler. r?iain!

This fixes issues caught by the assertion added in the next patch. These operations
are actually infallible so didn't have the same performance cliff.

Depends on D187264

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1850305 part 3 - Add an assertion. r?iain!

This would catch similar issues in the future and is a nice invariant to maintain.

Depends on D187265

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2d4c17692b7
part 1 - Fix a bailout performance cliff with GuardBooleanToInt32. r=iain

Comment 9

[Sandor Molnar[:smolnar]]

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

Comment 10

[Jan de Mooij [:jandem]]

Mayank, can you confirm this is fixed in the latest Nightly?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5fda6c22e8e3
part 2 - Add some infallible type conversions in the transpiler. r=iain
https://hg.mozilla.org/integration/autoland/rev/2689f838632f
part 3 - Add an assertion. r=iain

Comment 12

[Mayank Bansal]

(In reply to Jan de Mooij [:jandem] from comment #10)

Mayank, can you confirm this is fixed in the latest Nightly?

Profile with latest nightly : https://share.firefox.dev/3EmVd5k (https://hg.mozilla.org/mozilla-central/rev/738c0a460acb30ab2ab419f4c0dc93fa9af200d6)
This shows no improvement for some strange reason. However, earlier today I had downloaded a opt build built from the link in comment #8, and that had showed 16 seconds. Not sure whats the reason why Nightly doesnt show the perf improvement.

I will retest in a couple of days when i have my regular laptop charger.

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Mayank Bansal from comment #12)

Profile with latest nightly : https://share.firefox.dev/3EmVd5k (https://hg.mozilla.org/mozilla-central/rev/738c0a460acb30ab2ab419f4c0dc93fa9af200d6)
This shows no improvement for some strange reason. However, earlier today I had downloaded a opt build built from the link in comment #8, and that had showed 16 seconds. Not sure whats the reason why Nightly doesnt show the perf improvement.

I will retest in a couple of days when i have my regular laptop charger.

Mysterious. I just tested this locally with the latest Nightly and I got about 24 seconds. Your latest profile also shows that the ke function now spends almost all of its time in Ion.

Feel free to file a new bug if you're still seeing issues here.

Comment 14

[Norisz Fay [:noriszfay]]

Backed out or causing assertion failures on TypePolicy.cpp

Backout link

Push with failures

Failure log

Comment 15

[Mayank Bansal]

(In reply to Mayank Bansal from comment #12)

(In reply to Jan de Mooij [:jandem] from comment #10)

Mayank, can you confirm this is fixed in the latest Nightly?

Profile with latest nightly : https://share.firefox.dev/3EmVd5k (https://hg.mozilla.org/mozilla-central/rev/738c0a460acb30ab2ab419f4c0dc93fa9af200d6)
This shows no improvement for some strange reason. However, earlier today I had downloaded a opt build built from the link in comment #8, and that had showed 16 seconds. Not sure whats the reason why Nightly doesnt show the perf improvement.

I will retest in a couple of days when i have my regular laptop charger.

I have no idea what I was thinking when I wrote this comment :| . I have some jetlag...

The profile in this comment is 32.9 seconds long. . So we are already faster than Chrome which took 34 seconds on my machine.

This bug improved the original testcase from 84 seconds -> 33 seconds which is a massive 2.5x improvement.
This bug is quite effectively fixed. Thanks :jandem for the fix!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/495c75053248
part 2 - Add some infallible type conversions in the transpiler. r=iain
https://hg.mozilla.org/integration/autoland/rev/7cbe6eba05b9
part 3 - Add an assertion. r=iain

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/495c75053248
https://hg.mozilla.org/mozilla-central/rev/7cbe6eba05b9

Comment 18

[Mayank Bansal]

Definitely fixed: https://share.firefox.dev/3P0twnS