Low memory page navigation GC for low end devices

Low memory page navigation GC for low end devices

Until now we have avoided running GCs during page loads but for low end devices in low memory situations we should do a page navigation GC to reduce peak memory usage. system_health.memory_mobile showed that we can expect >10MB peak RSS reduction on certain sites.

Design doc: https://docs.google.com/document/d/13f25LGfpWWOOWk9U44it9Gva6i4JRsrQDlnxAJeSEZo/edit?usp=sharing

BUG=732664
Review-Url: https://codereview.chromium.org/2948983002
Cr-Commit-Position: refs/heads/master@{#482556}
Committed: https://chromium.googlesource.com/chromium/src/+/0787b78381f3abb74a078bde05a6084bf0ed8d2c

[keishi, 2017-06-22 04:51:18]

Description was changed from

==========
Page navigation GC for low end devices

BUG=
==========

to

==========
Low memory page navigation GC for low end devices

Until now we have avoided running GCs during page loads but for low end devices
in low memory situations we should do a page navigation GC to reduce peak memory
usage. system_health.memory_mobile showed that we can expect >10MB peak RSS
reduction on certain sites.

BUG=
==========

[keishi, 2017-06-22 05:19:26]

Description was changed from

==========
Low memory page navigation GC for low end devices

Until now we have avoided running GCs during page loads but for low end devices
in low memory situations we should do a page navigation GC to reduce peak memory
usage. system_health.memory_mobile showed that we can expect >10MB peak RSS
reduction on certain sites.

BUG=
==========

to

==========
Low memory page navigation GC for low end devices

Until now we have avoided running GCs during page loads but for low end devices
in low memory situations we should do a page navigation GC to reduce peak memory
usage. system_health.memory_mobile showed that we can expect >10MB peak RSS
reduction on certain sites.

Design doc:
https://docs.google.com/document/d/13f25LGfpWWOOWk9U44it9Gva6i4JRsrQDlnxAJeSE...

BUG=732664
==========

[keishi, 2017-06-22 05:46:22]

keishi@chromium.org changed reviewers:
+ haraken@chromium.org

[keishi, 2017-06-22 05:46:22]

PTAL

Draft design doc (with comments):
https://docs.google.com/document/d/1vJKPuMbJ5CQiFwrkxvTrB-x25SKF22-ui42oExvwp...
Public design doc in issue description

To trigger the GC I used MemoryPressureNotification(kCritical) as hpayer told me
to in this comment:
https://docs.google.com/a/google.com/document/d/1vJKPuMbJ5CQiFwrkxvTrB-x25SKF...

yfriedman pointed out that I should avoid a GC when the page navigation will
swtch to a new render process:
https://docs.google.com/a/google.com/document/d/1vJKPuMbJ5CQiFwrkxvTrB-x25SKF...

I've looked at the call sites (https://yuml.me/477ac141.pdf) and it looks like
NotifyContextDisposed can be called when:
1. committing a new document
2. swapping local/remote frames
3. closing a frame

I've written this CL so we only do a GC when reusing the frame (case 1). For
cases 2 and 3, on Android, the renderer process will be killed soon so it is not
worth doing a GC.

FYI I've rerun sytem_health.mobile_memory with this CL just in case, and it
shows the same memory reductions as I wrote in the design doc.
Example:
https://00e9e64bac50e3fbfe4b1c010ba84c54fc919273485e4c8f2c-apidata.googleuser...

[keishi, 2017-06-23 03:17:31]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 03:17:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-06-23 04:17:01]

LGTM

https://codereview.chromium.org/2948983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.h (right):

https://codereview.chromium.org/2948983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.h:77: void
DisposeContext(Lifecycle next_status, bool reusing_frame) override;

Can we introduce an enum?

https://codereview.chromium.org/2948983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp
(right):

https://codereview.chromium.org/2948983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp:65:
MemoryCoordinator::IsCurrentlyLowMemory()) {

Add a comment and explain what this is doing.

https://codereview.chromium.org/2948983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp:68:
v8::MemoryPressureLevel::kCritical);

