Add a UseCounter that measures the amount of memory used in PartitionAlloc

Add a UseCounter that measures the amount of memory used in PartitionAlloc

This CL adds a UseCounter that is counted up when the memory usage of
PartitionAlloc exceeds X MB, where X is 1, 2, 3, 4, ..., 4096.
The UseCounter is counted up only once for each slot size for each renderer process.
The UseCounter for X MB impiles the number of renderer proceses which will hit OOM
under the assumption that all the devices in the world have X MB of memory.

See https://groups.google.com/a/chromium.org/d/topic/blink-dev/_MOh3lZ8zxo/discussion for more background.

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

[haraken, 2015-04-02 12:26:18]

haraken@chromium.org changed reviewers:
+ cevans@chromium.org, oilpan-reviews@chromium.org, tsepez@chromium.org

[haraken, 2015-04-02 12:26:18]

PTAL

[Chris Evans, 2015-04-02 15:53:01]

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

https://codereview.chromium.org/1053793004/diff/1/Source/web/WebKit.cpp#newco...
Source/web/WebKit.cpp:148: return
Platform::current()->histogramEnumeration(name, sample, boundaryValue);
Nit: not sure what the style guide says about using "return" here in a void
function? I'd ditch it.

https://codereview.chromium.org/1053793004/diff/1/Source/wtf/PartitionAlloc.cpp
File Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/1053793004/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:33: #include "wtf/Partitions.h"
Hmm -- this introduces a circular dependency. Not a huge deal but I do try to
keep PartitionAlloc as dependency-free as possible.

Would it be possible to have the client register a callback via a new function
in PartitionAlloc.h?

https://codereview.chromium.org/1053793004/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:360: Partitions::recordMemoryUsageIfNeeded();
To reduce minor code duplication, can we pull both the updating of the variable,
the ASSERT, and the firing of the callback into a new function, something like
partitionUpdateMemoryAccounting() ?

https://codereview.chromium.org/1053793004/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:621: Partitions::recordMemoryUsageIfNeeded();
For consistency, we should probably call the callback whenever
totalSizeOfCommittedPages is called?

I don't see any calls directly after we've subtracted from the variable, so my
guess is that we've missed a place or two?

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

https://codereview.chromium.org/1053793004/diff/1/Source/wtf/Partitions.cpp#n...
Source/wtf/Partitions.cpp:74: void Partitions::recordMemoryUsageIfNeeded()
This might be called on any thread -- is that OK? Probably worth at least a
comment.

It won't be called concurrently because of the existing locking. We should
probably document (and preserve) that invariant too, possibly in
PartitionAlloc.h, if you accept my other comment about making callback
registration explicit?

https://codereview.chromium.org/1053793004/diff/1/Source/wtf/Partitions.cpp#n...
Source/wtf/Partitions.cpp:77: static bool s_isUsedSlot[maxSizeInMB] = { false };
This is kind of big -- 4KB. If we keep it, I recommend storing the booleans as
single bits.

https://codereview.chromium.org/1053793004/diff/1/Source/wtf/Partitions.cpp#n...
Source/wtf/Partitions.cpp:85: // Send the UseCounter only once for each slot for
each renderer process.
Another strategy -- which would eliminate the above array altogether -- would be
to only send the UseCounter when we see a new record high?

[haraken, 2015-04-02 23:31:32]

Thanks for review!

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

https://codereview.chromium.org/1053793004/diff/1/Source/web/WebKit.cpp#newco...
Source/web/WebKit.cpp:148: return
Platform::current()->histogramEnumeration(name, sample, boundaryValue);
On 2015/04/02 15:53:01, Chris Evans wrote:
> Nit: not sure what the style guide says about using "return" here in a void
> function? I'd ditch it.

Done.

https://codereview.chromium.org/1053793004/diff/1/Source/wtf/PartitionAlloc.cpp
File Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/1053793004/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:33: #include "wtf/Partitions.h"
On 2015/04/02 15:53:01, Chris Evans wrote:
> Hmm -- this introduces a circular dependency. Not a huge deal but I do try to
> keep PartitionAlloc as dependency-free as possible.
> 
> Would it be possible to have the client register a callback via a new function
> in PartitionAlloc.h?

Done.

https://codereview.chromium.org/1053793004/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:360: Partitions::recordMemoryUsageIfNeeded();
On 2015/04/02 15:53:01, Chris Evans wrote:
> To reduce minor code duplication, can we pull both the updating of the
variable,
> the ASSERT, and the firing of the callback into a new function, something like
> partitionUpdateMemoryAccounting() ?

Done.

