1317967 - Unity WebGL stops to render when key press Arrow key

Status: RESOLVED FIXED

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

Description

[Alice0775 White]

+++ This bug was initially created as a clone of Bug #1317960 +++

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.99 Safari/537.36

Steps to reproduce:

1. Launch firefox, open URL http://beta.unity3d.com/jonas/AngryBots/  and let it download.
2. Open Task Manager add-on(https://addons.mozilla.org/en-US/firefox/addon/task-manager/) and observe memory. (In my system its 600+ MB)
3. Now hit refresh(via button on browser or F5), and as soon as browser refresh page & start downloading content again, for some time, memory rises(in my case, memory increased to 800+MB). Let game download again.
4. Now, memory consumption reduced to 400+MB again, which is normal.
5. As soon as game starts again, refresh page again. This time, while still downloading game, the memory consumption is more that as compared to Step#3 (in my case 1,000+MB). At this point of time, i am getting Out of memory error from browser.


Actual results:

1. Browser is getting out of memory, when memory exceeding a threshold (threshold differs from system to system).


Expected results:

1. Memory should not increase progressively and consume only as much as first launch.

Comment 1

[Alice0775 White]

+++ This bug was initially created as a clone of Bug #1317960 +++

When I test Bug 1317960,
Unity WebGL stops to render when key press Arrow key

Steps to reproduce:
1. open URL http://beta.unity3d.com/jonas/AngryBots/ 
2. Key press Arrow key to move a player

Actual Results:
Unity WebGL stops to render

Expected Results:
Not stop.

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=add9dada238ed99b4f93c027b535423f067d3781&tochange=1196bf3032e1bce1fb07a01fd9082a767426c5fb

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]:

Regression window(m-i):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9d1b72a21a395e36cbfd9982243054e64a955bb3&tochange=eafea39ebf3977beefda0a7cdcef8c8266060ca0

Regressed by: eafea39ebf39	Luke Wagner — Bug 1313180 - Baldr: fix accidental disabling of asm.js when wasm is disabled (r=sunfish)

Comment 3

[Alice0775 White]

[Tracking Requested - why for this release]:

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

(can't repro under latest nightly 64 bits, linux x64)

Comment 5

[Alice0775 White]

Attachment: about_suppot

Comment 6

[Alice0775 White]

To easy reproduce the problem, 
You may need to play on a low spec PC. 
you need to key down and hold Arrow key immediately after start game(within 0-2 seconds as soon as possible).

But, It seems to depend on spec of CPU/GPU.

Steps to reproduce:
1. open URL http://beta.unity3d.com/jonas/AngryBots/ 
2. Key down and hold Arrow key to move a player immediately after start game(within 0-2 seconds as soon as possible)

Actual Results:
Unity WebGL stops to render.

Expected Results:
Not stop.

Comment 7

[Luke Wagner [:luke]]

(In reply to Alice0775 White from comment #6)
Thanks for taking the time to bisect!  Since this is performance sensitive, I expect the reason your bisection stopped on bug 1313180 is because it re-enabled asm.js for default profiles (when javascript.options.wasm=false) which significantly changes performance.  Could you try bisecting further with javascript.options.wasm=true?

Comment 8

[Alice0775 White]

(In reply to Luke Wagner [:luke] from comment #7)
> (In reply to Alice0775 White from comment #6)
> Thanks for taking the time to bisect!  Since this is performance sensitive,
> I expect the reason your bisection stopped on bug 1313180 is because it
> re-enabled asm.js for default profiles (when javascript.options.wasm=false)
> which significantly changes performance.  Could you try bisecting further
> with javascript.options.wasm=true?

Regression window(with javascript.options.wasm=true):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c69a0c17a17d4948c66a1e3dae73a959836be628&tochange=6dae0685c9d21b30fbe39ebb31254c733fd43900

Triggered by: 6dae0685c9d2	Luke Wagner — Bug 1313180 - Baldr: only allocate 4 bytes for float32 on the stack (r=bbouvier)

Comment 9

[Alice0775 White]

I cannot reproduce on Nightly53.0a1 x64.
So, this problem seems to be affected to 32bit build only.

Comment 10

[Luke Wagner [:luke]]

Ah, this bug does seem to be a likely culprit; I'll take a look, thanks.

By any chance, does it reproduce on Linux 32-bit?

Comment 11

[Alice0775 White]

(In reply to Luke Wagner [:luke] from comment #10)
> Ah, this bug does seem to be a likely culprit; I'll take a look, thanks.
> 
> By any chance, does it reproduce on Linux 32-bit?


I can reproduce the problem on Nightly53.0a1 32bit Ubuntu16.04 32bit.

And It works without problem on Nightly53.0a1 64bit Ubuntu16.04 64bit.

Comment 12

[Luke Wagner [:luke]]

Yay, thanks!

Comment 13

[Luke Wagner [:luke]]

Attachment: fix-float32

I found one potential culprit: the codegen of WasmStackArg was still using a storeDouble.  Because arguments are stored in order this mostly wasn't a problem unless the very last argument is a float and this clobbers a value in the spill space.  I was able to reproduce the problem with the attached test case and confirm the fix.  Note: since ARM32 puts the single/double in the ABIArg, the ma_vstr() in ARM32's codegen does the right thing already.

I still need to do a 32-bit browser build to confirm the problem and fix.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8811440 [details] [diff] [review]
fix-float32

Review of attachment 8811440 [details] [diff] [review]:
-----------------------------------------------------------------

Great catch!

::: js/src/jit-test/tests/wasm/wasm-abi.js
@@ +35,5 @@
> +                   getLocals +
> +                   adds +
> +                   `    )\n` +
> +                   `)`;
> +        assertEq(wasmEvalText(code).exports.run(100), numLocals * 100 + sum);

Mind to make this a wasmFullPass test, please?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/234ca79c0f2e
Baldr: Fix x86 float32 stack argument passing (r=bbouvier)

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 8811440 [details] [diff] [review]
fix-float32

Approval Request Comment
[Feature/regressing bug #]: 1313180
[User impact if declined]: totally wrong behavior in large wasm apps
[Describe test coverage new/current, TreeHerder]: tests added in m-c
[Risks and why]: low-risk: small change, with test
[String/UUID change made/needed]: none

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/234ca79c0f2e

Comment 18

[Marcia Knous [:marcia]]

Tracking 53- as this is now fixed.

Comment 19

[areya]

https://hg.mozilla.org/mozilla-central/rev/234ca79c0f2e

Comment 20

[Julien Cristau [:jcristau]]

Alice0775 White, can you verify the fix in nightly?  Thanks.

Comment 21

[Julien Cristau [:jcristau]]

Comment on attachment 8811440 [details] [diff] [review]
fix-float32

fix float argument passing on 32bit wasm, take in aurora52

Comment 22

[Alice0775 White]

Reproduced the problem on 2016-Nov17 build[1].
Verified to fix the problem on 2016-Nov-20 build[2].

[1]https://hg.mozilla.org/mozilla-central/rev/13f49da109ea460665ad27c8497cb1489548450c
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 ID:20161117030212

[2]https://hg.mozilla.org/mozilla-central/rev/57a8cde3f08ca9d60bcd8bdd698ceec687f0aed2
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 ID:20161120030205

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e852f73b1eff