Project: v8 Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 3 users
Status: Fixed
Owner:
Closed: Oct 2015
Cc:
Components:
HW: ----
OS: ----
Priority: 2
Type: Bug



Sign in to add a comment
TurboFan/asm.js memory not immediately released
Reported by alonza...@gmail.com, Aug 26 2015 Back to list
Investigating Unity failures to load in Chrome, I noticed that the following appears to happen:

1. asm.js code is compiled eagerly in TurboFan, reaching 1.1 GB
2. The game then starts to run, allocating a large typed array, etc., further increasing memory to 1.7 GB
3. Then memory decreases back to around 1.0 GB

When I manually separate the asm.js code into a separate script tag, and load the rest of the code (that allocates the large typed array, etc.) after a delay - a setTimeout of 1ms seems sufficient - then instead I see the 1.1 GB of compilation go down, then when the game starts to run, memory increases but it never passes the 1.1 GB point, avoiding the 1.7 GB spike.

Perhaps is the separation + delay enough to run the event loop or something else that leads to a collection of some temporary memory used by TurboFan? If so, I wonder if it might make sense to immediately release all that memory when compilation concludes (force a GC, if it's GC-allocated, or something like that)?
 
Comment 1 by and...@gmail.com, Aug 26 2015
We'd also be interested to know if there was a guaranteed way to flush out the asm.js compilation memory before allocating the big typed array.  Alon found that setTimeout() seems to work pretty well (that is, returning to the event loop before doing the typed array allocation), and I was curious if this was the most reliable mitigation.
Comment 2 by kbr@chromium.org, Aug 26 2015
Cc: alonza...@gmail.com
Labels: TurboFan
Owner: ----
Cc: seththompson@chromium.org
Labels: Type-Bug Area-Compiler Priority-Medium
Status: Available
Comment 4 by jfb@chromium.org, Sep 3 2015
Cc: jfb@chromium.org
Cc: jochen@chromium.org
This isn't compilation memory. It's V8 parser / scope analysis memory, which happens before any compiler gets to the code.

The main issue is the zone memory (i.e. C++ land) memory allocated by the parser and scope analysis for the module function. This ultimately turtles down to C++ new, which I think Blink has a hook to manage. It might be that Blink doesn't recycle this malloc'd and freed memory quite fast enough and that your experiments with waiting triggered a cleanup.

We probably need to ask an expert on Blink memory management, so I'll cc jochen@chromium.org.

On the V8 side, we are working on reducing the memory used by the parser by parsing the nested functions in different zones.


Comment 6 by jfb@chromium.org, Sep 14 2015
Cc: conradw@chromium.org
I believe conradw's recent patch should help with this issue:
https://codereview.chromium.org/1304923004/

This bug also looks like a duplicate of 417697.
Comment 7 by lwag...@mozilla.com, Sep 16 2015
That patchlooks like it would be very helpful.  I tried on the latest Canary on Windows (47.0.2510.0) on the DT2 demo (http://beta.unity3d.com/jonas/DT2/) and unfortunately see the same big spike that I see on Chrome 45.  I'm hoping perhaps conradw's patch is not on Canary yet?
Comment 8 by jochen@chromium.org, Sep 16 2015
  new/delete calls just go through the system allocator, the memory you delete is available immediately
That Canary does contain my change, but there's a bug in the CL that was preventing anything from actually getting allocated to the temp_zone. A fix is in flight.
Project Member Comment 10 by bugdroid1@chromium.org, Sep 17 2015
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/b185ed40611d8b5b1a6b8eea7288c28bc5d9b4d8

commit b185ed40611d8b5b1a6b8eea7288c28bc5d9b4d8
Author: conradw <conradw@chromium.org>
Date: Thu Sep 17 09:38:22 2015

Fix temp_zone scoping when parsing inner function literals

BUG= v8:4392 
LOG=Y

Review URL: https://codereview.chromium.org/1354523003

Cr-Commit-Position: refs/heads/master@{#30792}

[modify] http://crrev.com/b185ed40611d8b5b1a6b8eea7288c28bc5d9b4d8/src/ast.h
[modify] http://crrev.com/b185ed40611d8b5b1a6b8eea7288c28bc5d9b4d8/src/parser.cc

Testing in the v8 shell, I see a big improvement after that second revision that landed today, it goes from 1.81 GB to 0.97 GB on a big testcase, so it's around 50% less. Nice!
Great work!  After this has a little time to bake in Canary, is the risk profile of the patch such that it would be possible to land on Dev or Beta channel?  There are a lot of Unity users who are very anxious to be able to ship their products (and currently unable to do so due to crashes that this patch would likely fix) and so shaving off 6 or 12 weeks would be most appreciated by those users.
+1 let's make sure this is a stable change, but this has immense user value--especially with NPAPI api deprecation breaking Unity's plugin
Labels: merge-tbd-4.6
This has not yet landed on Canary, so this discussion can continue mid next week when we should have enough coverage.

I think it makes sense to merge it into 46 because we still have a few beta pushes left which should give us the opportunity to get enough test coverage.

I would advise against merging it into 45 because of the following reasons:
- Risk cost is high because it touches the general creation of the AST
- It is not a 'breaking' bug
- The delta between Canary and Stable is high so a lot of coverage is missing
- 45 will be obsolete in around 3 weeks

Just a heads up that the change should be on Canary now.
Cc: rossberg@chromium.org mstarzinger@chromium.org
Labels: -merge-tbd-4.6 merge-Approved-4.6
This seems to be fixed and I am not aware of negative side-effects. 

Merge to 4.6 is a yes.
Owner: rossberg@chromium.org
Status: Fixed
For the record, we only saved about half the memory possible here, so if that's not enough in practice, feel free to reopen this bug.
The improvement so far is extremely useful, and if it's feasible to save another large amount of memory here, I think that would be extremely useful as well.

On a Unity game, I see Chrome after this change use 976MB at peak, while Firefox takes 449MB. So Chrome is still using over 2x the memory, and the amounts of memory here are big enough - on the order of 1GB - that this is definitely going to be noticeable in some cases, and cause some users problems.

There is also a scaling issue here. I'm working on a compilation-memory benchmark for Massive, which generates increasingly-larger asm.js modules. Chrome scales linearly in the module size, while Firefox uses an almost fixed amount of memory (I believe this is due to Firefox compiling methods one by one, and freeing up all parsing and compilation memory as it finishes each). Chrome's behavior implies that larger games will see bigger problems. This isn't theoretical - Unreal Engine for example is larger than Unity, and in both of those engines, the total amount of code will depend on user scripts as well, which are compiled together with the engine. In some cases the amount of user code can be very large (in Unity for example, people often import a bunch of C# libraries).
Project Member Comment 20 by bugdroid1@chromium.org, Oct 8 2015
Labels: merge-merged-4.6
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/304da28bc0097013f9477ed616a9fc3f651644d3

commit 304da28bc0097013f9477ed616a9fc3f651644d3
Author: rossberg <rossberg@chromium.org>
Date: Thu Oct 08 12:30:04 2015

Cherry-picked: Fixes for initial memory pressure

33ec0b79b8ea60dcccf1d445b0cbd2eed8e1a165 (Allocate AstNodes of inner functions in temporary zone)
b185ed40611d8b5b1a6b8eea7288c28bc5d9b4d8 (Fix temp_zone scoping when parsing inner function literals)

BUG= v8:4392 
LOG=Y
R=hablich@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review URL: https://codereview.chromium.org/1399503002

Cr-Commit-Position: refs/branch-heads/4.6@{#29}
Cr-Branched-From: 24d34a8ae3cad186792fb1e44e2d7c00d49cd181-refs/heads/4.6.85@{#1}
Cr-Branched-From: 8f441181a570c44ef5c949e8dfd9fd326ac10345-refs/heads/master@{#30256}

[modify] http://crrev.com/304da28bc0097013f9477ed616a9fc3f651644d3/src/ast.h
[modify] http://crrev.com/304da28bc0097013f9477ed616a9fc3f651644d3/src/parser.cc
[modify] http://crrev.com/304da28bc0097013f9477ed616a9fc3f651644d3/test/cctest/test-parsing.cc
[add] http://crrev.com/304da28bc0097013f9477ed616a9fc3f651644d3/test/mjsunit/compiler/lazy-iife-no-parens.js

Labels: -merge-Approved-4.6
Is there a way to find out which version of chrome this landed in?
Thanks!
Labels: Priority-2
Sign in to add a comment