1667864 - JetStream2 subtest async-fs 50% slower with warp enabled

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

Enabling warp slows down async-fs by 50%

Warp Disabled

mozregression --launch 2020-09-28 --pref javascript.options.warp:false
 0:00.75 INFO: Downloading build from: https://archive.mozilla.org/pub/firefox/nightly/2020/09/2020-09-28-09-48-30-mozilla-central/firefox-83.0a1.en-US.mac.dmg

Final: 103.244
First: 90.909
Worst:97.403
Average: 124.283

Profile: https://share.firefox.dev/3j6RNr8

Warp Enabled

mozregression --launch 2020-09-28 --pref javascript.options.warp:true
 0:00.84 INFO: Using local file: /Users/mgaudet/.mozilla/mozregression/persist/2020-09-28--mozilla-central--firefox-83.0a1.en-US.mac.dmg

Final: 52.414
First: 49.505
Worst: 48.387
Average: 60.111

Profile: https://share.firefox.dev/3kWiq2I

Steps to Reproduce

1. Clone https://github.com/mozilla/perf-automation/
2. Apply the following patch:

   diff --git a/benchmarks/JetStream2/index.html b/benchmarks/JetStream2/index.html
   index be216c4..b020c12 100644
   --- a/benchmarks/JetStream2/index.html
   +++ b/benchmarks/JetStream2/index.html
   @@ -36,6 +36,10 @@
        const isInBrowser = true;
        var allIsGood = true;
        window.onerror = function() { allIsGood = false; }
   +    var testList = undefined;
   +    if (window.location.hash) {
   +        testList = window.location.hash.substr(1).split(/,/);
   +    }
    
        async function initialize() {
            if (allIsGood)
   

3. load index.html#async-fs (I use python3 -m http.server 9000 then visit http://ip/index.html#async-fs)
4. Run benchmark.

Comment 1

[Iain Ireland [:iain]]

Attachment: async-fs.js

I copied file-system.js from here and added some code at the bottom to invoke it, which reproduces a slowdown in the shell. I've attached a copy.

This benchmark creates a random "filesystem" made up of random bytes and then does some asynchronous work with it. Notably, to generate the random bytes, the benchmark replaces Math.random with the following code:

let seed;
function resetSeed() {
    seed = 49734321;
}
resetSeed();

Math.random = (function() {
    return function() {
        // Robert Jenkins' 32 bit integer hash function.
        seed = ((seed + 0x7ed55d16) + (seed << 12))  & 0xffffffff;
        seed = ((seed ^ 0xc761c23c) ^ (seed >>> 19)) & 0xffffffff;
        seed = ((seed + 0x165667b1) + (seed << 5))   & 0xffffffff;
        seed = ((seed + 0xd3a2646c) ^ (seed << 9))   & 0xffffffff;
        seed = ((seed + 0xfd7046c5) + (seed << 3))   & 0xffffffff;
        seed = ((seed ^ 0xb55a4f09) ^ (seed >>> 16)) & 0xffffffff;
        return (seed & 0xfffffff) / 0x10000000;
    };
})();

It looks like this function is invoked ~43M times, making up the majority of execution time. If this code is deleted and the normal Math.random is used, the gap between Ion and Warp shrinks to 10%. I have not investigated the remaining regression.

Comment 2

[Jan de Mooij [:jandem]]

For this we need to be smarter about optimizing stores and loads.

• We have a StoreDynamicSlotT and then right after we do a LoadDynamicSlotAndUnbox with a fallible unbox even though it's pretty obvious that there can only be an Int32 stored there.
• Similarly, the store has a pre-barrier even though a MIR analysis should be able to tell there can't be a GC thing stored in that slot because we just loaded an Int32 from it.

The coming months we should look into adding an optimization pass to handle at least these simple cases.

Comment 3

[Tom S. (please needinfo tschuster)]

• seed is a let-binding that means in GetNameIRGenerator::tryAttachGlobalNameValue for GetGName we don't emit a shape-guard, because let-bindings are non-configurable. We should be able to do the same for SetGName, we just have to pass the env through SetPropIRGenerator. This should automatically give us store-to-load forwarding, because the GuardShape instruction is currently in the way.
• We could also improve MLoadDynamicSlot::mightAlias to not require strict slot equality and skip guard instructions.
• We unbox some slots twice, first to Int32 and then to Double
• In MUnbox::foldsTo we fold unboxes of int32 to double. However for constant int32s we are still inserting MToDouble, instead of creating a new double typed MConstant. (I have a patch for this)

Comment 4

[Tom S. (please needinfo tschuster)]

Attachment: Bug 1667864 - Fold MUnbox to constant doubles when possible. r?jandem

Comment 5

[Tom S. (please needinfo tschuster)]

Attachment: Bug 1667864 - Remove shape check for SetGName similar to GetGName. r?jandem

Depends on D91747

Comment 6

[Tom S. (please needinfo tschuster)]

Using the async-fs.js benchmark from Iain using the hand-crafted Math.random the difference between Ion and Warp is now: 2050ms vs 2500ms.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bcafeb4c162e
Fold MUnbox to constant doubles when possible. r=jandem
https://hg.mozilla.org/integration/autoland/rev/716ae161877d
Remove shape check for SetGName similar to GetGName. r=jandem

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/bcafeb4c162e
https://hg.mozilla.org/mozilla-central/rev/716ae161877d

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1666417