Closed Bug 1515620 Opened 1 year ago Closed 1 year ago

Investigate bad performance with Gameboy emulator

Categories

(Core :: JavaScript Engine: JIT, enhancement, P2)

63 Branch
enhancement

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: alex.fdm, Unassigned)

References

()

Details

I recently read an interesting article about WASM performance for a GameBoy emulator, were Firefox shows great results.  
The problem is the article compares the performance with a JavaScript version of the same emulator, and there the performance of Firefox is way worse than Chrome and Safari.  Maybe some investigation is in order.
Url for reference: https://medium.com/@torch2424/webassembly-is-fast-a-real-world-benchmark-of-webassembly-vs-es6-d85a23f8e193

Looking at the regular spikes in JS slowness here, and the fact that the closure-compiled version does not have the same spikes, I suspect that this is GC related.

The closure compiler should eliminate a number of temporary object allocations, and probably kills lot of of the extra allocations that might be leading to GC pressure.

For ease of reference, we care about the middle section of the set of graphs a third of the way down the page.

Needinfoing sfink.
Flags: needinfo?(sphink)
We consistently bailout when running the benchmark, because the load function from <https://wasmboy.app/benchmark/index.iife.js>, line 1737:
---
  const load = offset => {
    return wasmByteMemory[offset];
  };
---

is called with non-integer inputs, which aren't handled in MToNumberInt32. (The load is handled through IonBuilder::jsop_getelem_typed, which converts the index via MToNumberInt32. MToNumberInt32 when lowered to LDoubleToInt32 bails out with Bailout_PrecisionLoss when the input isn't an integer.)

As a simple test I've removed |id->type() != MIRType::Double| in jit::ElementAccessIsTypedArray [1] - which means typed array accesses are no longer optimized when called with MIRType::Double - and I got a major speed-up in the benchmark [2].

[1] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/js/src/jit/MIR.cpp#5738
[2] The "Current Average FPS" counter at the end of the 2500 frames run of "Tobu Tobu Girl" was at 192 for the WASM case for me, unpatched JavaScript at meager 12 FPS, and the patched JavaScript version at 90 FPS.
The non-integer input to the |load| function looks like a bug in WasmBoy to me. While it'd be nice if non-integer inputs are better handled for typed array accesses or alternatively Ion gets taught to avoid trying to inline the same spot over and over again when bailouts were already observed a million times, the main problem here is probably an issue to be fixed in WasmBoy...
Filed <https://github.com/torch2424/wasmBoy/issues/216> against the wasmBoy project.
Thanks André for this precise analysis!
Flags: needinfo?(sphink)
Depends on: 1517235
Component: JavaScript Engine → JavaScript Engine: JIT

If I read the upstream bug correctly (comment 4), the issue should be fixed upstream, and I recall some of the derivative to have bugs opened on bugzilla as well. So I presume we can close this issue as WORKS_FOR_ME now?

Feel free to re-open this issue if needed.

Status: NEW → RESOLVED
Closed: 1 year ago
Flags: needinfo?(andrebargull)
Priority: -- → P2
Resolution: --- → WORKSFORME

I'm OK with closing, but it would be nice to add a reference to the bug with the array indexing fix on Mozilla's side for future reference.

Filed bug 1526315 for the bailout issue.

Apart from the bailout bug, which is no longer an issue in the wasmBoy project because of https://github.com/torch2424/wasmboy/pull/218, bug 1518857 also affects the performance of wasmBoy (under Linux, but not Windows and probably Mac).

Flags: needinfo?(andrebargull)
You need to log in before you can comment on or make changes to this bug.