Does this force an Oilpan conservative GC inside the MemoryPressureNotification
call? Or a precise GC is scheduled at the end of the event loop?

https://codereview.chromium.org/2948983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp (right):

https://codereview.chromium.org/2948983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp:32:
entry.value->ClearForNavigation();

ClearForSwap?

[commit-bot: I haz the power, 2017-06-23 05:31:35]

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

[commit-bot: I haz the power, 2017-06-23 05:31:36]

Dry run: This issue passed the CQ dry run.

[keishi, 2017-06-23 06:25:03]

https://codereview.chromium.org/2948983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.h (right):

https://codereview.chromium.org/2948983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.h:77: void
DisposeContext(Lifecycle next_status, bool reusing_frame) override;
On 2017/06/23 04:17:00, haraken wrote:
> 
> Can we introduce an enum?

Done.

https://codereview.chromium.org/2948983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp
(right):

https://codereview.chromium.org/2948983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp:65:
MemoryCoordinator::IsCurrentlyLowMemory()) {
On 2017/06/23 04:17:00, haraken wrote:
> 
> Add a comment and explain what this is doing.

Done.

https://codereview.chromium.org/2948983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp:68:
v8::MemoryPressureLevel::kCritical);
On 2017/06/23 04:17:00, haraken wrote:
> 
> Does this force an Oilpan conservative GC inside the
MemoryPressureNotification
> call? Or a precise GC is scheduled at the end of the event loop?

V8GController will run a conservative ForcedGC synchronously and also schedule a
Precise GC.

https://codereview.chromium.org/2948983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp (right):

https://codereview.chromium.org/2948983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp:32:
entry.value->ClearForNavigation();
On 2017/06/23 04:17:00, haraken wrote:
> 
> ClearForSwap?

Done.

[keishi, 2017-06-23 06:25:25]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 06:25:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-06-23 06:25:51]

LGTM

[keishi, 2017-06-23 06:28:51]

keishi@chromium.org changed reviewers:
+ isherman@chromium.org

[keishi, 2017-06-23 06:28:52]

+isherman Could you review the change to tools/metrics/histograms/histograms.xml
? Thanks.

[commit-bot: I haz the power, 2017-06-23 08:25:32]

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

[commit-bot: I haz the power, 2017-06-23 08:25:33]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-06-26 18:48:37]

https://codereview.chromium.org/2948983002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp
(right):

https://codereview.chromium.org/2948983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp:79:
("LowMemoryPageNavigationGC.Reduction", 0, 512, 50));
nit: 0 is not a valid histogram min; please use 1 instead.  (Also, I believe
there's a DCHECK enforcing this, so please double-check that you're building
with DCHECKs enabled.)

https://codereview.chromium.org/2948983002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2948983002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27421: +<histogram
name="LowMemoryPageNavigationGC.Reduction" units="MB">
It looks like you're adding a new top-level group name,
"LowMemoryPageNavigationGC".  Could you please use one of the existing group
names as well; or create a more general top-level group if you think a new one
is really most appropriate?

https://codereview.chromium.org/2948983002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27425: +    the low memory page
navigation GC.
It would be great to document this feature a wee bit more here: Is this a GC
that occurs following a page navigation?  Only on some types of devices?  Only
when memory is low?  I think just splitting this sentence into two or three, and
then filling in a few blanks, will help quite a lot.

[keishi, 2017-06-27 03:44:53]

https://codereview.chromium.org/2948983002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp
(right):

https://codereview.chromium.org/2948983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp:79:
("LowMemoryPageNavigationGC.Reduction", 0, 512, 50));
On 2017/06/26 18:48:37, Ilya Sherman wrote:
> nit: 0 is not a valid histogram min; please use 1 instead.  (Also, I believe
> there's a DCHECK enforcing this, so please double-check that you're building
> with DCHECKs enabled.)

Done.

FYI looks like we don't have a DCHECK yet
https://cs.chromium.org/chromium/src/base/metrics/histogram.cc?rcl=942e74b0d7...
. We have many places where min=0.

