Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2630453004:
Increase heap limit for DevTools isolate. (Closed)
|
Created:
3 years, 11 months ago by ulan Modified:
3 years, 11 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, kinuko+watch, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIncrease heap limit for DevTools isolate.
BUG=675911
Review-Url: https://codereview.chromium.org/2630453004
Cr-Commit-Position: refs/heads/master@{#443670}
Committed: https://chromium.googlesource.com/chromium/src/+/1a56e240a23b3d01c57a9bf39f9296a45eced88c
Patch Set 1 #
Total comments: 1
Patch Set 2 : fix comment #Messages
Total messages: 23 (8 generated)
ulan@chromium.org changed reviewers: + dgozman@chromium.org, kozyatinskiy@chromium.org
PTAL. Is this the right place for the function call? I tried to find the earliest possible point with available isolate after initialization of DevTools. I will follow-up with a CL that increases heap limit for workers spawned by DevTools.
alph@chromium.org changed reviewers: + alph@chromium.org
https://codereview.chromium.org/2630453004/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebDevToolsFrontendImpl.cpp (right): https://codereview.chromium.org/2630453004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebDevToolsFrontendImpl.cpp:70: isolate->IncreaseHeapLimitForDebugging(); I wonder why it helps. Most of the heap snapshot data (nodes and edges) are stored in external arrays. The only thing which is within the heap is strings. However they shouldn't occupy more space than they do in the inspected heap. Moreover, each snapshot is stored in a dedicated worker heap, so increasing main isolate heap should have no effect. Do you have an example page that OOMs? I'm curious to take a look..
On 2017/01/12 17:03:22, alph wrote: > https://codereview.chromium.org/2630453004/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/web/WebDevToolsFrontendImpl.cpp (right): > > https://codereview.chromium.org/2630453004/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/web/WebDevToolsFrontendImpl.cpp:70: > isolate->IncreaseHeapLimitForDebugging(); > I wonder why it helps. Most of the heap snapshot data (nodes and edges) are > stored in external arrays. The only thing which is within the heap is strings. > However they shouldn't occupy more space than they do in the inspected heap. > Moreover, each snapshot is stored in a dedicated worker heap, so increasing main > isolate heap should have no effect. > > Do you have an example page that OOMs? I'm curious to take a look.. You're right, most of the memory is allocated in workers, not in the main isolate. The main isolate uses only fraction of inspected tab's memory in my tests, which pretty cool. :) My plan was to increase the heap limit for the main isolate and then propagate the increased limit to workers so that all DevTools isolates have similar heap limits. Another reason for this CL would be "Record allocation timeline", which seems to allocate in the main isolate. You can check it by 1) loading https://ulan.github.io/misc/stress-heap.html 2) starting "Record Allocation Timeline". 3) clicking "Generate" in the inspected tab. 4) stop "Record Allocation Timeline" and check the main isolate heap. I corrected the comment from "heap snapshot" to "heap profiling".
On 2017/01/12 18:51:49, ulan wrote: > On 2017/01/12 17:03:22, alph wrote: > > > https://codereview.chromium.org/2630453004/diff/1/third_party/WebKit/Source/w... > > File third_party/WebKit/Source/web/WebDevToolsFrontendImpl.cpp (right): > > > > > https://codereview.chromium.org/2630453004/diff/1/third_party/WebKit/Source/w... > > third_party/WebKit/Source/web/WebDevToolsFrontendImpl.cpp:70: > > isolate->IncreaseHeapLimitForDebugging(); > > I wonder why it helps. Most of the heap snapshot data (nodes and edges) are > > stored in external arrays. The only thing which is within the heap is strings. > > However they shouldn't occupy more space than they do in the inspected heap. > > Moreover, each snapshot is stored in a dedicated worker heap, so increasing > main > > isolate heap should have no effect. > > > > Do you have an example page that OOMs? I'm curious to take a look.. > > You're right, most of the memory is allocated in workers, not in the main > isolate. > The main isolate uses only fraction of inspected tab's memory in my tests, which > pretty cool. :) > > My plan was to increase the heap limit for the main isolate and then propagate > the increased limit to workers so that all DevTools isolates have similar heap > limits. > > Another reason for this CL would be "Record allocation timeline", which seems to > allocate in the main isolate. > > You can check it by > 1) loading https://ulan.github.io/misc/stress-heap.html > 2) starting "Record Allocation Timeline". > 3) clicking "Generate" in the inspected tab. > 4) stop "Record Allocation Timeline" and check the main isolate heap. > > I corrected the comment from "heap snapshot" to "heap profiling". I tried, but still can't repro. The page isolate with 50K objects allocated has a heap of 500MB. The heap profiler for that heap has 20MB is Main isolate and less than 10MB in a worker isolate plus 2GB in native typed arrays. Record allocation timeline should not be any different from the heap snapshot in that sense, as the only difference is that each time stamp it remembers the last object id that has been allocated. So this example does not prove we need to increase the heap size for DevTools.
On 2017/01/12 20:17:17, alph wrote: > I tried, but still can't repro. The page isolate with 50K objects allocated has > a heap of 500MB. > The heap profiler for that heap has 20MB is Main isolate and less than 10MB in a > worker isolate > plus 2GB in native typed arrays. > > Record allocation timeline should not be any different from the heap snapshot in > that sense, > as the only difference is that each time stamp it remembers the last object id > that has been allocated. > > So this example does not prove we need to increase the heap size for DevTools. I've a CL which introduces pause on OOM: https://codereview.chromium.org/2624543004/. If chrome is running with low memory limit (via --js-flags), then when on pause I'm trying to collect heap profile - I'll get OOM in DevTools process very often - I can show you repro on my machine.
> I tried, but still can't repro. The page isolate with 50K objects allocated has > a heap of 500MB. > The heap profiler for that heap has 20MB is Main isolate and less than 10MB in a > worker isolate > plus 2GB in native typed arrays. The peak memory usage of a worker is definitely more than 700MB. It drops significantly after loading edges. If you limit the heap to 700MB by using --js-flags="--max-old-space-size=700" command line flag then DevTools will OOM while taking the snapshot. I cannot reproduce the main isolate issue anymore. Sorry about that, it probably depends on sequence of steps I did besides recording allocation timeline (I took couple of heap snapshot and clicked around).
I think I know what's going on, see https://bugs.chromium.org/p/chromium/issues/detail?id=680789 It needs to be fixed. For now let's increase the heap size as a workaround. lgtm
The CQ bit was checked by ulan@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ulan@chromium.org changed reviewers: + haraken@chromium.org
+haraken@, for owners review.
lgtm
The CQ bit was checked by ulan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484336261329150,
"parent_rev": "bdf0f57baf2f4ae139df5f3f928fabca76a3d4c1", "commit_rev":
"1a56e240a23b3d01c57a9bf39f9296a45eced88c"}
Message was sent while issue was closed.
Description was changed from ========== Increase heap limit for DevTools isolate. BUG=675911 ========== to ========== Increase heap limit for DevTools isolate. BUG=675911 Review-Url: https://codereview.chromium.org/2630453004 Cr-Commit-Position: refs/heads/master@{#443670} Committed: https://chromium.googlesource.com/chromium/src/+/1a56e240a23b3d01c57a9bf39f92... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/1a56e240a23b3d01c57a9bf39f92...
Message was sent while issue was closed.
LGTM |
