Add memory pressure notification API

Add memory pressure notification API

Based on CL 1777883002.

BUG=chromium:590975
LOG=NO

Committed: https://crrev.com/bb9b74e75495ff0d00897760f5f13784e88930b0
Cr-Commit-Position: refs/heads/master@{#35053}

[ulan, 2016-03-17 19:21:38]

ulan@chromium.org changed reviewers:
+ hong.zheng@intel.com, jochen@chromium.org

[ulan, 2016-03-17 19:21:39]

PTAL. I will add tests tomorrow.

[ulan, 2016-03-17 19:22:07]

Description was changed from

==========
Add memory pressure notification API

BUG=chromium:590975
LOG=NO
==========

to

==========
Add memory pressure notification API

Based on CL 1777883002.

BUG=chromium:590975
LOG=NO
==========

[jochen (gone - plz use gerrit), 2016-03-17 19:33:39]

https://codereview.chromium.org/1813963002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1813963002/diff/1/include/v8.h#newcode5605
include/v8.h:5605: * the isolate is executing long running JavaScript code.
do we expect the embedder to set the pressure level to kNone if the memory
pressure ends?

https://codereview.chromium.org/1813963002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1813963002/diff/1/src/api.cc#newcode7592
src/api.cc:7592: static_cast<i::MemoryPressureLevel>(level),
Locker::IsLocked(this));
IsLocked() should never be true in blink, right?

https://codereview.chromium.org/1813963002/diff/1/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1813963002/diff/1/src/heap/heap.h#newcode415
src/heap/heap.h:415: enum class MemoryPressureLevel { kNone, kModerate,
kCritical };
why not just use the external class?

[hong.zheng, 2016-03-18 02:27:16]

https://codereview.chromium.org/1813963002/diff/1/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1813963002/diff/1/src/heap/heap.cc#newcode4428
src/heap/heap.cc:4428: void Heap::MemoryPressureNotification(MemoryPressureLevel
level,
On Android, LMK(low memory killer) and memory pressure notification are
different mechanisms. Because LMK is handled by kernel, there is no guarantee
that process can see kCritical notification before LMK kills it. So we had
better handle kModerate notification immediately. Of course, we can do part of
GC for kModerate level to reduce performance impact.

[ulan, 2016-03-18 12:49:41]

Thanks for comments. I added test.

https://codereview.chromium.org/1813963002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1813963002/diff/1/include/v8.h#newcode5605
include/v8.h:5605: * the isolate is executing long running JavaScript code.
On 2016/03/17 19:33:39, jochen wrote:
> do we expect the embedder to set the pressure level to kNone if the memory
> pressure ends?

That it nice to have, but we do not rely on that. After GC we set the level to
kNone ourselves. If the embedder sets to kNone proactively before GC, then it
will be good for performance.

https://codereview.chromium.org/1813963002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1813963002/diff/1/src/api.cc#newcode7592
src/api.cc:7592: static_cast<i::MemoryPressureLevel>(level),
Locker::IsLocked(this));
On 2016/03/17 19:33:39, jochen wrote:
> IsLocked() should never be true in blink, right?

If V8 is not executing JS code and there is memory pressure event in chrome,
then it would be optimal to enter the isolate and then call the function. That
way we can do GC immediately.  Otherwise, the notification will have no effect
until V8 starts running.