https://codereview.chromium.org/2948983002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2948983002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27421: +<histogram
name="LowMemoryPageNavigationGC.Reduction" units="MB">
On 2017/06/26 18:48:37, Ilya Sherman wrote:
> It looks like you're adding a new top-level group name,
> "LowMemoryPageNavigationGC".  Could you please use one of the existing group
> names as well; or create a more general top-level group if you think a new one
> is really most appropriate?

Done. Used BlinkGC top level group.

https://codereview.chromium.org/2948983002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27425: +    the low memory page
navigation GC.
On 2017/06/26 18:48:37, Ilya Sherman wrote:
> It would be great to document this feature a wee bit more here: Is this a GC
> that occurs following a page navigation?  Only on some types of devices?  Only
> when memory is low?  I think just splitting this sentence into two or three,
and
> then filling in a few blanks, will help quite a lot.

Done.

[Ilya Sherman, 2017-06-27 03:48:44]

Metrics LGTM % nits.  Thanks!

https://codereview.chromium.org/2948983002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp
(right):

https://codereview.chromium.org/2948983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp:79:
("LowMemoryPageNavigationGC.Reduction", 0, 512, 50));
On 2017/06/27 03:44:53, keishi wrote:
> On 2017/06/26 18:48:37, Ilya Sherman wrote:
> > nit: 0 is not a valid histogram min; please use 1 instead.  (Also, I believe
> > there's a DCHECK enforcing this, so please double-check that you're building
> > with DCHECKs enabled.)
> 
> Done.
> 
> FYI looks like we don't have a DCHECK yet
>
https://cs.chromium.org/chromium/src/base/metrics/histogram.cc?rcl=942e74b0d7...
> . We have many places where min=0.

Ah, I guess I misremembered.  Thanks for pointing that out!

https://codereview.chromium.org/2948983002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp
(right):

https://codereview.chromium.org/2948983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp:79:
("LowMemoryPageNavigationGC.Reduction", 1, 512, 50));
nit: Please update the histogram name here too.

[keishi, 2017-06-27 04:30:27]

Thanks!

https://codereview.chromium.org/2948983002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp
(right):

https://codereview.chromium.org/2948983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCForContextDispose.cpp:79:
("LowMemoryPageNavigationGC.Reduction", 1, 512, 50));
On 2017/06/27 03:48:44, Ilya Sherman wrote:
> nit: Please update the histogram name here too.

Done.

[keishi, 2017-06-27 04:30:37]

The CQ bit was checked by keishi@chromium.org

[keishi, 2017-06-27 04:30:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2948983002/#ps80001
(title: "fix")

[commit-bot: I haz the power, 2017-06-27 04:30:46]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 07:03:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498537837148490,
"parent_rev": "13e5cf2934637659dc1ad32d249e7dc8d76180cf", "commit_rev":
"0787b78381f3abb74a078bde05a6084bf0ed8d2c"}

[commit-bot: I haz the power, 2017-06-27 07:03:56]

Description was changed from

==========
Low memory page navigation GC for low end devices

Until now we have avoided running GCs during page loads but for low end devices
in low memory situations we should do a page navigation GC to reduce peak memory
usage. system_health.memory_mobile showed that we can expect >10MB peak RSS
reduction on certain sites.

Design doc:
https://docs.google.com/document/d/13f25LGfpWWOOWk9U44it9Gva6i4JRsrQDlnxAJeSE...

BUG=732664
==========

to

==========
Low memory page navigation GC for low end devices

Until now we have avoided running GCs during page loads but for low end devices
in low memory situations we should do a page navigation GC to reduce peak memory
usage. system_health.memory_mobile showed that we can expect >10MB peak RSS
reduction on certain sites.

Design doc:
https://docs.google.com/document/d/13f25LGfpWWOOWk9U44it9Gva6i4JRsrQDlnxAJeSE...

BUG=732664

Review-Url: https://codereview.chromium.org/2948983002
Cr-Commit-Position: refs/heads/master@{#482556}
Committed:
https://chromium.googlesource.com/chromium/src/+/0787b78381f3abb74a078bde05a6...
==========

[commit-bot: I haz the power, 2017-06-27 07:03:57]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/0787b78381f3abb74a078bde05a6...