Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2230133002:
Add ImageResource data size to V8 external memory allocation (Closed)
|
Created:
4 years, 4 months ago by keishi Modified:
4 years, 4 months ago Reviewers:
haraken CC:
chromium-reviews, tyoshino+watch_chromium.org, Yoav Weiss, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, Nate Chapin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ImageResource data size to V8 external memory allocation
BUG=570268
Committed: https://crrev.com/62c31e6a87a45ce1d0b58c1f40af531497ea9b3e
Cr-Commit-Position: refs/heads/master@{#412253}
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix #Patch Set 3 : fix #Messages
Total messages: 20 (11 generated)
Description was changed from ========== Add ImageResource data size to V8 external memory allocation BUG= ========== to ========== Add ImageResource data size to V8 external memory allocation BUG=570268 ==========
keishi@chromium.org changed reviewers: + haraken@chromium.org
This CL is a fix for a) 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.
LGTM
https://codereview.chromium.org/2230133002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/2230133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ImageResource.cpp:43: #include <v8/include/v8.h> We normally include <v8.h>
https://codereview.chromium.org/2230133002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/2230133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ImageResource.cpp:43: #include <v8/include/v8.h> On 2016/08/12 07:05:09, haraken wrote: > > We normally include <v8.h> Done.
The CQ bit was checked by keishi@chromium.org
The CQ bit was unchecked by keishi@chromium.org
The CQ bit was checked by keishi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2230133002/#ps20001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by keishi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2230133002/#ps40001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add ImageResource data size to V8 external memory allocation BUG=570268 ========== to ========== Add ImageResource data size to V8 external memory allocation BUG=570268 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add ImageResource data size to V8 external memory allocation BUG=570268 ========== to ========== Add ImageResource data size to V8 external memory allocation BUG=570268 Committed: https://crrev.com/62c31e6a87a45ce1d0b58c1f40af531497ea9b3e Cr-Commit-Position: refs/heads/master@{#412253} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/62c31e6a87a45ce1d0b58c1f40af531497ea9b3e Cr-Commit-Position: refs/heads/master@{#412253} |
