Oilpan: Move synchronous Oilpan GCs out of V8 GC epilogues

Oilpan: Move synchronous Oilpan GCs out of V8 GC epilogues

It is problematic to run an Oilpan GC in a V8 GC epilogue because
it makes the time consumed by a V8 GC unpredictable.
This CL moves an Oilpan GC out of the V8 GC epilogue.
Alternately the Oilpan GC is scheduled at the end of the event loop (if necessary).

I verified that this CL doesn't cause a peak memory increase in memory.top_7_stress.

BUG=582831
Committed: https://crrev.com/9bdb6bc2d91c210e23f3bde2401420df28c13641
Cr-Commit-Position: refs/heads/master@{#372899}

[haraken, 2016-02-01 14:20:10]

haraken@chromium.org changed reviewers:
+ jochen@chromium.org, oilpan-reviews@chromium.org, sigbjornf@opera.com

[haraken, 2016-02-01 14:20:11]

PTAL

Here is a result for memory.top_7_stress. No regression in the peak memory
usage:

http://haraken.info/a/results.html

[jochen (gone - plz use gerrit), 2016-02-01 14:25:35]

lgtm

[sof, 2016-02-01 14:41:48]

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

https://codereview.chromium.org/1659453002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:347: if
(ThreadState::current())
What does the switch to scheduling via kGCTypeMarkSweepCompact instead of
kGCTypeProcessWeakCallbacks accomplish?

https://codereview.chromium.org/1659453002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1659453002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:600: if ((gcType ==
BlinkGC::V8MajorGC && shouldForceMemoryPressureGC())
Won't this regress on some tests that do run into memory pressure?

[haraken, 2016-02-01 15:05:58]

Yeah, this CL needs more explanation...

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

https://codereview.chromium.org/1659453002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:347: if
(ThreadState::current())
On 2016/02/01 14:41:48, sof wrote:
> What does the switch to scheduling via kGCTypeMarkSweepCompact instead of
> kGCTypeProcessWeakCallbacks accomplish?

V8 is now making a change not to invoke kGCTypeProcessWeakCallbacks if there is
no second-pass callback (in common cases there is no second-pass callback with
oilpan enabled). So we cannot rely on kGCTypeProcessWeakCallbacks any more.

https://codereview.chromium.org/1659453002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1659453002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:600: if ((gcType ==
BlinkGC::V8MajorGC && shouldForceMemoryPressureGC())
On 2016/02/01 14:41:48, sof wrote:
> Won't this regress on some tests that do run into memory pressure?

It may regress, but I think it isn't as terrible as you expect.

- V8 calls kGCTypeMarkSweepCompact's epilogue when it finishes the major GC.
Then V8 posts a new task to run kGCTypeProcessWeakCallbacks. (If V8 is under
memory pressure, the kGCTypeProcessWeakCallbacks is scheduled immediately after
the kGCTypeMarkSweepCompact's epilogue.)

- This CL moves scheduleV8FollowupGCIfNeeded from kGCTypeProcessWeakCallbacks'
epilogue to kGCTypeMarkSweepCompact's epilogue and then schedules a precise GC
at the end of the event loop.

- This means that before this CL, the precise GC doesn't run until the posted
task is scheduled, but after this CL, the precise GC is scheduled at the end of
the event loop. This is an improvement (i.e., the precise GC is scheduled
earlier).

- On the other hand, this CL stops running a conservative GC under memory
pressure. This is a regression but the memory pressure GC will anyway be
triggered when the next out-of-line allocation takes place.

- Overall, I guess this change isn't as terrible as you expect.

[sof, 2016-02-01 21:56:37]

Thanks for the details on the underlying shift in approach & GC order.

I verified that no OOMs were encountered with the usual suspects on Win32 with
this CL in effect; lgtm.

[haraken, 2016-02-02 01:40:36]

On 2016/02/01 21:56:37, sof wrote:
> Thanks for the details on the underlying shift in approach & GC order.
> 
> I verified that no OOMs were encountered with the usual suspects on Win32 with
> this CL in effect; lgtm.

Thanks for the verification!

[haraken, 2016-02-02 01:40:44]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-02-02 01:42:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1659453002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1659453002/1

[commit-bot: I haz the power, 2016-02-02 01:48:26]

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

[commit-bot: I haz the power, 2016-02-02 01:48:27]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[haraken, 2016-02-02 02:19:01]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-02-02 02:20:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1659453002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1659453002/1

[commit-bot: I haz the power, 2016-02-02 06:00:18]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-02-02 06:02:50]

Description was changed from

==========
Oilpan: Move synchronous Oilpan GCs out of V8 GC epilogues

It is problematic to run an Oilpan GC in a V8 GC epilogue because
it makes the time consumed by a V8 GC unpredictable.
This CL moves an Oilpan GC out of the V8 GC epilogue.
Alternately the Oilpan GC is scheduled at the end of the event loop (if
necessary).

I verified that this CL doesn't cause a peak memory increase in
memory.top_7_stress.

BUG=582831
==========

to

==========
Oilpan: Move synchronous Oilpan GCs out of V8 GC epilogues

It is problematic to run an Oilpan GC in a V8 GC epilogue because
it makes the time consumed by a V8 GC unpredictable.
This CL moves an Oilpan GC out of the V8 GC epilogue.
Alternately the Oilpan GC is scheduled at the end of the event loop (if
necessary).

I verified that this CL doesn't cause a peak memory increase in
memory.top_7_stress.

BUG=582831

Committed: https://crrev.com/9bdb6bc2d91c210e23f3bde2401420df28c13641
Cr-Commit-Position: refs/heads/master@{#372899}
==========

[commit-bot: I haz the power, 2016-02-02 06:02:52]

Patchset 1 (id:??) landed as
https://crrev.com/9bdb6bc2d91c210e23f3bde2401420df28c13641
Cr-Commit-Position: refs/heads/master@{#372899}