Set HistogramEnumerationFunction even when Partitions are initialized

Set HistogramEnumerationFunction even when Partitions are initialized

PartitionAlloc.CommittedSize isn't collected because we don't set
m_histogramEnumeration correctly when we try to allocate memory
before Partitions::initialize() is called. As a workaround, this CL
separates partitions initialization and m_histogramEnumeration setting
so that we can overrides m_histogramEnumeration. This workaround should
be removed when we can make sure that initialize() is always called
before memory allocation.

This CL also remove a spin lock because initialization must happen
on the main thread.

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

[bashi, 2015-06-17 04:54:06]

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

[bashi, 2015-06-17 04:54:06]

PTAL?

I think https://codereview.chromium.org/1182083006/ fixes the issue but we might
want to have a workaround.

[haraken, 2015-06-17 05:24:20]

Thanks for working on this!

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

https://codereview.chromium.org/1190883003/diff/1/Source/wtf/Partitions.cpp#n...
Source/wtf/Partitions.cpp:65: spinLockLock(&s_lock);

I don't think this lock is needed. Partitions::setHistogramEnumeration can be
called only from the main thread. Also it is guaranteed that WTF::initialize()
is called before the m_histogramEnumeration gets used. Thus there is no
threading contention on the m_histogramEnumeration.

[bashi, 2015-06-17 05:25:27]

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

https://codereview.chromium.org/1190883003/diff/1/Source/wtf/Partitions.cpp#n...
Source/wtf/Partitions.cpp:65: spinLockLock(&s_lock);
On 2015/06/17 05:24:20, haraken wrote:
> 
> I don't think this lock is needed. Partitions::setHistogramEnumeration can be
> called only from the main thread. Also it is guaranteed that WTF::initialize()
> is called before the m_histogramEnumeration gets used. Thus there is no
> threading contention on the m_histogramEnumeration.
> 
> 

Hmm, the same can be said for initialize()? Can we remove the lock totally?

[haraken, 2015-06-17 05:29:37]

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

https://codereview.chromium.org/1190883003/diff/1/Source/wtf/Partitions.cpp#n...
Source/wtf/Partitions.cpp:65: spinLockLock(&s_lock);
On 2015/06/17 05:25:27, bashi1 wrote:
> On 2015/06/17 05:24:20, haraken wrote:
> > 
> > I don't think this lock is needed. Partitions::setHistogramEnumeration can
be
> > called only from the main thread. Also it is guaranteed that
WTF::initialize()
> > is called before the m_histogramEnumeration gets used. Thus there is no
> > threading contention on the m_histogramEnumeration.
> > 
> > 
> 
> Hmm, the same can be said for initialize()? Can we remove the lock totally?

I think your understanding is correct. Any Blink thread won't start before the
main thread finishes WTF::initialize(), it doesn't happen that multiple threads
contend on s_initialized. (If that happens, it will already be detected as a
threading contention because Partitions::bufferPartition() is touching
s_initialized without acquiring a lock.) So I think it should be safe to remove
the lock.

[bashi, 2015-06-17 05:45:37]

Patchset #2 (id:20001) has been deleted

[bashi, 2015-06-17 05:49:41]

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

https://codereview.chromium.org/1190883003/diff/1/Source/wtf/Partitions.cpp#n...
Source/wtf/Partitions.cpp:65: spinLockLock(&s_lock);
On 2015/06/17 05:29:37, haraken wrote:
> On 2015/06/17 05:25:27, bashi1 wrote:
> > On 2015/06/17 05:24:20, haraken wrote:
> > > 
> > > I don't think this lock is needed. Partitions::setHistogramEnumeration can
> be
> > > called only from the main thread. Also it is guaranteed that
> WTF::initialize()
> > > is called before the m_histogramEnumeration gets used. Thus there is no
> > > threading contention on the m_histogramEnumeration.
> > > 
> > > 
> > 
> > Hmm, the same can be said for initialize()? Can we remove the lock totally?
> 
> I think your understanding is correct. Any Blink thread won't start before the
> main thread finishes WTF::initialize(), it doesn't happen that multiple
threads
> contend on s_initialized. (If that happens, it will already be detected as a
> threading contention because Partitions::bufferPartition() is touching
> s_initialized without acquiring a lock.) So I think it should be safe to
remove
> the lock.
> 

