Oilpan: Refactor the way we calculate heap statistics

Oilpan: Refactor the way we calculate heap statistics

This is a preparation for implementing incremental sweeping.

- Remove ThreadState::HeapStats.

- Introduce Heap::allocatedSpace, Heap::allocatedObjectSize and Heap::liveObjectSize.

--- Heap::allocatedSpace keeps track of the size of allocated space.

--- Heap::allocatedObjectSize keeps track of the size of objects allocated since the last GC.

--- Heap::liveObjectSize keeps track of the size of marked objects in the last GC.

- Adds a couple of atomic instructions to Atomics.h to update the Heap metrics atomically.

- This CL fixes the issue that shouldGC() determines GC timing only based on the size of the current thread heap. After this CL, shouldGC() determines GC timing based on the entire heap.

- Rename getStatsForTesting() to objectPayloadSizeForTesting().

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

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

[haraken, 2014-11-12 11:30:03]

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

[haraken, 2014-11-12 11:30:04]

This CL is not yet for review.

Probably we should not move the HeapStats calculation to the marking phase,
because we have to make the marking phase as light-weight as possible.

[haraken, 2014-11-14 06:05:23]

OK, this CL is now ready for review. PTAL.

This CL just refactors the way we calculate heap statictics and doesn't change
behavior.

(This CL might be a bit too big to review. Feel free to say so, I'll split the
CL into pieces.)

[haraken, 2014-11-14 06:17:17]

On 2014/11/14 06:05:23, haraken wrote:
> OK, this CL is now ready for review. PTAL.
> 
> This CL just refactors the way we calculate heap statictics and doesn't change
> behavior.
> 
> (This CL might be a bit too big to review. Feel free to say so, I'll split the
> CL into pieces.)

Started splitting.

[haraken, 2014-11-14 06:52:00]

On 2014/11/14 06:17:17, haraken wrote:
> On 2014/11/14 06:05:23, haraken wrote:
> > OK, this CL is now ready for review. PTAL.
> > 
> > This CL just refactors the way we calculate heap statictics and doesn't
change
> > behavior.
> > 
> > (This CL might be a bit too big to review. Feel free to say so, I'll split
the
> > CL into pieces.)
> 
> Started splitting.

Hmm, it looks hard to split the CL any more (other than
https://codereview.chromium.org/728753002/). So... PTAL.

[sof, 2014-11-14 12:44:19]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2014-11-14 12:44:20]

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (left):

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:1389:
stats->increaseAllocatedSpace(current->size());
Could you explain why we don't need to increase the allocated size now?

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:2190: s_allocatedObjectSize = 0;
Use resetAllocatedObjectSize() ?

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:2192: s_liveObjectSize = 0;
Use resetLiveObjectSize() ?

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:991: static void resetAllocatedObjectSize() {
s_allocatedObjectSize = 0; }
Add a comment to make it clearer why this doesn't have to be an atomic operation
also?

[haraken, 2014-11-14 15:15:00]

Thanks for review!

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (left):

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:1389:
stats->increaseAllocatedSpace(current->size());
On 2014/11/14 12:44:20, sof wrote:
> Could you explain why we don't need to increase the allocated size now?

After this CL, s_allocatedSpace is never reset or increased during sweeping.
s_allocatedSpace is initialized to 0 when the heap is initialized, and
increased/decreased when actually allocating/deallocating normal pages or large
objects. Then when the heap is shut down, we verify that s_allocatedSpace is
back to 0.

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:2190: s_allocatedObjectSize = 0;
On 2014/11/14 12:44:20, sof wrote:
> Use resetAllocatedObjectSize() ?

Because I added ASSERT(ThreadState::isAnyThreadInGC()) to
resetAllocatedObjectSize(), I cannot use resetAllocatedObjectSize() here.

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:991: static void resetAllocatedObjectSize() {
s_allocatedObjectSize = 0; }
On 2014/11/14 12:44:20, sof wrote:
> Add a comment to make it clearer why this doesn't have to be an atomic
operation
> also?

Added ASSERT(ThreadState::isAnyThreadInGC()).

[sof, 2014-11-14 16:37:21]

lgtm

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (left):

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:1389:
stats->increaseAllocatedSpace(current->size());
On 2014/11/14 15:15:00, haraken wrote:
> On 2014/11/14 12:44:20, sof wrote:
> > Could you explain why we don't need to increase the allocated size now?
> 
> After this CL, s_allocatedSpace is never reset or increased during sweeping.
> s_allocatedSpace is initialized to 0 when the heap is initialized, and
> increased/decreased when actually allocating/deallocating normal pages or
large
> objects. Then when the heap is shut down, we verify that s_allocatedSpace is
> back to 0.

