Expose ReleaseFreeMemory to free up memory on memory pressure or idle cleanup.

Expose ReleaseFreeMemory to free up memory on memory pressure or idle cleanup.

Some unused memory maybe left committed by PartitionAlloc for
performance reasons. This CL exposes api from blink to free up the
memory at various situations like a low memory signal or invoked at
idle cleanup.

BUG=495171
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196675

[ssid, 2015-06-02 14:45:18]

ssid@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-06-02 17:18:56]

https://codereview.chromium.org/1155293007/diff/1/Source/web/WebKit.cpp
File Source/web/WebKit.cpp (right):

https://codereview.chromium.org/1155293007/diff/1/Source/web/WebKit.cpp#newco...
Source/web/WebKit.cpp:293: void ReleaseFreeMemory()
nit: lowercase r (releaseFreeMemory)

https://codereview.chromium.org/1155293007/diff/1/Source/web/WebKit.cpp#newco...
Source/web/WebKit.cpp:295:
WTF::partitionPurgeMemory(WTF::Partitions::getBufferPartition());
Hmm threading..
I think that here you need:
1) ASSERT(isMainThread()) for the object model and rendering partitions
2) Take the lock for the other two partitions.

Also, I would move these four lines to Partitions.cpp, expose this method there,
and here just call the method in Partitions.h.

Rationale: the knowledge about the existing partitions should be all in one
place, not spread out.

[ssid, 2015-06-03 10:58:16]

Done.

[Primiano Tucci (use gerrit), 2015-06-03 11:09:22]

primiano@chromium.org changed reviewers:
+ cevans@chromium.org, haraken@chromium.org

[Primiano Tucci (use gerrit), 2015-06-03 11:09:23]

LGTM.
It seems that the pattern that we are using already is expose the individual
APIs from blink and hook them into RenderThreadImpl::OnMemoryPressure

+haraken@, WDYT?
+cevans@ to doublecheck the locking logic here. My understanding is that two of
partitions are only accessible from the main thread and the others should be
locked instead.

https://codereview.chromium.org/1155293007/diff/20001/public/web/WebKit.h
File public/web/WebKit.h (right):

https://codereview.chromium.org/1155293007/diff/20001/public/web/WebKit.h#new...
public/web/WebKit.h:88: // Clears free memory held for performance.
Hmm this is a bit confusing. Maybe say
// Decommit freeable memory pages.

[haraken, 2015-06-03 11:39:00]

https://codereview.chromium.org/1155293007/diff/20001/Source/wtf/Partitions.cpp
File Source/wtf/Partitions.cpp (right):

https://codereview.chromium.org/1155293007/diff/20001/Source/wtf/Partitions.c...
Source/wtf/Partitions.cpp:79: spinLockLock(&getBufferPartition()->lock);

Can we move this lock into partitionPurgeMemory?

We want to write these as:

  getBufferPartition()->partitionPurgeMemory();
  getFastMallocPartition()->partitionPurgeMemory();
  ...;

https://codereview.chromium.org/1155293007/diff/20001/public/web/WebKit.h
File public/web/WebKit.h (right):

https://codereview.chromium.org/1155293007/diff/20001/public/web/WebKit.h#new...
public/web/WebKit.h:89: BLINK_EXPORT void releaseFreeMemory();

Also I'd rename this API to decommitFreeableMemory.

[ssid, 2015-06-03 11:45:31]

Please see comment inline.

https://codereview.chromium.org/1155293007/diff/20001/Source/wtf/Partitions.cpp
File Source/wtf/Partitions.cpp (right):

https://codereview.chromium.org/1155293007/diff/20001/Source/wtf/Partitions.c...
Source/wtf/Partitions.cpp:79: spinLockLock(&getBufferPartition()->lock);
On 2015/06/03 11:39:00, haraken wrote:
> 
> Can we move this lock into partitionPurgeMemory?
> 
> We want to write these as:
> 
>   getBufferPartition()->partitionPurgeMemory();
>   getFastMallocPartition()->partitionPurgeMemory();
>   ...;

1. PartitionRootBase is a struct and partitionPurgeMemory() is a function no in
the struct.
2. Lock is only needed in PartitionRootGeneric and not in PartitionRoot (both of
these extend PartitionRootBase).

I would have refactor partitionPurgeMemory() to move into the struct and have
two different implementation for it, one with lock and one without.

WDYT I should do here?

[haraken, 2015-06-03 11:49:22]

On 2015/06/03 11:45:31, ssid wrote:
> Please see comment inline.
> 
>
https://codereview.chromium.org/1155293007/diff/20001/Source/wtf/Partitions.cpp
> File Source/wtf/Partitions.cpp (right):
> 
>
https://codereview.chromium.org/1155293007/diff/20001/Source/wtf/Partitions.c...
> Source/wtf/Partitions.cpp:79: spinLockLock(&getBufferPartition()->lock);
> On 2015/06/03 11:39:00, haraken wrote:
> > 
> > Can we move this lock into partitionPurgeMemory?
> > 
> > We want to write these as:
> > 
> >   getBufferPartition()->partitionPurgeMemory();
> >   getFastMallocPartition()->partitionPurgeMemory();
> >   ...;
> 
> 1. PartitionRootBase is a struct and partitionPurgeMemory() is a function no
in
> the struct.
> 2. Lock is only needed in PartitionRootGeneric and not in PartitionRoot (both
of
> these extend PartitionRootBase).
> 
> I would have refactor partitionPurgeMemory() to move into the struct and have
> two different implementation for it, one with lock and one without.
> 
> WDYT I should do here?