Removed. We can't add ASSERT(isMainThread()) because isMainThread() doesn't work
at this point. (mainThreadIdendifier isn't initialized at this point)

[haraken, 2015-06-17 14:18:44]

LGTM

On 2015/06/17 05:49:41, bashi1 wrote:
> https://codereview.chromium.org/1190883003/diff/1/Source/wtf/Partitions.cpp
> File Source/wtf/Partitions.cpp (right):
> 
>
https://codereview.chromium.org/1190883003/diff/1/Source/wtf/Partitions.cpp#n...
> Source/wtf/Partitions.cpp:65: spinLockLock(&s_lock);
> On 2015/06/17 05:29:37, haraken wrote:
> > On 2015/06/17 05:25:27, bashi1 wrote:
> > > On 2015/06/17 05:24:20, haraken wrote:
> > > > 
> > > > I don't think this lock is needed. Partitions::setHistogramEnumeration
can
> > be
> > > > called only from the main thread. Also it is guaranteed that
> > WTF::initialize()
> > > > is called before the m_histogramEnumeration gets used. Thus there is no
> > > > threading contention on the m_histogramEnumeration.
> > > > 
> > > > 
> > > 
> > > Hmm, the same can be said for initialize()? Can we remove the lock
totally?
> > 
> > I think your understanding is correct. Any Blink thread won't start before
the
> > main thread finishes WTF::initialize(), it doesn't happen that multiple
> threads
> > contend on s_initialized. (If that happens, it will already be detected as a
> > threading contention because Partitions::bufferPartition() is touching
> > s_initialized without acquiring a lock.) So I think it should be safe to
> remove
> > the lock.
> > 
> 
> Removed. We can't add ASSERT(isMainThread()) because isMainThread() doesn't
work
> at this point. (mainThreadIdendifier isn't initialized at this point)

This is good. We want to keep PartitionAlloc as isoalted as possible, so we
don't want to add an unnecessary dependency here.

[haraken, 2015-06-17 14:20:33]

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

https://codereview.chromium.org/1190883003/diff/40001/Source/wtf/Partitions.c...
Source/wtf/Partitions.cpp:52: // rely on isMainThread() at this point.

Sorry, I noticed this file is already including MainThread.h. But I think you
can remove this comment, since it's not a big deal.

[haraken, 2015-06-17 14:21:12]

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

[haraken, 2015-06-17 14:21:13]

+cevans for owner review.

[Chris Evans, 2015-06-17 16:36:36]

On 2015/06/17 14:21:13, haraken wrote:
> +cevans for owner review.

I don't quite understand what this CL changes? Is this a first CL in a series,
or is it supposed to have an effect on its own?
The call sequence change in WTF.cpp from initialize(histogram); to initialize();
setHistogram(histogram) looks equivalent to me?

[haraken, 2015-06-17 16:42:21]

On 2015/06/17 16:36:36, Chris Evans wrote:
> On 2015/06/17 14:21:13, haraken wrote:
> > +cevans for owner review.
> 
> I don't quite understand what this CL changes? Is this a first CL in a series,
> or is it supposed to have an effect on its own?
> The call sequence change in WTF.cpp from initialize(histogram); to
initialize();
> setHistogram(histogram) looks equivalent to me?

Currnetly initialize() (without the histogram) is called before
WTF::initialize() calls initialize(histogram).

Before this CL:

void WTF::initialize() {
  Partitions::initialize(histogram);  // This does nothing because
Partitions::initialize() is already called. Thus we failed in setting the
histogram function.
}

After hit CL:

void WTF::initialize() {
  Partitions::initialize();  // This does nothing because
Partitions::initialize() is already called.
  Partitions::setHistogramFunction(histogramFunction);  // This sets the
histogram function.
}

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

On 2015/06/17 16:42:21, haraken wrote:
> On 2015/06/17 16:36:36, Chris Evans wrote:
> > On 2015/06/17 14:21:13, haraken wrote:
> > > +cevans for owner review.
> > 
> > I don't quite understand what this CL changes? Is this a first CL in a
series,
> > or is it supposed to have an effect on its own?
> > The call sequence change in WTF.cpp from initialize(histogram); to
> initialize();
> > setHistogram(histogram) looks equivalent to me?
> 
> Currnetly initialize() (without the histogram) is called before
> WTF::initialize() calls initialize(histogram).
> 
> Before this CL:
> 
> void WTF::initialize() {
>   Partitions::initialize(histogram);  // This does nothing because
> Partitions::initialize() is already called. Thus we failed in setting the
> histogram function.
> }
> 
> After hit CL:
> 
> void WTF::initialize() {
>   Partitions::initialize();  // This does nothing because
> Partitions::initialize() is already called.
>   Partitions::setHistogramFunction(histogramFunction);  // This sets the
> histogram function.
> }

Thanks! In the case where Partitions::initialize() has already been called,
where is the call site that called it? I didn't see it and I'd like to
understand it :)

[haraken, 2015-06-17 17:45:07]

On 2015/06/17 17:04:05, Chris Evans wrote:
> On 2015/06/17 16:42:21, haraken wrote:
> > On 2015/06/17 16:36:36, Chris Evans wrote:
> > > On 2015/06/17 14:21:13, haraken wrote:
> > > > +cevans for owner review.
> > > 
> > > I don't quite understand what this CL changes? Is this a first CL in a
> series,
> > > or is it supposed to have an effect on its own?
> > > The call sequence change in WTF.cpp from initialize(histogram); to
> > initialize();
> > > setHistogram(histogram) looks equivalent to me?
> > 
> > Currnetly initialize() (without the histogram) is called before
> > WTF::initialize() calls initialize(histogram).
> > 
> > Before this CL:
> > 
> > void WTF::initialize() {
> >   Partitions::initialize(histogram);  // This does nothing because
> > Partitions::initialize() is already called. Thus we failed in setting the
> > histogram function.
> > }
> > 
> > After hit CL:
> > 
> > void WTF::initialize() {
> >   Partitions::initialize();  // This does nothing because
> > Partitions::initialize() is already called.
> >   Partitions::setHistogramFunction(histogramFunction);  // This sets the
> > histogram function.
> > }
> 
> Thanks! In the case where Partitions::initialize() has already been called,
> where is the call site that called it? I didn't see it and I'd like to
> understand it :)