https://codereview.chromium.org/1813963002/diff/1/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1813963002/diff/1/src/heap/heap.cc#newcode4428
src/heap/heap.cc:4428: void Heap::MemoryPressureNotification(MemoryPressureLevel
level,
On 2016/03/18 02:27:16, hong.zheng wrote:
> On Android, LMK(low memory killer) and memory pressure notification are
> different mechanisms. Because LMK is handled by kernel, there is no guarantee
> that process can see kCritical notification before LMK kills it. So we had
> better handle kModerate notification immediately. Of course, we can do part of
> GC for kModerate level to reduce performance impact.

I didn't get it. Will blink send only kModerate memory pressure notifications?
Why can't it send kCritical notification when the memory pressure is very high?

If we do non-incremental GC on kModerate, then there is no difference between
kModerate and kCritical.

https://codereview.chromium.org/1813963002/diff/1/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1813963002/diff/1/src/heap/heap.h#newcode415
src/heap/heap.h:415: enum class MemoryPressureLevel { kNone, kModerate,
kCritical };
On 2016/03/17 19:33:39, jochen wrote:
> why not just use the external class?

Done.

[Hannes Payer (out of office), 2016-03-18 13:27:25]

hpayer@chromium.org changed reviewers:
+ hpayer@chromium.org

[Hannes Payer (out of office), 2016-03-18 13:27:26]

https://codereview.chromium.org/1813963002/diff/1/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1813963002/diff/1/src/heap/heap.cc#newcode4428
src/heap/heap.cc:4428: void Heap::MemoryPressureNotification(MemoryPressureLevel
level,
On 2016/03/18 12:49:40, ulan wrote:
> On 2016/03/18 02:27:16, hong.zheng wrote:
> > On Android, LMK(low memory killer) and memory pressure notification are
> > different mechanisms. Because LMK is handled by kernel, there is no
guarantee
> > that process can see kCritical notification before LMK kills it. So we had
> > better handle kModerate notification immediately. Of course, we can do part
of
> > GC for kModerate level to reduce performance impact.
> 
> I didn't get it. Will blink send only kModerate memory pressure notifications?
> Why can't it send kCritical notification when the memory pressure is very
high?
> 
> If we do non-incremental GC on kModerate, then there is no difference between
> kModerate and kCritical.

I am with Ulan. kModerate should still take latency into account. It should be
called when memory reaches a certain, not yet critical level where immediate
action is not required.

https://codereview.chromium.org/1813963002/diff/20001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1813963002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4435: CollectAllGarbage(
When the level is critical, one memory reducing GC may not be good enough to
free up all resources. Another option would be to call
CollectAllAvailableGarbage which is quite a latency killer. I guess it depends
on the meaning of "critical".

https://codereview.chromium.org/1813963002/diff/20001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1813963002/diff/20001/src/heap/heap.h#newcode...
src/heap/heap.h:2011: AtomicValue<MemoryPressureLevel> memory_pressure_level_;
For completeness, add a comment to describe that field.

[jochen (gone - plz use gerrit), 2016-03-18 15:29:12]

https://codereview.chromium.org/1813963002/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1813963002/diff/20001/src/api.cc#newcode7592
src/api.cc:7592: Locker::IsLocked(this));
as explained offline, Locker::IsLocked() always returns false in blink (as it
doesn't use lockers).

in general, we can't know whether the "main" thread is running JS currently
(without races), so I'd recommend to set the interrupt flag and internally
schedule a task using PostTaskOnForegroundThread() to make sure that the
interrupt flag actually gets honored.

[ulan, 2016-03-18 16:10:00]

https://codereview.chromium.org/1813963002/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1813963002/diff/20001/src/api.cc#newcode7592
src/api.cc:7592: Locker::IsLocked(this));
On 2016/03/18 15:29:12, jochen - slow wrote:
> as explained offline, Locker::IsLocked() always returns false in blink (as it
> doesn't use lockers).
> 
> in general, we can't know whether the "main" thread is running JS currently
> (without races), so I'd recommend to set the interrupt flag and internally
> schedule a task using PostTaskOnForegroundThread() to make sure that the
> interrupt flag actually gets honored.

Done.

https://codereview.chromium.org/1813963002/diff/20001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1813963002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4435: CollectAllGarbage(
On 2016/03/18 13:27:26, Hannes Payer - slow wrote:
> When the level is critical, one memory reducing GC may not be good enough to
> free up all resources. Another option would be to call
> CollectAllAvailableGarbage which is quite a latency killer. I guess it depends
> on the meaning of "critical".

We also restart memory reducer.

https://codereview.chromium.org/1813963002/diff/20001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1813963002/diff/20001/src/heap/heap.h#newcode...
src/heap/heap.h:2011: AtomicValue<MemoryPressureLevel> memory_pressure_level_;
On 2016/03/18 13:27:26, Hannes Payer - slow wrote:
> For completeness, add a comment to describe that field.

Done.

[Hannes Payer (out of office), 2016-03-21 09:58:26]

https://codereview.chromium.org/1813963002/diff/20001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1813963002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4435: CollectAllGarbage(
On 2016/03/18 16:10:00, ulan wrote:
> On 2016/03/18 13:27:26, Hannes Payer - slow wrote:
> > When the level is critical, one memory reducing GC may not be good enough to
> > free up all resources. Another option would be to call
> > CollectAllAvailableGarbage which is quite a latency killer. I guess it
depends
> > on the meaning of "critical".
> 
> We also restart memory reducer.

Depending on what critical means, starting the memory reducer may be too late
here. If critical means that we have to free up memory now otherwise we are
going to get killed we have to do clean up as much as possible.

For sure, that is a dangerous game. We do not necessarily want emergency GCs to
happen for no good reason.

[jochen (gone - plz use gerrit), 2016-03-21 10:01:56]

On 2016/03/21 at 09:58:26, hpayer wrote:
> https://codereview.chromium.org/1813963002/diff/20001/src/heap/heap.cc
> File src/heap/heap.cc (right):
> 
>
https://codereview.chromium.org/1813963002/diff/20001/src/heap/heap.cc#newcod...
> src/heap/heap.cc:4435: CollectAllGarbage(
> On 2016/03/18 16:10:00, ulan wrote:
> > On 2016/03/18 13:27:26, Hannes Payer - slow wrote:
> > > When the level is critical, one memory reducing GC may not be good enough
to
> > > free up all resources. Another option would be to call
> > > CollectAllAvailableGarbage which is quite a latency killer. I guess it
depends
> > > on the meaning of "critical".
> > 
> > We also restart memory reducer.
> 
> Depending on what critical means, starting the memory reducer may be too late
here. If critical means that we have to free up memory now otherwise we are
going to get killed we have to do clean up as much as possible.
> 
> For sure, that is a dangerous game. We do not necessarily want emergency GCs
to happen for no good reason.

http://developer.android.com/reference/android/content/ComponentCallbacks2.html
contains a description of what the levels mean.

Note that they have different meanings depending on whether we're the foreground
or background process.

[ulan, 2016-03-21 15:28:28]

Added more comments in v8.h

Made kModerate more aggressive with incremental GC: it starts earlier, makes
bigger and more frequent steps.

After GC with high memory pressure, we shrink and uncommit new space.

[jochen (gone - plz use gerrit), 2016-03-21 15:30:42]

lgtm

[jochen (gone - plz use gerrit), 2016-03-21 15:30:46]

lgtm

[ulan, 2016-03-21 15:35:23]

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

[commit-bot: I haz the power, 2016-03-21 15:35:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1813963002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1813963002/100001

[commit-bot: I haz the power, 2016-03-21 15:39:40]

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

[commit-bot: I haz the power, 2016-03-21 15:39:41]

Dry run: Try jobs failed on following builders:
  v8_win_compile_dbg on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/1...)

[ulan, 2016-03-22 09:42:48]

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

[commit-bot: I haz the power, 2016-03-22 09:42:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1813963002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1813963002/120001

[commit-bot: I haz the power, 2016-03-22 10:06:20]

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

[commit-bot: I haz the power, 2016-03-22 10:06:21]

Dry run: This issue passed the CQ dry run.

[hong.zheng, 2016-03-24 07:43:54]

lgtm. Thanks for your work

[ulan, 2016-03-24 09:26:29]

On 2016/03/24 07:43:54, hong.zheng wrote:
> lgtm. Thanks for your work

Hannes, I'll go ahead and land the CL since it is blocking Hong from landing
Chromium CLs.

The API interface is simple and shouldn't change. We can always change the
implementation in case you have comments.

[ulan, 2016-03-24 09:27:09]

The CQ bit was checked by ulan@chromium.org

[ulan, 2016-03-24 09:27:10]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1813963002/#ps160001
(title: "GC interrupt on kModerate to start incremental marking")

[commit-bot: I haz the power, 2016-03-24 09:27:21]

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

[commit-bot: I haz the power, 2016-03-24 09:51:53]

Description was changed from

==========
Add memory pressure notification API

Based on CL 1777883002.

BUG=chromium:590975
LOG=NO
==========

to

==========
Add memory pressure notification API

Based on CL 1777883002.

BUG=chromium:590975
LOG=NO
==========

[commit-bot: I haz the power, 2016-03-24 09:51:54]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-24 09:53:18]

Description was changed from

==========
Add memory pressure notification API

Based on CL 1777883002.

BUG=chromium:590975
LOG=NO
==========

to

==========
Add memory pressure notification API

Based on CL 1777883002.

BUG=chromium:590975
LOG=NO

Committed: https://crrev.com/bb9b74e75495ff0d00897760f5f13784e88930b0
Cr-Commit-Position: refs/heads/master@{#35053}
==========

[commit-bot: I haz the power, 2016-03-24 09:53:19]

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