Thanks for clarifying.

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:991: static void resetAllocatedObjectSize() {
s_allocatedObjectSize = 0; }
On 2014/11/14 15:15:00, haraken wrote:
> On 2014/11/14 12:44:20, sof wrote:
> > Add a comment to make it clearer why this doesn't have to be an atomic
> operation
> > also?
> 
> Added ASSERT(ThreadState::isAnyThreadInGC()).

An assert is even better. I suppose there isn't a finer-grained condition to
test for here.

Could we make some of these static methods private?

[haraken, 2014-11-17 01:05:02]

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

[haraken, 2014-11-17 01:05:02]

tkent-san@: Would you take a look at wtf/ ?

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/723513002/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:991: static void resetAllocatedObjectSize() {
s_allocatedObjectSize = 0; }
On 2014/11/14 16:37:21, sof wrote:
> On 2014/11/14 15:15:00, haraken wrote:
> > On 2014/11/14 12:44:20, sof wrote:
> > > Add a comment to make it clearer why this doesn't have to be an atomic
> > operation
> > > also?
> > 
> > Added ASSERT(ThreadState::isAnyThreadInGC()).
> 
> An assert is even better. I suppose there isn't a finer-grained condition to
> test for here.
> 
> Could we make some of these static methods private?

Done.

[tkent, 2014-11-17 01:22:53]

On 2014/11/17 01:05:02, haraken wrote:
> tkent-san@: Would you take a look at wtf/ ?

Please upload a buildable patch first.

[haraken, 2014-11-17 09:42:24]

On 2014/11/17 01:22:53, tkent wrote:
> On 2014/11/17 01:05:02, haraken wrote:
> > tkent-san@: Would you take a look at wtf/ ?
> 
> Please upload a buildable patch first.

tkent-san@: The patch set 11 seems buildable. Would you take a look at wtf/?

[tkent, 2014-11-17 10:30:15]

> --- Heap::liveObjectSize keeps track of the size of marked objects in the last
GC.

nit:  I feel |live| is confusing.  markedObjectSize?
It's up to you.

https://codereview.chromium.org/723513002/diff/200001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/723513002/diff/200001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:1105: m_lowCollectionRate =
Heap::liveObjectSize() > (allocatedObjectSizeBeforeSweeping / 2);
Other threads might not finish sweeping yet, and Heap::liveObjectSize might has
a smaller value.  Is it ok?

https://codereview.chromium.org/723513002/diff/200001/Source/wtf/Atomics.h
File Source/wtf/Atomics.h (right):

https://codereview.chromium.org/723513002/diff/200001/Source/wtf/Atomics.h#ne...
Source/wtf/Atomics.h:57: return InterlockedExchangeAdd(reinterpret_cast<LONG
volatile*>(addend), static_cast<LONG>(increment)) + increment;
Replacing |long| with |LONG| is inconsistent with atomicIncrement() and
atomicDecrement().

https://codereview.chromium.org/723513002/diff/200001/Source/wtf/Atomics.h#ne...
Source/wtf/Atomics.h:61: return InterlockedExchangeAdd(reinterpret_cast<ULONG
volatile*>(addend), static_cast<ULONG>(increment)) + increment;
Why do you cast to ULONG?  The arguments of InterlockedExchangeAdd are |LONG
volatile*| and LONG.

https://codereview.chromium.org/723513002/diff/200001/Source/wtf/Atomics.h#ne...
Source/wtf/Atomics.h:66: return
InterlockedExchangeAdd64(reinterpret_cast<ULONGLONG volatile*>(addend),
static_cast<ULONGLONG>(increment)) + increment;
Ditto.  |LONGLONG volatile*| and |LONGLONG|.

[haraken, 2014-11-17 10:43:36]

Thanks for review.

> nit:  I feel |live| is confusing.  markedObjectSize?

Done.

https://codereview.chromium.org/723513002/diff/200001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/723513002/diff/200001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:1105: m_lowCollectionRate =
Heap::liveObjectSize() > (allocatedObjectSizeBeforeSweeping / 2);
On 2014/11/17 10:30:15, tkent wrote:
> Other threads might not finish sweeping yet, and Heap::liveObjectSize might
has
> a smaller value.  Is it ok?

m_lowCollectionRate is just a heurstic anyway, so I think it is OK. Added a
FIXME about it.

https://codereview.chromium.org/723513002/diff/200001/Source/wtf/Atomics.h
File Source/wtf/Atomics.h (right):

https://codereview.chromium.org/723513002/diff/200001/Source/wtf/Atomics.h#ne...
Source/wtf/Atomics.h:57: return InterlockedExchangeAdd(reinterpret_cast<LONG
volatile*>(addend), static_cast<LONG>(increment)) + increment;
On 2014/11/17 10:30:15, tkent wrote:
> Replacing |long| with |LONG| is inconsistent with atomicIncrement() and
> atomicDecrement().