This is one place bashi@ found: https://codereview.chromium.org/1182083006/. I'm
not sure if there are other places.

It's important to enable the UMA asap, so we're planning to land this CL.

[Chris Evans, 2015-06-17 18:00:16]

On 2015/06/17 17:45:07, haraken wrote:
> On 2015/06/17 17:04:05, Chris Evans wrote:
> > On 2015/06/17 16:42:21, haraken wrote:
> > > On 2015/06/17 16:36:36, Chris Evans wrote:
> > > > On 2015/06/17 14:21:13, haraken wrote:
> > > > > +cevans for owner review.
> > > > 
> > > > I don't quite understand what this CL changes? Is this a first CL in a
> > series,
> > > > or is it supposed to have an effect on its own?
> > > > The call sequence change in WTF.cpp from initialize(histogram); to
> > > initialize();
> > > > setHistogram(histogram) looks equivalent to me?
> > > 
> > > Currnetly initialize() (without the histogram) is called before
> > > WTF::initialize() calls initialize(histogram).
> > > 
> > > Before this CL:
> > > 
> > > void WTF::initialize() {
> > >   Partitions::initialize(histogram);  // This does nothing because
> > > Partitions::initialize() is already called. Thus we failed in setting the
> > > histogram function.
> > > }
> > > 
> > > After hit CL:
> > > 
> > > void WTF::initialize() {
> > >   Partitions::initialize();  // This does nothing because
> > > Partitions::initialize() is already called.
> > >   Partitions::setHistogramFunction(histogramFunction);  // This sets the
> > > histogram function.
> > > }
> > 
> > Thanks! In the case where Partitions::initialize() has already been called,
> > where is the call site that called it? I didn't see it and I'd like to
> > understand it :)
> 
> This is one place bashi@ found: https://codereview.chromium.org/1182083006/.
I'm
> not sure if there are other places.
> 
> It's important to enable the UMA asap, so we're planning to land this CL.

Thank you. I understand this now.
I agree that making sure that initialize() is called prior to any allocations
would be awesome. I like the comments to the effect that you added in
Partitions.h. Getting rid of those initialization branches in that inlined code
might even speed things up a little :)

LGTM

[bashi, 2015-06-18 22:53:37]

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

https://codereview.chromium.org/1190883003/diff/40001/Source/wtf/Partitions.c...
Source/wtf/Partitions.cpp:52: // rely on isMainThread() at this point.
On 2015/06/17 14:20:33, haraken wrote:
> 
> Sorry, I noticed this file is already including MainThread.h. But I think you
> can remove this comment, since it's not a big deal.
> 

Removed.

[bashi, 2015-06-18 22:53:42]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-06-18 22:53:43]

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

[commit-bot: I haz the power, 2015-06-18 22:54:41]

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

[commit-bot: I haz the power, 2015-06-19 00:18:39]

Committed patchset #3 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197413