Given the current design of PartitionRoot, the current CL makes sense to me.
LGTM.

Let's wait for cevans' approval.

[Chris Evans, 2015-06-05 06:18:41]

On 2015/06/03 11:49:22, haraken wrote:
> On 2015/06/03 11:45:31, ssid wrote:
> > Please see comment inline.
> > 
> >
>
https://codereview.chromium.org/1155293007/diff/20001/Source/wtf/Partitions.cpp
> > File Source/wtf/Partitions.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1155293007/diff/20001/Source/wtf/Partitions.c...
> > Source/wtf/Partitions.cpp:79: spinLockLock(&getBufferPartition()->lock);
> > On 2015/06/03 11:39:00, haraken wrote:
> > > 
> > > Can we move this lock into partitionPurgeMemory?
> > > 
> > > We want to write these as:
> > > 
> > >   getBufferPartition()->partitionPurgeMemory();
> > >   getFastMallocPartition()->partitionPurgeMemory();
> > >   ...;
> > 
> > 1. PartitionRootBase is a struct and partitionPurgeMemory() is a function no
> in
> > the struct.
> > 2. Lock is only needed in PartitionRootGeneric and not in PartitionRoot
(both
> of
> > these extend PartitionRootBase).
> > 
> > I would have refactor partitionPurgeMemory() to move into the struct and
have
> > two different implementation for it, one with lock and one without.
> > 
> > WDYT I should do here?
> 
> Given the current design of PartitionRoot, the current CL makes sense to me.
> LGTM.
> 
> Let's wait for cevans' approval.

not lgtm

I messed up here, sorry! The locking should definitely be internal to
PartitionAlloc. The poor client should not have to bother about such details!

There should be a partitionPurgeMemory separate from a
partitionPurgeMemoryGeneric, both accepting the different subclasses of
PartitionRootBase in order to be type safe.

We can either fix it as part of this CL, or if you prefer, you can land this and
I'll mop up the mess since it's my mess. Let me know if you prefer the latter
and I'll reverse my not lgtm.

[ssid, 2015-06-05 06:34:17]

On 2015/06/05 06:18:41, Chris Evans wrote:
> On 2015/06/03 11:49:22, haraken wrote:
> > On 2015/06/03 11:45:31, ssid wrote:
> > > Please see comment inline.
> > > 
> > >
> >
>
https://codereview.chromium.org/1155293007/diff/20001/Source/wtf/Partitions.cpp
> > > File Source/wtf/Partitions.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1155293007/diff/20001/Source/wtf/Partitions.c...
> > > Source/wtf/Partitions.cpp:79: spinLockLock(&getBufferPartition()->lock);
> > > On 2015/06/03 11:39:00, haraken wrote:
> > > > 
> > > > Can we move this lock into partitionPurgeMemory?
> > > > 
> > > > We want to write these as:
> > > > 
> > > >   getBufferPartition()->partitionPurgeMemory();
> > > >   getFastMallocPartition()->partitionPurgeMemory();
> > > >   ...;
> > > 
> > > 1. PartitionRootBase is a struct and partitionPurgeMemory() is a function
no
> > in
> > > the struct.
> > > 2. Lock is only needed in PartitionRootGeneric and not in PartitionRoot
> (both
> > of
> > > these extend PartitionRootBase).
> > > 
> > > I would have refactor partitionPurgeMemory() to move into the struct and
> have
> > > two different implementation for it, one with lock and one without.
> > > 
> > > WDYT I should do here?
> > 
> > Given the current design of PartitionRoot, the current CL makes sense to me.
> > LGTM.
> > 
> > Let's wait for cevans' approval.
> 
> not lgtm
> 
> I messed up here, sorry! The locking should definitely be internal to
> PartitionAlloc. The poor client should not have to bother about such details!
> 
> There should be a partitionPurgeMemory separate from a
> partitionPurgeMemoryGeneric, both accepting the different subclasses of
> PartitionRootBase in order to be type safe.
> 
> We can either fix it as part of this CL, or if you prefer, you can land this
and
> I'll mop up the mess since it's my mess. Let me know if you prefer the latter
> and I'll reverse my not lgtm.

No problem. Will fix this here :)

[ssid, 2015-06-05 12:44:49]

Made changes, PTAL.

https://codereview.chromium.org/1155293007/diff/20001/public/web/WebKit.h
File public/web/WebKit.h (right):

https://codereview.chromium.org/1155293007/diff/20001/public/web/WebKit.h#new...
public/web/WebKit.h:89: BLINK_EXPORT void releaseFreeMemory();
On 2015/06/03 11:39:00, haraken wrote:
> 
> Also I'd rename this API to decommitFreeableMemory.

