Oilpan: Trace Heap memory statistics more often.

Oilpan: Trace Heap memory statistics more often.

This patch adds more places where Heap statistics are recorded.

The main interest of this patch is to track the change of these values after
a GC; the original code records them in ThreadState::scheduleGCIfNeeded(), but
the function is not called for a while after a GC (until completeSweep() is
called), and we miss the value changes in that period.

BUG=none
R=haraken@chromium.org, oilpan-reviews@chromium.org

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

[haraken, 2015-06-22 04:28:51]

https://codereview.chromium.org/1201773004/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1201773004/diff/1/Source/platform/heap/Heap.c...
Source/platform/heap/Heap.cpp:507: Heap::reportMemoryUsageForTracing();

I think it's better to put Heap::reportMemoryUsageForTracing() in
ThreadState::XXX instead of BaseHeap::XXX. BaseHeap::XXX is called multiple
times (it's called for each heap) by ThreadState::XXX, so you will end up
printing the traced values multiple times.

The same comment for other Heap::reportMemoryUsageForTracing()s in Heap.cpp.

[Yuta Kitamura, 2015-06-22 08:31:52]

https://codereview.chromium.org/1201773004/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1201773004/diff/1/Source/platform/heap/Heap.c...
Source/platform/heap/Heap.cpp:507: Heap::reportMemoryUsageForTracing();
On 2015/06/22 04:28:51, haraken wrote:
> 
> I think it's better to put Heap::reportMemoryUsageForTracing() in
> ThreadState::XXX instead of BaseHeap::XXX. BaseHeap::XXX is called multiple
> times (it's called for each heap) by ThreadState::XXX, so you will end up
> printing the traced values multiple times.
> 
> The same comment for other Heap::reportMemoryUsageForTracing()s in Heap.cpp.

Correct me if I'm wrong, but I think no ThreadState method
is called during BaseHeap::lazySweep() or lazySweepPages().

This patch's point is that I want to look at the value changes
while lazy sweep is in progress. I'd like to capture the cases
where NormalPageHeap::outOfLineAllocate() exits before it
reaches step 6 (threadState()->completeSweep()). I don't think
there is a chance for ThreadState to be able to hook something
for cases where outOfLineAllocate() returns in step 4 or 5.

(With that said, these reportMemoryUsageForTracing() calls
may certainly be redundant; I may be able to remove these
calls in Heap.cpp.)

[haraken, 2015-06-22 08:42:03]

On 2015/06/22 08:31:52, Yuta Kitamura wrote:
>
https://codereview.chromium.org/1201773004/diff/1/Source/platform/heap/Heap.cpp
> File Source/platform/heap/Heap.cpp (right):
> 
>
https://codereview.chromium.org/1201773004/diff/1/Source/platform/heap/Heap.c...
> Source/platform/heap/Heap.cpp:507: Heap::reportMemoryUsageForTracing();
> On 2015/06/22 04:28:51, haraken wrote:
> > 
> > I think it's better to put Heap::reportMemoryUsageForTracing() in
> > ThreadState::XXX instead of BaseHeap::XXX. BaseHeap::XXX is called multiple
> > times (it's called for each heap) by ThreadState::XXX, so you will end up
> > printing the traced values multiple times.
> > 
> > The same comment for other Heap::reportMemoryUsageForTracing()s in Heap.cpp.
> 
> Correct me if I'm wrong, but I think no ThreadState method
> is called during BaseHeap::lazySweep() or lazySweepPages().
> 
> This patch's point is that I want to look at the value changes
> while lazy sweep is in progress. I'd like to capture the cases
> where NormalPageHeap::outOfLineAllocate() exits before it
> reaches step 6 (threadState()->completeSweep()). I don't think
> there is a chance for ThreadState to be able to hook something
> for cases where outOfLineAllocate() returns in step 4 or 5.
> 
> (With that said, these reportMemoryUsageForTracing() calls
> may certainly be redundant; I may be able to remove these
> calls in Heap.cpp.)

Sorry, you're complete right! LGTM.

[haraken, 2015-06-22 08:42:13]

On 2015/06/22 08:42:03, haraken wrote:
> On 2015/06/22 08:31:52, Yuta Kitamura wrote:
> >
>
https://codereview.chromium.org/1201773004/diff/1/Source/platform/heap/Heap.cpp
> > File Source/platform/heap/Heap.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1201773004/diff/1/Source/platform/heap/Heap.c...
> > Source/platform/heap/Heap.cpp:507: Heap::reportMemoryUsageForTracing();
> > On 2015/06/22 04:28:51, haraken wrote:
> > > 
> > > I think it's better to put Heap::reportMemoryUsageForTracing() in
> > > ThreadState::XXX instead of BaseHeap::XXX. BaseHeap::XXX is called
multiple
> > > times (it's called for each heap) by ThreadState::XXX, so you will end up
> > > printing the traced values multiple times.
> > > 
> > > The same comment for other Heap::reportMemoryUsageForTracing()s in
Heap.cpp.
> > 
> > Correct me if I'm wrong, but I think no ThreadState method
> > is called during BaseHeap::lazySweep() or lazySweepPages().
> > 
> > This patch's point is that I want to look at the value changes
> > while lazy sweep is in progress. I'd like to capture the cases
> > where NormalPageHeap::outOfLineAllocate() exits before it
> > reaches step 6 (threadState()->completeSweep()). I don't think
> > there is a chance for ThreadState to be able to hook something
> > for cases where outOfLineAllocate() returns in step 4 or 5.
> > 
> > (With that said, these reportMemoryUsageForTracing() calls
> > may certainly be redundant; I may be able to remove these
> > calls in Heap.cpp.)
> 
> Sorry, you're complete right! LGTM.

completely

[Yuta Kitamura, 2015-06-23 05:14:30]

The CQ bit was checked by yutak@chromium.org

[commit-bot: I haz the power, 2015-06-23 05:14:35]

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

[commit-bot: I haz the power, 2015-06-23 07:25:46]

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