Add memory pressure interrupt for memory pressure notification

Add memory pressure interrupt for memory pressure notification

The CL is one part of "Do V8 GC ASAP if system memory is pressured"(https://codereview.chromium.org/1749073002/)

BUG=chromium:590975
LOG=n

[hong.zheng, 2016-03-09 12:00:34]

Description was changed from

==========
Add memory pressure interrupt for memory pressure notification

The CL is one part of "Do V8 GC ASAP if system memory is
pressured"(https://codereview.chromium.org/1749073002/)

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

to

==========
Add memory pressure interrupt for memory pressure notification

The CL is one part of "Do V8 GC ASAP if system memory is
pressured"(https://codereview.chromium.org/1749073002/)

BUG=590975
==========

[hong.zheng, 2016-03-09 12:23:59]

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

[hong.zheng, 2016-03-09 12:24:28]

On 2016/03/09 12:23:59, hong.zheng wrote:
> mailto:hong.zheng@intel.com changed reviewers:
> + mailto:jochen@chromium.org, mailto:ulan@chromium.org

PTAL

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

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

https://codereview.chromium.org/1777883002/diff/1/include/v8.h#newcode5881
include/v8.h:5881: void RequestMemoryPressureInterrupt();
I'd prefer an API ReportMemoryPressure(level) where level is an enum class
MemoryPressureLevel { kNone, kModerate, kCritical}

internally, we could just clear the interrupt request for none and do a low
memory gc for moderate / critical

https://codereview.chromium.org/1777883002/diff/1/include/v8.h#newcode6039
include/v8.h:6039: void GCNotificationBasedOnMemoryPressureInterrupt();
that shouldn't be required

[jochen (gone - plz use gerrit), 2016-03-09 15:19:10]

Description was changed from

==========
Add memory pressure interrupt for memory pressure notification

The CL is one part of "Do V8 GC ASAP if system memory is
pressured"(https://codereview.chromium.org/1749073002/)

BUG=590975
==========

to

==========
Add memory pressure interrupt for memory pressure notification

The CL is one part of "Do V8 GC ASAP if system memory is
pressured"(https://codereview.chromium.org/1749073002/)

BUG=chromium:590975
LOG=n
==========

[hong.zheng, 2016-03-10 08:05:31]

On 2016/03/09 15:18:59, jochen wrote:
> https://codereview.chromium.org/1777883002/diff/1/include/v8.h
> File include/v8.h (right):
> 
> https://codereview.chromium.org/1777883002/diff/1/include/v8.h#newcode5881
> include/v8.h:5881: void RequestMemoryPressureInterrupt();
> I'd prefer an API ReportMemoryPressure(level) where level is an enum class
> MemoryPressureLevel { kNone, kModerate, kCritical}
> 
> internally, we could just clear the interrupt request for none and do a low
> memory gc for moderate / critical
> 
> https://codereview.chromium.org/1777883002/diff/1/include/v8.h#newcode6039
> include/v8.h:6039: void GCNotificationBasedOnMemoryPressureInterrupt();
> that shouldn't be required

The purpose of GCNotificationBasedOnMemoryPressureInterrupt is to avoid double
invoking low memory GC. For example, if memory pressure interrupt is handled in
V8 firstly, low memory GC triggered by Chromium should be ignored. On the other
hand, if chromium triggers GC earlier than memory pressure interrupt, the
interrupt should be cleared before being handled. 
What do you think? Or do you have other proposal? Thanks.

[jochen (gone - plz use gerrit), 2016-03-10 15:48:57]

On 2016/03/10 at 08:05:31, hong.zheng wrote:
> On 2016/03/09 15:18:59, jochen wrote:
> > https://codereview.chromium.org/1777883002/diff/1/include/v8.h
> > File include/v8.h (right):
> > 
> > https://codereview.chromium.org/1777883002/diff/1/include/v8.h#newcode5881
> > include/v8.h:5881: void RequestMemoryPressureInterrupt();
> > I'd prefer an API ReportMemoryPressure(level) where level is an enum class
> > MemoryPressureLevel { kNone, kModerate, kCritical}
> > 
> > internally, we could just clear the interrupt request for none and do a low
> > memory gc for moderate / critical
> > 
> > https://codereview.chromium.org/1777883002/diff/1/include/v8.h#newcode6039
> > include/v8.h:6039: void GCNotificationBasedOnMemoryPressureInterrupt();
> > that shouldn't be required
> 
> The purpose of GCNotificationBasedOnMemoryPressureInterrupt is to avoid double
invoking low memory GC. For example, if memory pressure interrupt is handled in
V8 firstly, low memory GC triggered by Chromium should be ignored. On the other
hand, if chromium triggers GC earlier than memory pressure interrupt, the
interrupt should be cleared before being handled. 
> What do you think? Or do you have other proposal? Thanks.

I'd expect that chromium just sends the interrupt in the future

[hong.zheng, 2016-03-11 01:53:28]

On 2016/03/10 15:48:57, jochen wrote:
> On 2016/03/10 at 08:05:31, hong.zheng wrote:
> > On 2016/03/09 15:18:59, jochen wrote:
> > > https://codereview.chromium.org/1777883002/diff/1/include/v8.h
> > > File include/v8.h (right):
> > > 
> > > https://codereview.chromium.org/1777883002/diff/1/include/v8.h#newcode5881
> > > include/v8.h:5881: void RequestMemoryPressureInterrupt();
> > > I'd prefer an API ReportMemoryPressure(level) where level is an enum class
> > > MemoryPressureLevel { kNone, kModerate, kCritical}
> > > 
> > > internally, we could just clear the interrupt request for none and do a
low
> > > memory gc for moderate / critical
> > > 
> > > https://codereview.chromium.org/1777883002/diff/1/include/v8.h#newcode6039
> > > include/v8.h:6039: void GCNotificationBasedOnMemoryPressureInterrupt();
> > > that shouldn't be required
> > 
> > The purpose of GCNotificationBasedOnMemoryPressureInterrupt is to avoid
double
> invoking low memory GC. For example, if memory pressure interrupt is handled
in
> V8 firstly, low memory GC triggered by Chromium should be ignored. On the
other
> hand, if chromium triggers GC earlier than memory pressure interrupt, the
> interrupt should be cleared before being handled. 
> > What do you think? Or do you have other proposal? Thanks.
> 
> I'd expect that chromium just sends the interrupt in the future

I get your point that memory pressure GC should be controlled by V8. However
there is a scenario that when chromium receives memory pressure notification, V8
isn't in execution. If we only send interrupt to V8, the V8 GC might be
postponed long time until V8 running again.  
You know, memory pressure is a critical situation, it is better to purge memory
ASAP. So in the current mechanism we need chromium and V8 to control memory
pressure GC together. I agree with you, in the future, When V8 can cover all
scenarios by its own, some API will be obsoleted, like
GCNotificationBasedOnMemoryPressureInterrupt. Does it make sense?

[ulan, 2016-03-11 11:42:48]

On 2016/03/11 01:53:28, hong.zheng wrote:
> On 2016/03/10 15:48:57, jochen wrote:
> > On 2016/03/10 at 08:05:31, hong.zheng wrote:
> > > On 2016/03/09 15:18:59, jochen wrote:
> > > > https://codereview.chromium.org/1777883002/diff/1/include/v8.h
> > > > File include/v8.h (right):
> > > > 
> > > >
https://codereview.chromium.org/1777883002/diff/1/include/v8.h#newcode5881
> > > > include/v8.h:5881: void RequestMemoryPressureInterrupt();
> > > > I'd prefer an API ReportMemoryPressure(level) where level is an enum
class
> > > > MemoryPressureLevel { kNone, kModerate, kCritical}
> > > > 
> > > > internally, we could just clear the interrupt request for none and do a
> low
> > > > memory gc for moderate / critical
> > > > 
> > > >
https://codereview.chromium.org/1777883002/diff/1/include/v8.h#newcode6039
> > > > include/v8.h:6039: void GCNotificationBasedOnMemoryPressureInterrupt();
> > > > that shouldn't be required
> > > 
> > > The purpose of GCNotificationBasedOnMemoryPressureInterrupt is to avoid
> double
> > invoking low memory GC. For example, if memory pressure interrupt is handled
> in
> > V8 firstly, low memory GC triggered by Chromium should be ignored. On the
> other
> > hand, if chromium triggers GC earlier than memory pressure interrupt, the
> > interrupt should be cleared before being handled. 
> > > What do you think? Or do you have other proposal? Thanks.
> > 
> > I'd expect that chromium just sends the interrupt in the future
> 
> I get your point that memory pressure GC should be controlled by V8. However
> there is a scenario that when chromium receives memory pressure notification,
V8
> isn't in execution. If we only send interrupt to V8, the V8 GC might be
> postponed long time until V8 running again.  
> You know, memory pressure is a critical situation, it is better to purge
memory
> ASAP. So in the current mechanism we need chromium and V8 to control memory
> pressure GC together. I agree with you, in the future, When V8 can cover all
> scenarios by its own, some API will be obsoleted, like
> GCNotificationBasedOnMemoryPressureInterrupt. Does it make sense?

I discussed this with Jochen offline. V8 should expose
Isolate::MemoryPressureNotification(MemoryPressureLevel) that can be called from
any thread.
Isolate::MemoryPressureNotification(kCritical) would mean that the process will
be killed by OS if we don't do GC now.
Isolate::MemoryPressureNotification(kModerate) would mean that we should tune V8
heuristics to save memory.

If you agree, I'll upload CL that adds this notification to API.

[hong.zheng, 2016-03-15 09:47:17]

PTAL. 
enable memory pressure level and add interface
Isolate::HandleMemoryPressureInterrupt for handling MemoryPressureInterrupt in
V8 idle

[hong.zheng, 2016-03-15 09:48:05]

On 2016/03/11 11:42:48, ulan wrote:
> On 2016/03/11 01:53:28, hong.zheng wrote:
> > On 2016/03/10 15:48:57, jochen wrote:
> > > On 2016/03/10 at 08:05:31, hong.zheng wrote:
> > > > On 2016/03/09 15:18:59, jochen wrote:
> > > > > https://codereview.chromium.org/1777883002/diff/1/include/v8.h
> > > > > File include/v8.h (right):
> > > > > 
> > > > >
> https://codereview.chromium.org/1777883002/diff/1/include/v8.h#newcode5881
> > > > > include/v8.h:5881: void RequestMemoryPressureInterrupt();
> > > > > I'd prefer an API ReportMemoryPressure(level) where level is an enum
> class
> > > > > MemoryPressureLevel { kNone, kModerate, kCritical}
> > > > > 
> > > > > internally, we could just clear the interrupt request for none and do
a
> > low
> > > > > memory gc for moderate / critical
> > > > > 
> > > > >
> https://codereview.chromium.org/1777883002/diff/1/include/v8.h#newcode6039
> > > > > include/v8.h:6039: void
GCNotificationBasedOnMemoryPressureInterrupt();
> > > > > that shouldn't be required
> > > > 
> > > > The purpose of GCNotificationBasedOnMemoryPressureInterrupt is to avoid
> > double
> > > invoking low memory GC. For example, if memory pressure interrupt is
handled
> > in
> > > V8 firstly, low memory GC triggered by Chromium should be ignored. On the
> > other
> > > hand, if chromium triggers GC earlier than memory pressure interrupt, the
> > > interrupt should be cleared before being handled. 
> > > > What do you think? Or do you have other proposal? Thanks.
> > > 
> > > I'd expect that chromium just sends the interrupt in the future
> > 
> > I get your point that memory pressure GC should be controlled by V8. However
> > there is a scenario that when chromium receives memory pressure
notification,
> V8
> > isn't in execution. If we only send interrupt to V8, the V8 GC might be
> > postponed long time until V8 running again.  
> > You know, memory pressure is a critical situation, it is better to purge
> memory
> > ASAP. So in the current mechanism we need chromium and V8 to control memory
> > pressure GC together. I agree with you, in the future, When V8 can cover all
> > scenarios by its own, some API will be obsoleted, like
> > GCNotificationBasedOnMemoryPressureInterrupt. Does it make sense?
> 
> I discussed this with Jochen offline. V8 should expose
> Isolate::MemoryPressureNotification(MemoryPressureLevel) that can be called
from
> any thread.
> Isolate::MemoryPressureNotification(kCritical) would mean that the process
will
> be killed by OS if we don't do GC now.
> Isolate::MemoryPressureNotification(kModerate) would mean that we should tune
V8
> heuristics to save memory.
> 
> If you agree, I'll upload CL that adds this notification to API.

According to Jochen's feedback, I have enabled HandleMemoryPressureInterrupt
with MemoryPressureLevel in the CL. Please review. And I will update Chromium CL
based on the CL soon. 
I agree with you that V8 should use different GC policy in different memory
pressure level. I'm interested in the heuristics mechanism you mentioned. Could
we collaborate with each other to enable it in the CL?

[hong.zheng, 2016-03-15 09:54:19]

On 2016/03/15 09:48:05, hong.zheng wrote:
> On 2016/03/11 11:42:48, ulan wrote:
> > On 2016/03/11 01:53:28, hong.zheng wrote:
> > > On 2016/03/10 15:48:57, jochen wrote:
> > > > On 2016/03/10 at 08:05:31, hong.zheng wrote:
> > > > > On 2016/03/09 15:18:59, jochen wrote:
> > > > > > https://codereview.chromium.org/1777883002/diff/1/include/v8.h
> > > > > > File include/v8.h (right):
> > > > > > 
> > > > > >
> > https://codereview.chromium.org/1777883002/diff/1/include/v8.h#newcode5881
> > > > > > include/v8.h:5881: void RequestMemoryPressureInterrupt();
> > > > > > I'd prefer an API ReportMemoryPressure(level) where level is an enum
> > class
> > > > > > MemoryPressureLevel { kNone, kModerate, kCritical}
> > > > > > 
> > > > > > internally, we could just clear the interrupt request for none and
do
> a
> > > low
> > > > > > memory gc for moderate / critical
> > > > > > 
> > > > > >
> > https://codereview.chromium.org/1777883002/diff/1/include/v8.h#newcode6039
> > > > > > include/v8.h:6039: void
> GCNotificationBasedOnMemoryPressureInterrupt();
> > > > > > that shouldn't be required
> > > > > 
> > > > > The purpose of GCNotificationBasedOnMemoryPressureInterrupt is to
avoid
> > > double
> > > > invoking low memory GC. For example, if memory pressure interrupt is
> handled
> > > in
> > > > V8 firstly, low memory GC triggered by Chromium should be ignored. On
the
> > > other
> > > > hand, if chromium triggers GC earlier than memory pressure interrupt,
the
> > > > interrupt should be cleared before being handled. 
> > > > > What do you think? Or do you have other proposal? Thanks.
> > > > 
> > > > I'd expect that chromium just sends the interrupt in the future
> > > 
> > > I get your point that memory pressure GC should be controlled by V8.
However
> > > there is a scenario that when chromium receives memory pressure
> notification,
> > V8
> > > isn't in execution. If we only send interrupt to V8, the V8 GC might be
> > > postponed long time until V8 running again.  
> > > You know, memory pressure is a critical situation, it is better to purge
> > memory
> > > ASAP. So in the current mechanism we need chromium and V8 to control
memory
> > > pressure GC together. I agree with you, in the future, When V8 can cover
all
> > > scenarios by its own, some API will be obsoleted, like
> > > GCNotificationBasedOnMemoryPressureInterrupt. Does it make sense?
> > 
> > I discussed this with Jochen offline. V8 should expose
> > Isolate::MemoryPressureNotification(MemoryPressureLevel) that can be called
> from
> > any thread.
> > Isolate::MemoryPressureNotification(kCritical) would mean that the process
> will
> > be killed by OS if we don't do GC now.
> > Isolate::MemoryPressureNotification(kModerate) would mean that we should
tune
> V8
> > heuristics to save memory.
> > 
> > If you agree, I'll upload CL that adds this notification to API.
> 
> According to Jochen's feedback, I have enabled HandleMemoryPressureInterrupt
> with MemoryPressureLevel in the CL. Please review. And I will update Chromium
CL
> based on the CL soon. 
> I agree with you that V8 should use different GC policy in different memory
> pressure level. I'm interested in the heuristics mechanism you mentioned.
Could
> we collaborate with each other to enable it in the CL?

Or would you like to enable the heuristics mechanism after the CL landed?

[jochen (gone - plz use gerrit), 2016-03-15 20:57:19]

where not really moving closer to the kind of API I was envisioning. The
embedder should not have to decide whether v8 should use interrupts or not to do
something about the  memory pressure.

I think it's more efficient if we just implement the API ourselves.

https://codereview.chromium.org/1777883002/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1777883002/diff/20001/include/v8.h#newcode5557
include/v8.h:5557: enum MemoryPressureLevel { kNone, kModerate, kCritical };
this should be an enum class

https://codereview.chromium.org/1777883002/diff/20001/include/v8.h#newcode5883
include/v8.h:5883: void RequestMemoryPressureInterrupt(MemoryPressureLevel
level);
this should be NotifyMemoryPressure(MemoryPressureLevel)

https://codereview.chromium.org/1777883002/diff/20001/include/v8.h#newcode6200
include/v8.h:6200: void HandleMemoryPressureInterrupt();
please remove this.