Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(594)

Issue 1190513006: Oilpan: adjust GC policy under memory pressure (only.) (Closed)

Created:
5 years, 6 months ago by sof
Modified:
5 years, 6 months ago
Reviewers:
oilpan-reviews, haraken
CC:
blink-reviews, arv+blink, oilpan-reviews, Mads Ager (chromium), vivekg_samsung, vivekg, blink-reviews-bindings_chromium.org, haraken, kouhei+heap_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Oilpan: adjust GC policy under memory pressure (only.) When notified of a v8 major GC, check if we're under memory pressure and overdue an Oilpan GC & urgently. If so, force a conservative GC right away. This addresses dom-modify.html OOM troubles on win32, in particular. It runs into the allocation pattern where the Oilpan allocator is able to handle a tight loop just allocating Text nodes by bump allocating each such node. Each Text node has a large external string allocation however, which leads to a steep ramp-upin overall allocation amounts to the point where it might OOM as no Oilpan GCs will run. v8 will however notice the increasing memory pressure and schedule major GCs of its own. Oilpan is notified after each v8 major GC, so use that opportunity to check for memory pressure & force an Oilpan GC right away. R=haraken BUG=474470 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197245

Patch Set 1 #

Total comments: 3

Patch Set 2 : remove testing CPPery #

Unified diffs Side-by-side diffs Delta from patch set Stats (+54 lines, -25 lines) Patch
M Source/bindings/core/v8/V8GCController.cpp View 1 chunk +9 lines, -5 lines 0 comments Download
M Source/platform/heap/ThreadState.h View 2 chunks +7 lines, -2 lines 0 comments Download
M Source/platform/heap/ThreadState.cpp View 1 6 chunks +38 lines, -18 lines 0 comments Download

Messages

Total messages: 18 (8 generated)
sof
please take a look. This is another take on https://codereview.chromium.org/1174123002/ (and https://codereview.chromium.org/1178063004/ ), following the ...
5 years, 6 months ago (2015-06-16 10:56:23 UTC) #2
haraken
LGTM, thanks for taking care of this! https://codereview.chromium.org/1190513006/diff/1/Source/platform/heap/ThreadState.cpp File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1190513006/diff/1/Source/platform/heap/ThreadState.cpp#newcode833 Source/platform/heap/ThreadState.cpp:833: } Actually ...
5 years, 6 months ago (2015-06-16 16:21:27 UTC) #4
sof
https://codereview.chromium.org/1190513006/diff/1/Source/platform/heap/ThreadState.cpp File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1190513006/diff/1/Source/platform/heap/ThreadState.cpp#newcode833 Source/platform/heap/ThreadState.cpp:833: } On 2015/06/16 16:21:27, haraken wrote: > > Actually ...
5 years, 6 months ago (2015-06-16 16:27:32 UTC) #5
haraken
https://codereview.chromium.org/1190513006/diff/1/Source/platform/heap/ThreadState.cpp File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1190513006/diff/1/Source/platform/heap/ThreadState.cpp#newcode833 Source/platform/heap/ThreadState.cpp:833: } On 2015/06/16 16:27:31, sof wrote: > On 2015/06/16 ...
5 years, 6 months ago (2015-06-16 16:36:44 UTC) #6
sof
On 2015/06/16 16:36:44, haraken wrote: > https://codereview.chromium.org/1190513006/diff/1/Source/platform/heap/ThreadState.cpp > File Source/platform/heap/ThreadState.cpp (right): > > https://codereview.chromium.org/1190513006/diff/1/Source/platform/heap/ThreadState.cpp#newcode833 > ...
5 years, 6 months ago (2015-06-16 21:22:39 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1190513006/20001
5 years, 6 months ago (2015-06-16 21:23:02 UTC) #10
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/66895)
5 years, 6 months ago (2015-06-17 01:47:50 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1190513006/20001
5 years, 6 months ago (2015-06-17 05:34:46 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1190513006/20001
5 years, 6 months ago (2015-06-17 07:22:50 UTC) #17
commit-bot: I haz the power
5 years, 6 months ago (2015-06-17 08:59:16 UTC) #18
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197245

Powered by Google App Engine
This is Rietveld 408576698