https://codereview.chromium.org/1053793004/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:621: Partitions::recordMemoryUsageIfNeeded();
On 2015/04/02 15:53:01, Chris Evans wrote:
> For consistency, we should probably call the callback whenever
> totalSizeOfCommittedPages is called?
> 
> I don't see any calls directly after we've subtracted from the variable, so my
> guess is that we've missed a place or two?

Done. Actually we don't need to report memory usage when we subtracted from the
variable (since we report the memory usage only when we see a new value high)
though.

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

https://codereview.chromium.org/1053793004/diff/1/Source/wtf/Partitions.cpp#n...
Source/wtf/Partitions.cpp:74: void Partitions::recordMemoryUsageIfNeeded()
On 2015/04/02 15:53:01, Chris Evans wrote:
> This might be called on any thread -- is that OK? Probably worth at least a
> comment.
> 
> It won't be called concurrently because of the existing locking. We should
> probably document (and preserve) that invariant too, possibly in
> PartitionAlloc.h, if you accept my other comment about making callback
> registration explicit?

To avoid races on the static variable, I restricted it to the main thread.

https://codereview.chromium.org/1053793004/diff/1/Source/wtf/Partitions.cpp#n...
Source/wtf/Partitions.cpp:85: // Send the UseCounter only once for each slot for
each renderer process.
On 2015/04/02 15:53:01, Chris Evans wrote:
> Another strategy -- which would eliminate the above array altogether -- would
be
> to only send the UseCounter when we see a new record high?

Nice idea! Done.

[Chris Evans, 2015-04-03 22:14:21]

https://codereview.chromium.org/1053793004/diff/40001/Source/wtf/PartitionAll...
File Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/1053793004/diff/40001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:33: #include "wtf/Partitions.h"
Nit: we can get rid of this include now, right?

https://codereview.chromium.org/1053793004/diff/40001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:338: root->reportMemoryUsageFunction();
Crash? We don't want to force the client to provide one of these, so it could be
NULL.
(Remember that PartitionAlloc is generic allocator, so we don't want to tie it
to Chrome's usage of it)

https://codereview.chromium.org/1053793004/diff/40001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:344: ASSERT(root->totalSizeOfCommittedPages <=
root->totalSizeOfSuperPages + root->totalSizeOfDirectMappedPages);
I think we should still fire the callback here. The simplest / most generic
callback I can think of is "number of committed pages changed".

It's true that Chrome will always ignore the callback if the number goes down,
but that's an implementation detail of Chrome that is totally opaque to
PartitionAlloc.

https://codereview.chromium.org/1053793004/diff/40001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:346: // only when we see the highest memory usage
we've ever seen.
Layering violation: mention of UseCounter here. PartitionAlloc is just a
malloc() that happens to be used by Chrome.

https://codereview.chromium.org/1053793004/diff/40001/Source/wtf/PartitionAll...
File Source/wtf/PartitionAlloc.h (right):

https://codereview.chromium.org/1053793004/diff/40001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.h:278: 
Nit: add comment here: "This callback can be called on any thread."

https://codereview.chromium.org/1053793004/diff/40001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.h:279: typedef void (*ReportMemoryUsageFunction)();
Nit: I'm not sure about the name of this callback function. It doesn't take any
parameters, so we're not really "report"ing anything.
Maybe "NotifyCommittedMemoryChangedFunction" or something like that?

[haraken, 2015-04-05 15:01:52]

Thanks for review! All comments addressed.

https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/PartitionAll...
File Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:333: static void
partitionIncreaseCommittedPages(PartitionRootBase* root, size_t len)

I added two separate functions (for increase and decrease) to use size_t. If you
want to use signed long long and unify these functions, please say so :)

[Chris Evans, 2015-04-06 04:39:53]

LGTM but please do check the re-entrancy concern!
(I wonder if it's possible to find some other place to post the histogram, such
as some Blink timer tick function?)

https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/PartitionAll...
File Source/wtf/PartitionAlloc.h (right):

https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.h:279: // This callback can be called on any thread.
Nit: also document that it's illegal to re-enter PartitionAlloc from within this
callback.

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

https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/Partitions.c...
Source/wtf/Partitions.cpp:92:
m_histogramEnumeration("PartitionAlloc.CommittedSize", sizeInMB,
supportedMaxSizeInMB);
Hmm! Can we be sure that this histogram enumeration function will never itself
try and call PartitionAlloc? Re-entrancy here would be illegal.

[haraken, 2015-04-06 06:14:14]

https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/PartitionAll...
File Source/wtf/PartitionAlloc.h (right):

https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.h:279: // This callback can be called on any thread.
On 2015/04/06 04:39:53, Chris Evans wrote:
> Nit: also document that it's illegal to re-enter PartitionAlloc from within
this
> callback.

Done.

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