Done.

https://codereview.chromium.org/723513002/diff/200001/Source/wtf/Atomics.h#ne...
Source/wtf/Atomics.h:61: return InterlockedExchangeAdd(reinterpret_cast<ULONG
volatile*>(addend), static_cast<ULONG>(increment)) + increment;
On 2014/11/17 10:30:15, tkent wrote:
> Why do you cast to ULONG?  The arguments of InterlockedExchangeAdd are |LONG
> volatile*| and LONG.

Fixed.

https://codereview.chromium.org/723513002/diff/200001/Source/wtf/Atomics.h#ne...
Source/wtf/Atomics.h:66: return
InterlockedExchangeAdd64(reinterpret_cast<ULONGLONG volatile*>(addend),
static_cast<ULONGLONG>(increment)) + increment;
On 2014/11/17 10:30:15, tkent wrote:
> Ditto.  |LONGLONG volatile*| and |LONGLONG|.

Done.

[tkent, 2014-11-17 10:51:58]

lgtm.

It's possible that this CL causes performance regression on some platforms. 
Please compare benchmark results before and after this CL.

[haraken, 2014-11-17 10:58:20]

> It's possible that this CL causes performance regression on some platforms. 
> Please compare benchmark results before and after this CL.

What's your concern?

[tkent, 2014-11-17 11:00:56]

On 2014/11/17 10:58:20, haraken wrote:
> > It's possible that this CL causes performance regression on some platforms. 
> > Please compare benchmark results before and after this CL.
> 
> What's your concern?

__sync_*_and_fetch might be implemented with a global mutex if CPU can't support
them natively.

[haraken, 2014-11-17 11:07:10]

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

[haraken, 2014-11-17 11:07:10]

On 2014/11/17 11:00:56, tkent wrote:
> On 2014/11/17 10:58:20, haraken wrote:
> > > It's possible that this CL causes performance regression on some
platforms. 
> > > Please compare benchmark results before and after this CL.
> > 
> > What's your concern?
> 
> __sync_*_and_fetch might be implemented with a global mutex if CPU can't
support
> them natively.

Hmm, if that is the case, it would be problematic because
Heap::increaseAllocatedObjectSize() is called in ensureCurrentAllocation(). Also
the performance issue won't show up in my local environment where CPU supports
atomic operations.

I guess a better solution would be to ask Slava :)

Slava: In modern CPU architectures, is it possible that __sync_*_and_fetch is
implemented with a global mutex?

[tkent, 2014-11-17 11:32:07]

https://gcc.gnu.org/viewcvs/gcc/trunk/libgcc/config/arm/linux-atomic.c?revisi...

Looks like ARM implementation needs a helper function provided by the kernel. 
I'm not sure how expensive it is.

[haraken, 2014-11-17 11:33:24]

On 2014/11/17 11:32:07, tkent wrote:
>
https://gcc.gnu.org/viewcvs/gcc/trunk/libgcc/config/arm/linux-atomic.c?revisi...
> 
> Looks like ARM implementation needs a helper function provided by the kernel. 
> I'm not sure how expensive it is.

Thanks, it looks like we need to get numbers from Android.

[haraken, 2014-11-18 09:23:27]

On 2014/11/17 11:33:24, haraken wrote:
> On 2014/11/17 11:32:07, tkent wrote:
> >
>
https://gcc.gnu.org/viewcvs/gcc/trunk/libgcc/config/arm/linux-atomic.c?revisi...
> > 
> > Looks like ARM implementation needs a helper function provided by the
kernel. 
> > I'm not sure how expensive it is.
> 
> Thanks, it looks like we need to get numbers from Android.

I experimented with Nexus7 and observed no regression. So let me land this.

[haraken, 2014-11-18 09:23:31]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-11-18 09:24:21]

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

[commit-bot: I haz the power, 2014-11-18 11:19:14]

Committed patchset #12 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185513

[Michael Achenbach, 2014-11-18 11:48:16]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/738773003/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Breaks compilation on win x64:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20x64%20Bui....

[haraken, 2014-11-19 03:57:59]

On 2014/11/18 11:48:16, Michael Achenbach wrote:
> A revert of this CL (patchset #12 id:220001) has been created in
> https://codereview.chromium.org/738773003/ by mailto:machenbach@chromium.org.
> 
> The reason for reverting is: [Sheriff] Breaks compilation on win x64:
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20x64%20Bui....

Fixed the build failure. Re-landing.

[haraken, 2014-11-19 03:58:04]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-11-19 03:58:11]

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

[commit-bot: I haz the power, 2014-11-19 04:22:58]

Committed patchset #13 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185569