Done.

[ssid, 2015-06-05 12:44:51]

Made changes, PTAL.

[Chris Evans, 2015-06-05 17:06:29]

On 2015/06/05 12:44:51, ssid wrote:
> Made changes, PTAL.

LGTM

[Primiano Tucci (use gerrit), 2015-06-05 17:18:22]

LGTM thanks

[Primiano Tucci (use gerrit), 2015-06-05 17:25:57]

primiano@chromium.org changed reviewers:
+ rbyers@chromium.org

[Primiano Tucci (use gerrit), 2015-06-05 17:25:58]

+rbyers for public/web/WebKit.h stamp (we miss an owner there)

[Rick Byers, 2015-06-05 17:35:47]

public/ LGTM with nit

https://codereview.chromium.org/1155293007/diff/60001/public/web/WebKit.h
File public/web/WebKit.h (right):

https://codereview.chromium.org/1155293007/diff/60001/public/web/WebKit.h#new...
public/web/WebKit.h:88: // Decommit freeable memory pages.
nit: This is just for tooling scenarios, right?  It would be nice if the comment
indicated when/how this should be used - eg. "Intended to be used only by memory
monitoring performance tools".  Like forcing a GC I'd expect you normally
wouldn't want callers invoking this in non-tooling scenarios, right?

[Primiano Tucci (use gerrit), 2015-06-05 18:24:51]

https://codereview.chromium.org/1155293007/diff/60001/public/web/WebKit.h
File public/web/WebKit.h (right):

https://codereview.chromium.org/1155293007/diff/60001/public/web/WebKit.h#new...
public/web/WebKit.h:88: // Decommit freeable memory pages.
On 2015/06/05 17:35:46, Rick Byers wrote:
> nit: This is just for tooling scenarios, right?  It would be nice if the
comment
> indicated when/how this should be used - eg. "Intended to be used only by
memory
> monitoring performance tools".  Like forcing a GC I'd expect you normally
> wouldn't want callers invoking this in non-tooling scenarios, right?

Nope, this is going to be an actual memory saving thing (yay).
As the title suggests we are going to call this during long idle pauses and on
memory pressure.
We figured out while writing the tools (it's going to save 200-800k per renderer
process) :)
See the case study this doc for details:
https://docs.google.com/document/d/1zBuz0WVBTReoj0gXooihTFPwMQQkCkQGy0Cl9lSwU...

[Primiano Tucci (use gerrit), 2015-06-05 18:28:36]

Just to be clear, the only reason why we don't do this after every free is that
there is a small performance hit due to calling this method, so... like a GC and
free is usually a perf hit.
It should be called from time to time, but not always.

Do you want this to be commented?

[Rick Byers, 2015-06-05 18:40:12]

On 2015/06/05 18:28:36, Primiano Tucci wrote:
> Just to be clear, the only reason why we don't do this after every free is
that
> there is a small performance hit due to calling this method, so... like a GC
and
> free is usually a perf hit.
> It should be called from time to time, but not always.

Makes sense, thanks.

> Do you want this to be commented?

Yes I think it's worth replacing the comment with something brief about this -
eg. 'The embedder should call this periodically in an attempt to balance overall
performance and memory usage'.  Obviously it's non-trivial to decide what the
best times to call it is, but I can see how the embedder is in the best position
to decide and so an API makes sense.

Thanks for the explanation!

[ssid, 2015-06-05 18:46:21]

On 2015/06/05 18:40:12, Rick Byers wrote:
> On 2015/06/05 18:28:36, Primiano Tucci wrote:
> > Just to be clear, the only reason why we don't do this after every free is
> that
> > there is a small performance hit due to calling this method, so... like a GC
> and
> > free is usually a perf hit.
> > It should be called from time to time, but not always.
> 
> Makes sense, thanks.
> 
> > Do you want this to be commented?
> 
> Yes I think it's worth replacing the comment with something brief about this -
> eg. 'The embedder should call this periodically in an attempt to balance
overall
> performance and memory usage'.  Obviously it's non-trivial to decide what the
> best times to call it is, but I can see how the embedder is in the best
position
> to decide and so an API makes sense.
> 
> Thanks for the explanation!

Done, thanks.

[ssid, 2015-06-05 18:46:27]

The CQ bit was checked by ssid@chromium.org

[ssid, 2015-06-05 18:46:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, primiano@chromium.org, cevans@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1155293007/#ps100001
(title: "Old comment doesn't make sense. removed it.")

[commit-bot: I haz the power, 2015-06-05 18:47:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1155293007/100001

[commit-bot: I haz the power, 2015-06-05 19:00:07]

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

[commit-bot: I haz the power, 2015-06-05 19:00:09]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/57882)

[ssid, 2015-06-08 13:36:52]

The CQ bit was checked by ssid@chromium.org

[ssid, 2015-06-08 13:36:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, haraken@chromium.org, cevans@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1155293007/#ps120001
(title: "Fixing test calls.")

[commit-bot: I haz the power, 2015-06-08 13:37:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1155293007/120001

[commit-bot: I haz the power, 2015-06-08 13:40:54]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196675