Closed Bug 1607986 Opened 10 months ago Closed 7 months ago

Clicking "play" on paper.io is extremely slow on Firefox, smooth on Chrome

Categories

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

73 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla76
Tracking Status
firefox73 --- wontfix
firefox76 --- fixed

People

(Reporter: karlcow, Assigned: evilpie)

References

(Blocks 1 open bug, )

Details

(Keywords: perf, Whiteboard: [qf:p3:responsiveness])

Attachments

(4 files)

As reported on Webcompat

Clicking "play" is extremely slow on Firefox, smooth on Chrome
https://perfht.ml/35jATxT is a profile on a fast macbook, https://perfht.ml/2ZiTqZM is a profile on normal user hardware.

fwiw I can't reproduce
with Firefox Nightly 74.0a1 (2020-01-08) (64-bit)
on macbook 16 Go 2133 MHz LPDDR3, 2,3 GHz Intel Core i5 double cœur, Intel Iris Plus Graphics 640 1536 Mo

But ciprian was able to reproduce on macos.

I can reproduce this, on a machine in Germany. I tried to debug this for a bit (wanted to check what the size of the arrays that were being iterated over were, between Firefox and Chrome) but the debugger couldn't handle the site and hung the entire browser.

Component: General → Performance

I think we're bitten by a particularly annoying code obfuscator. The meat of this obfuscator is a "hex number as string" -> number -> string translation table, with the following lookup function:

var _p2_0x4889 = function (_0x4b0df3, _0x5e54a6) {
  _0x4b0df3 = _0x4b0df3 - 0;
  var _0x497a42 = _p2_0x31db[_0x4b0df3];
  return _0x497a42;
};

Almost all of the time is being spent in this function.

With the help of this function, something like _0x1ffc04.base.polygon.intersections(_0x57b7a7) is expressed as follows:

_0x1ffc04[_p2_0x4889('0x11a')][_p2_0x4889('0x15f')][_p2_0x4889('0xf3')](_0x57b7a7)

A fast path for "hex string minus zero" would probably speed this up a fair bit.

What I don't understand is why there is only a long initial pause, and the actual game afterwards runs smoothly. I think the gameplay itself runs the same code.

Component: Performance → JavaScript Engine
Whiteboard: [qf] → [qf:p3:responsiveness]

Adding CacheIR support for numeric - string operations gives me a ~30% speed-up on this µ-benchmark:

var g_array = [0, 1];

function g(p) {
  p = p - 0;
  return g_array[p - 0];
}

function f() {
  var xs = ["0x0", "0x1"];
  var q = 0;
  var t = performance.now()
  for (var i = 0; i < 1000000; ++i) {
    q += g(xs[i & 1]);
  }
  return [performance.now() - t, q];
}

for (var j = 0; j < 10; ++j) {
  console.log(f());
}

But we're actually already ~10% faster than V8, so why is Chrome faster for that web page? V8 is probably able to inline _p2_0x4889 and then constant-fold the subtraction at compile time. For example in the following µ-benchmark, V8 finishes instantly whereas SpiderMonkey actually calls SubValues in each iteration.

var g_array = [0, 1];

function g(p) {
  p = p - 0;
  return g_array[p - 0];
}

function f() {
  var q = 0;
  var t = performance.now()
  for (var i = 0; i < 1000000; ++i) {
    q += g("0x0");
    q += g("0x1");
  }
  return [performance.now() - t, q];
}

for (var j = 0; j < 10; ++j) {
  console.log(f());
}

Here's a quick PoC to implement the aforementioned CacheIR support and also teach IonBuilder to constant-fold the subtraction, in case someone wants to verify if these changes help on that web page: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c64a337e048943cc9e854f23208d65ae8b569f8

Component: JavaScript Engine → JavaScript Engine: JIT

Works great!

This reduces the time from clicking the "Skip Ad" button to the game being displayed by 10x for me, from 20 seconds to 1-3 seconds. It takes 0.5-3 seconds in Chrome for me. I think it's dependent on the existing content of the playing area.
Before: https://perfht.ml/37eVtkO
After: https://perfht.ml/38oJd1c
Chrome: https://perfht.ml/2TIootv

André, do you want to finish this patch?

Flags: needinfo?(andrebargull)
Priority: -- → P1

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

André, do you want to finish this patch?

Not right now, maybe at a later point of time.

Flags: needinfo?(andrebargull)
Keywords: perf
Summary: Clicking "play" is extremely slow on Firefox, smooth on Chrome → Clicking "play" on paper.io is extremely slow on Firefox, smooth on Chrome

I am going to try to split up André's patch and get at least some of it landed.

Assignee: nobody → evilpies

I modified André's code slightly to allow string/int32 on either lhs or rhs.

I still need to write some tests, probably based on some of the benchmarks in this bug. André is there something specific missing from these patches that you still wanted to fix?

Flags: needinfo?(andrebargull)

I don't think so. I just wasn't too happy about using TlsContext.get() and wanted to investigate a non-JSContext approach, but maybe it's okay to use TlsContext here.

Flags: needinfo?(andrebargull)
Blocks: jsperf

We should probably switch to using MToNumber like Wrap, but we can do that in a different bug.
After the third patch, which converts constant strings to numbers we would get assertion
failures in binaryArithTrySpecialized, which uses the baseline inspector.

As far as I can tell for JSOp::Pos only ever MBinaryArithInstruction actually applies.

Blocks: 1626075

Depends on D68825

Blocks: 1626297

I've filed bug 1626297 for comment #12. (I've a patch ready, but it conflicts with the patches from this bug, so I'll wait before requesting review for them.)

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7d5ff4b20e2c
Use MBinaryArithInstruction directly for JSOp::Pos. r=jandem
https://hg.mozilla.org/integration/autoland/rev/0899c7f33144
Add CacheIR IC for String with Int32 arithmetic. r=jandem,anba
https://hg.mozilla.org/integration/autoland/rev/2327b3f650ae
Ion: Convert constant string to number operand for arithmetic r=jandem
https://hg.mozilla.org/integration/autoland/rev/987c2ef0a10c
Test. r=jandem

We should probably try contacting the site owners to get the warning alert removed.

Flags: needinfo?(kdubost)

I just talked to the developers on their Discord server and them removed the warning for Firefox users \o/

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