https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/Partitions.c...
Source/wtf/Partitions.cpp:92:
m_histogramEnumeration("PartitionAlloc.CommittedSize", sizeInMB,
supportedMaxSizeInMB);
On 2015/04/06 04:39:53, Chris Evans wrote:
> Hmm! Can we be sure that this histogram enumeration function will never itself
> try and call PartitionAlloc? Re-entrancy here would be illegal.

It can potentially be re-entered, but is it really concerning? Even if that
happens, it will just reports two UseCounters for the same CommittedSize.

I can add a thread-safe flag to histogramEnumerationFunction() in WebKit.cpp to
prevent the re-entrancy, but I'm not sure if we really need this. As far as I
look at the implementation of histogramEnumeration, it looks like
re-entrant-safe.

[Chris Evans, 2015-04-06 17:31:49]

On 2015/04/06 06:14:14, haraken wrote:
>
https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/PartitionAll...
> File Source/wtf/PartitionAlloc.h (right):
> 
>
https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/PartitionAll...
> Source/wtf/PartitionAlloc.h:279: // This callback can be called on any thread.
> On 2015/04/06 04:39:53, Chris Evans wrote:
> > Nit: also document that it's illegal to re-enter PartitionAlloc from within
> this
> > callback.
> 
> Done.
> 
>
https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/Partitions.cpp
> File Source/wtf/Partitions.cpp (right):
> 
>
https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/Partitions.c...
> Source/wtf/Partitions.cpp:92:
> m_histogramEnumeration("PartitionAlloc.CommittedSize", sizeInMB,
> supportedMaxSizeInMB);
> On 2015/04/06 04:39:53, Chris Evans wrote:
> > Hmm! Can we be sure that this histogram enumeration function will never
itself
> > try and call PartitionAlloc? Re-entrancy here would be illegal.
> 
> It can potentially be re-entered, but is it really concerning? Even if that
> happens, it will just reports two UseCounters for the same CommittedSize.

Ah, sorry, I was referring to a different re-entrancy:

The histogramEnumerationFunction() is a very non-trivial function, and it is
also subject to implementation changes over time. I'm worried that it might call
a memory allocation function, and that function might be backed by
partitionAlloc(), either now or in the future.

It is very illegal to call back into partitionAlloc() while we are already
inside partitionAlloc() ;-)

If it's possible, I think the best design might be to poll the commit usage at
some safe point. Do we have some suitable timer tick or heatbeat location in
Blink?

> 
> I can add a thread-safe flag to histogramEnumerationFunction() in WebKit.cpp
to
> prevent the re-entrancy, but I'm not sure if we really need this. As far as I
> look at the implementation of histogramEnumeration, it looks like
> re-entrant-safe.

[haraken, 2015-04-07 00:42:38]

On 2015/04/06 17:31:49, Chris Evans wrote:
> On 2015/04/06 06:14:14, haraken wrote:
> >
>
https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/PartitionAll...
> > File Source/wtf/PartitionAlloc.h (right):
> > 
> >
>
https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/PartitionAll...
> > Source/wtf/PartitionAlloc.h:279: // This callback can be called on any
thread.
> > On 2015/04/06 04:39:53, Chris Evans wrote:
> > > Nit: also document that it's illegal to re-enter PartitionAlloc from
within
> > this
> > > callback.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/Partitions.cpp
> > File Source/wtf/Partitions.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/Partitions.c...
> > Source/wtf/Partitions.cpp:92:
> > m_histogramEnumeration("PartitionAlloc.CommittedSize", sizeInMB,
> > supportedMaxSizeInMB);
> > On 2015/04/06 04:39:53, Chris Evans wrote:
> > > Hmm! Can we be sure that this histogram enumeration function will never
> itself
> > > try and call PartitionAlloc? Re-entrancy here would be illegal.
> > 
> > It can potentially be re-entered, but is it really concerning? Even if that
> > happens, it will just reports two UseCounters for the same CommittedSize.
> 
> Ah, sorry, I was referring to a different re-entrancy:
> 
> The histogramEnumerationFunction() is a very non-trivial function, and it is
> also subject to implementation changes over time. I'm worried that it might
call
> a memory allocation function, and that function might be backed by
> partitionAlloc(), either now or in the future.
> 
> It is very illegal to call back into partitionAlloc() while we are already
> inside partitionAlloc() ;-)
> 
> If it's possible, I think the best design might be to poll the commit usage at
> some safe point. Do we have some suitable timer tick or heatbeat location in
> Blink?

One way to do that would be using a Timer to trigger
histogramEnumerationFunction(), but it's not that easy since Timer is in
platform/ and Partitions is in wtf/ (Note: This dependency issue is going to be
solved in the near future by our Blink componentization).

How about calling Partitions::notifyCommittedMemoryChanged() in Oilpan's GC? We
can call both Partitions::notifyCommittedMemoryChanged() and
Heap::notifyCommittedMemoryChanged() there. The latest CL does that.

