Trigger Blink GC on v8::kGCCallbackFlagCollectAllExternalMemory

Trigger Blink GC on v8::kGCCallbackFlagCollectAllExternalMemory

BUG=570268
Committed: https://crrev.com/c987ef551cd26d57e53bb2d592d4b75ed3048ac8
Cr-Commit-Position: refs/heads/master@{#414400}

[keishi, 2016-08-12 06:58:50]

keishi@chromium.org changed reviewers:
+ haraken@chromium.org

[keishi, 2016-08-12 06:58:50]

This CL is a fix for c)

Explanation:

After investigating the memory leak for the html below (crbug.com/570268) I saw
three problems.
https://dl.dropboxusercontent.com/u/15217362/bloburl/bitmaptest.html

a) AdjustAmountOfExternalMemory is not called for ImageResource.
b) external_memory_limit increases too much because it is adjusted after a v8
gc, not blink gc.
c) V8Followup blinkgc doesn't always run after a "external memory induced V8 GC"
because ImageResource data is allocated in malloc and blinkgc doesn't know about
malloc.

I confirmed that we can reduce peak memory usage by 500MB (after 20 seconds of
running the html) if these three problems are eliminated.

The fix for a) is https://codereview.chromium.org/2230133002/
To keep the change simple and avoid issues like double counting memory this only
reports the ImageResource data size to v8.

The fix for c) is https://codereview.chromium.org/2235653002/ and
https://codereview.chromium.org/2238173002/
This adds a new callback flag so Blink can be extra careful to free the external
allocations.

external_memory_limit is set to (external_memory + 192MB) after a V8 GC. When
external_memory becomes greater than external_memory_limit a V8 GC is triggered.
external_memory_limit increases too much because the external_memory hasn't
shrunk yet.
For example, for this html, i observed that the Image object requires two GC
cycles for it to be collected.
I'm not sure why but I believe the fact that the onload event handler references
the Image object makes the V8 Image object uncollectable until after a Blink GC.
The ImageResource data is not freed until a BlinkGC happens after that.
The significant delay from when the V8 GC is triggered by external memory alloc,
and when the external memory is a problem but I don't think we can fix that.
Instead I propose we reduce external_memory_limit when external_memory reduces
https://codereview.chromium.org/2233683002/
This will make it harder for the external_memory_limit to just keep on
increasing, and I'm hoping it will have minimal negative impact.

[haraken, 2016-08-12 07:08:49]

https://codereview.chromium.org/2238173002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/2238173002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:391: if (flags &
v8::kGCCallbackFlagCollectAllExternalMemory)

Do you think it's worth distinguishing kGCCallbackFlagCollectAllExternalMemory
from kGCCallbackFlagCollectAllAvailableGarbage?

[keishi, 2016-08-15 10:30:44]

https://codereview.chromium.org/2238173002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/2238173002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:391: if (flags &
v8::kGCCallbackFlagCollectAllExternalMemory)
On 2016/08/12 07:08:48, haraken wrote:
> 
> Do you think it's worth distinguishing kGCCallbackFlagCollectAllExternalMemory
> from kGCCallbackFlagCollectAllAvailableGarbage?

I'm not sure. I was thinking of maybe calling MemoryCache::purgeAll
kGCCallbackFlagCollectAllExternalMemory in the future and
v8::Heap::CollectAllAvailableGarbage seems to be called from many places.

[haraken, 2016-08-15 14:22:44]

On 2016/08/15 10:30:44, keishi wrote:
>
https://codereview.chromium.org/2238173002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):
> 
>
https://codereview.chromium.org/2238173002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:391: if (flags &
> v8::kGCCallbackFlagCollectAllExternalMemory)
> On 2016/08/12 07:08:48, haraken wrote:
> > 
> > Do you think it's worth distinguishing
kGCCallbackFlagCollectAllExternalMemory
> > from kGCCallbackFlagCollectAllAvailableGarbage?
> 
> I'm not sure. I was thinking of maybe calling MemoryCache::purgeAll
> kGCCallbackFlagCollectAllExternalMemory in the future and
> v8::Heap::CollectAllAvailableGarbage seems to be called from many places.

As far as I understand, V8 specifies CollectAllAvailableGarbage when the system
is under high memory pressure. So I thought it would make more sense to just use
CollectAllAvailableGarbage when the external memory exceeds some threshold.
(Also it would make more sense to call purgeAll when CollectAllAvailableGarbage
is specified.)

hpayer@, ulan@: Do you have any thoughts?

[keishi, 2016-08-25 05:13:39]

The conservative GC is required but I'm not sure if we need the PreciseGC for
kGCCallbackFlagCollectAllExternalMemory. But I think we should be safe and start
with both to confirm that the v8 side change is working, and adjust later.

[haraken, 2016-08-25 05:22:29]

https://codereview.chromium.org/2238173002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/2238173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:380: && flags &
v8::kGCCallbackFlagCollectAllExternalMemory) {

Add () to make the condition clearer.

Shouldn't it be ||, not &&?

[keishi, 2016-08-25 05:25:05]

https://codereview.chromium.org/2238173002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/2238173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:380: && flags &
v8::kGCCallbackFlagCollectAllExternalMemory) {
On 2016/08/25 05:22:29, haraken wrote:
> 
> Add () to make the condition clearer.
> 
> Shouldn't it be ||, not &&?

oops. Done

[haraken, 2016-08-25 05:25:48]

LGTM

[keishi, 2016-08-25 06:07:42]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2016-08-25 06:08:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 08:15:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 08:15:20]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[keishi, 2016-08-25 09:07:09]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2016-08-25 09:07:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 12:07:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-25 12:10:22]

Description was changed from

==========
Trigger Blink GC on v8::kGCCallbackFlagCollectAllExternalMemory

BUG=570268
==========

to

==========
Trigger Blink GC on v8::kGCCallbackFlagCollectAllExternalMemory

BUG=570268

Committed: https://crrev.com/c987ef551cd26d57e53bb2d592d4b75ed3048ac8
Cr-Commit-Position: refs/heads/master@{#414400}
==========

[commit-bot: I haz the power, 2016-08-25 12:10:23]

Patchset 3 (id:??) landed as
https://crrev.com/c987ef551cd26d57e53bb2d592d4b75ed3048ac8
Cr-Commit-Position: refs/heads/master@{#414400}