Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2233683002:
Add kGCCallbackFlagCollectAllExternalMemory for external memory limit triggered gc (Closed)
DescriptionAdd kGCCallbackFlagCollectAllExternalMemory for external memory limit triggered gc
BUG=chromium:570268, chromium:621829
Committed: https://crrev.com/00c49a3e855793278d8ca6920ac98e9c922b966c
Cr-Commit-Position: refs/heads/master@{#38808}
Patch Set 1 #
Total comments: 2
Patch Set 2 : just add flag #Patch Set 3 : added cast #Messages
Total messages: 27 (13 generated)
Description was changed from ========== Reduce external_memory_limit when external_memory reduces BUG= ========== to ========== Reduce external_memory_limit when external_memory reduces BUG=570268 ==========
keishi@chromium.org changed reviewers: + haraken@chromium.org
This CL is a fix for b) 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.
mlippautz@chromium.org changed reviewers: + mlippautz@chromium.org
Can you add the problems to the design doc at https://docs.google.com/document/d/1moS2JJ2CXUWQ5kFgOBlIbtRIo202NjFnNWqB9ewfN... (Just request permissions.) https://codereview.chromium.org/2233683002/diff/1/include/v8.h File include/v8.h (right): https://codereview.chromium.org/2233683002/diff/1/include/v8.h#newcode8802 include/v8.h:8802: if (amount + 192 * 1024 * 1024 < *external_memory_limit) { Can we avoid polluting the API and instead adjust the external_memory_limit from within V8? The fast check here should merely be a comparison of a counter against a limit.
The change looks good to me if the V8 team likes it.
hpayer@chromium.org changed reviewers: + hpayer@chromium.org
https://codereview.chromium.org/2233683002/diff/1/include/v8.h File include/v8.h (right): https://codereview.chromium.org/2233683002/diff/1/include/v8.h#newcode8803 include/v8.h:8803: *external_memory_limit = amount + 192 * 1024 * 1024; Why are you shrinking the limit here?
On 2016/08/12 14:06:09, Hannes Payer wrote: > https://codereview.chromium.org/2233683002/diff/1/include/v8.h > File include/v8.h (right): > > https://codereview.chromium.org/2233683002/diff/1/include/v8.h#newcode8803 > include/v8.h:8803: *external_memory_limit = amount + 192 * 1024 * 1024; > Why are you shrinking the limit here? keishi@'s comment on #3 is helpful?
keishi: Can you change the CL to just include the flag changes but hold off on the memory limit change? We have some ideas on how to do this in a more dynamic way but in order to make progress it would be good to have the flag in the API and the chromium side ready. (Basically a variant of https://codereview.chromium.org/2256853003/ with the new flag added to the GC that is triggered on hitting the hard limit). Also add chromium:621829 to description.
Description was changed from ========== Reduce external_memory_limit when external_memory reduces BUG=570268 ========== to ========== Add kGCCallbackFlagCollectAllExternalMemory for external memory limit triggered gc BUG=570268 ==========
Changed to flag addition only
Description was changed from ========== Add kGCCallbackFlagCollectAllExternalMemory for external memory limit triggered gc BUG=570268 ========== to ========== Add kGCCallbackFlagCollectAllExternalMemory for external memory limit triggered gc BUG=570268,chromium:621829 ==========
lgtm, thanks also added the v8-related tracking bug to the descripton
LGTM
Description was changed from ========== Add kGCCallbackFlagCollectAllExternalMemory for external memory limit triggered gc BUG=570268,chromium:621829 ========== to ========== Add kGCCallbackFlagCollectAllExternalMemory for external memory limit triggered gc BUG=chromium:570268,chromium:621829 ==========
The CQ bit was checked by keishi@chromium.org
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: v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/11327)
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, mlippautz@chromium.org Link to the patchset: https://codereview.chromium.org/2233683002/#ps40001 (title: "added cast")
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 kGCCallbackFlagCollectAllExternalMemory for external memory limit triggered gc BUG=chromium:570268,chromium:621829 ========== to ========== Add kGCCallbackFlagCollectAllExternalMemory for external memory limit triggered gc BUG=chromium:570268,chromium:621829 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add kGCCallbackFlagCollectAllExternalMemory for external memory limit triggered gc BUG=chromium:570268,chromium:621829 ========== to ========== Add kGCCallbackFlagCollectAllExternalMemory for external memory limit triggered gc BUG=chromium:570268,chromium:621829 Committed: https://crrev.com/00c49a3e855793278d8ca6920ac98e9c922b966c Cr-Commit-Position: refs/heads/master@{#38808} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/00c49a3e855793278d8ca6920ac98e9c922b966c Cr-Commit-Position: refs/heads/master@{#38808} |