[haraken, 2015-04-07 00:44:10]

On 2015/04/07 00:42:38, haraken wrote:
> On 2015/04/06 17:31:49, Chris Evans wrote:
> > On 2015/04/06 06:14:14, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/PartitionAll...
> > > File Source/wtf/PartitionAlloc.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/PartitionAll...
> > > Source/wtf/PartitionAlloc.h:279: // This callback can be called on any
> thread.
> > > On 2015/04/06 04:39:53, Chris Evans wrote:
> > > > Nit: also document that it's illegal to re-enter PartitionAlloc from
> within
> > > this
> > > > callback.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/Partitions.cpp
> > > File Source/wtf/Partitions.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1053793004/diff/80001/Source/wtf/Partitions.c...
> > > Source/wtf/Partitions.cpp:92:
> > > m_histogramEnumeration("PartitionAlloc.CommittedSize", sizeInMB,
> > > supportedMaxSizeInMB);
> > > On 2015/04/06 04:39:53, Chris Evans wrote:
> > > > Hmm! Can we be sure that this histogram enumeration function will never
> > itself
> > > > try and call PartitionAlloc? Re-entrancy here would be illegal.
> > > 
> > > It can potentially be re-entered, but is it really concerning? Even if
that
> > > happens, it will just reports two UseCounters for the same CommittedSize.
> > 
> > Ah, sorry, I was referring to a different re-entrancy:
> > 
> > The histogramEnumerationFunction() is a very non-trivial function, and it is
> > also subject to implementation changes over time. I'm worried that it might
> call
> > a memory allocation function, and that function might be backed by
> > partitionAlloc(), either now or in the future.
> > 
> > It is very illegal to call back into partitionAlloc() while we are already
> > inside partitionAlloc() ;-)
> > 
> > If it's possible, I think the best design might be to poll the commit usage
at
> > some safe point. Do we have some suitable timer tick or heatbeat location in
> > Blink?
> 
> One way to do that would be using a Timer to trigger
> histogramEnumerationFunction(), but it's not that easy since Timer is in
> platform/ and Partitions is in wtf/ (Note: This dependency issue is going to
be
> solved in the near future by our Blink componentization).
> 
> How about calling Partitions::notifyCommittedMemoryChanged() in Oilpan's GC?
We
> can call both Partitions::notifyCommittedMemoryChanged() and
> Heap::notifyCommittedMemoryChanged() there. The latest CL does that.

^^^ At that point, we're guaranteed to be in a safe point where no allocation on
Oilpan or PartitionAlloc happens.

[Chris Evans, 2015-04-07 00:50:36]

LGTM

Thanks. I think this is safer and a stronger long-term design. Thanks for
hitting a PartitionAlloc code clean-up along the way!

https://codereview.chromium.org/1053793004/diff/140001/Source/wtf/PartitionAl...
File Source/wtf/PartitionAlloc.h (right):

https://codereview.chromium.org/1053793004/diff/140001/Source/wtf/PartitionAl...
Source/wtf/PartitionAlloc.h:672: partitionAllocInit(&m_partitionRoot,
kNumBuckets, kMaxAllocation);
Nit: we no longer need this whitespace-only chance?

https://codereview.chromium.org/1053793004/diff/140001/Source/wtf/PartitionAl...
Source/wtf/PartitionAlloc.h:685: partitionAllocGenericInit(&m_partitionRoot);
Same nit.

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

https://codereview.chromium.org/1053793004/diff/140001/Source/wtf/Partitions....
Source/wtf/Partitions.cpp:75: void Partitions::notifyCommittedMemoryChanged()
Nit: we're no longer responding to a notification callback here (yay!) so
perhaps we could call this "reportPartitionUsageHistogram" or some such?

[haraken, 2015-04-07 01:02:56]

Thanks for review!

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

https://codereview.chromium.org/1053793004/diff/140001/Source/wtf/Partitions....
Source/wtf/Partitions.cpp:75: void Partitions::notifyCommittedMemoryChanged()
On 2015/04/07 00:50:36, Chris Evans wrote:
> Nit: we're no longer responding to a notification callback here (yay!) so
> perhaps we could call this "reportPartitionUsageHistogram" or some such?

Renamed to reportMemoryUsageHistogram and used it in both PartitionAlloc and
Oilpan.

[haraken, 2015-04-07 01:03:00]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-04-07 01:03:01]

The patchset sent to the CQ was uploaded after l-g-t-m from cevans@chromium.org
Link to the patchset: https://codereview.chromium.org/1053793004/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-04-07 01:03:12]

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

[commit-bot: I haz the power, 2015-04-07 04:01:47]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193240