Introduce a way to grab heap stats.

This patch is introducing a way to grab heap stats.
The idea is to monitor the heap regulary and track each object in the heap.
With this data we will be able do draw heap usage diagram.
Where X is time and Y is the number of objects.

BUG=none
TEST=HeapSnapshotObjectsStats

Committed: https://code.google.com/p/v8/source/detail?r=11302

[alexeif, 2012-04-11 13:53:36]

https://chromiumcodereview.appspot.com/10049002/diff/2001/src/profile-generat...
File src/profile-generator.cc (right):

https://chromiumcodereview.appspot.com/10049002/diff/2001/src/profile-generat...
src/profile-generator.cc:1428: void HeapObjectsMap::StartHeapObjectsTracking() {
Do you plan to put something in here?

https://chromiumcodereview.appspot.com/10049002/diff/2001/src/profile-generat...
src/profile-generator.cc:1437: uint64_t start = OS::Ticks();
leftover?

https://chromiumcodereview.appspot.com/10049002/diff/2001/src/profile-generat...
src/profile-generator.cc:1456: uint64_t start = OS::Ticks();
ditto

[mnaganov (inactive), 2012-04-11 14:35:40]

https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8-profiler.h
File include/v8-profiler.h (right):

https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8-profiler...
include/v8-profiler.h:420: static void StartHeapObjectsTracking();
Comments are required.

https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8-profiler...
include/v8-profiler.h:422: static void PushHeapObjectsStats(OutputStream*
stream);
Why "Push"? Maybe "Serialize"?

https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8.h#newcod...
include/v8.h:3745: kUint32 = 1
Any particular reason for not sending this data as ASCII / JSON? This is just a
stream of numbers, AFAICT.

https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8.h#newcod...
include/v8.h:3764: 
nit: no blank line

https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8.h#newcod...
include/v8.h:3766: * Writes the next chunk of snapshot data into the stream.
Writing
Does it really write snapshot data?

https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generat...
File src/profile-generator.cc (right):

https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generat...
src/profile-generator.cc:1409: SnapshotObjectId
HeapObjectsMap::FindOrAddEntry(Address addr) {
This functions seems to reuse code from FindEntry, FindObject and AddEntry. Can
you please try to avoid code duplication?

https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generat...
src/profile-generator.cc:1455: int collected_fragment_updates = 0;
This variable seems to be write-only.

https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generat...
File src/profile-generator.h (right):

https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generat...
src/profile-generator.h:758: struct FragmentInfo {
Please move under EntryInfo definition.

https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generat...
src/profile-generator.h:760: SnapshotObjectId id;
'id' isn't very descriptive. Should it be something like 'last_id'?

https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generat...
src/profile-generator.h:778: void StartHeapObjectsTracking() {
I think, non-trivial methods should be implemented in the .cc file.

https://chromiumcodereview.appspot.com/10049002/diff/4001/test/cctest/test-he...
File test/cctest/test-heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/10049002/diff/4001/test/cctest/test-he...
test/cctest/test-heap-profiler.cc:749: v8::Persistent<v8::String> p_string =
This handle can be local, you have a HandleScope at the function level, so the
string will be kept alive until you return.

https://chromiumcodereview.appspot.com/10049002/diff/4001/test/cctest/test-he...
test/cctest/test-heap-profiler.cc:766: 
I think, you should also test stats update on object deletion.

[alexeif, 2012-04-11 14:38:51]

https://chromiumcodereview.appspot.com/10049002/diff/7001/src/profile-generat...
File src/profile-generator.cc (right):

https://chromiumcodereview.appspot.com/10049002/diff/7001/src/profile-generat...
src/profile-generator.cc:1463: while (entry_info < end_entry_info &&
entry_info->id < fragment_id) {
it doesn't look very efficient.
May be do the binary search or better make entry info know its fragment index
and update counters when its created/deleted. wdyt?

[loislo, 2012-04-12 12:25:29]

On 2012/04/11 14:35:40, Mikhail Naganov (Chromium) wrote:
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8-profiler.h
> File include/v8-profiler.h (right):
> 
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8-profiler...
> include/v8-profiler.h:420: static void StartHeapObjectsTracking();
> Comments are required.

Done.

> 
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8-profiler...
> include/v8-profiler.h:422: static void PushHeapObjectsStats(OutputStream*
> stream);
> Why "Push"? Maybe "Serialize"?

It is not a serialization. I'm not sure that I like this term.

> 
> https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8.h
> File include/v8.h (right):
> 
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8.h#newcod...
> include/v8.h:3745: kUint32 = 1
> Any particular reason for not sending this data as ASCII / JSON? This is just
a
> stream of numbers, AFAICT.

I'd like to upstream serialization part of the profiler code to WebCore.
It will significantly increase our productivity and simplify the business
process.

> 
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8.h#newcod...
> include/v8.h:3764: 
> nit: no blank line

done

> 
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8.h#newcod...
> include/v8.h:3766: * Writes the next chunk of snapshot data into the stream.
> Writing
> Does it really write snapshot data?

done

> 
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generat...
> File src/profile-generator.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generat...
> src/profile-generator.cc:1409: SnapshotObjectId
> HeapObjectsMap::FindOrAddEntry(Address addr) {
> This functions seems to reuse code from FindEntry, FindObject and AddEntry.
Can
> you please try to avoid code duplication?

It has single look-up call.

> 
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generat...
> src/profile-generator.cc:1455: int collected_fragment_updates = 0;
> This variable seems to be write-only.

done.

> 
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generat...
> File src/profile-generator.h (right):
> 
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generat...
> src/profile-generator.h:758: struct FragmentInfo {
> Please move under EntryInfo definition.

done

> 
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generat...
> src/profile-generator.h:760: SnapshotObjectId id;
> 'id' isn't very descriptive. Should it be something like 'last_id'?

mmm

> 
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generat...
> src/profile-generator.h:778: void StartHeapObjectsTracking() {
> I think, non-trivial methods should be implemented in the .cc file.

it is trivial now.

> 
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/test/cctest/test-he...
> File test/cctest/test-heap-profiler.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/test/cctest/test-he...
> test/cctest/test-heap-profiler.cc:749: v8::Persistent<v8::String> p_string =
> This handle can be local, you have a HandleScope at the function level, so the
> string will be kept alive until you return.

done

> 
>
https://chromiumcodereview.appspot.com/10049002/diff/4001/test/cctest/test-he...
> test/cctest/test-heap-profiler.cc:766: 
> I think, you should also test stats update on object deletion.

The code with persistent handles is very complicated. I did that with Local and
HandleScope.

[mnaganov (inactive), 2012-04-12 13:32:26]

https://chromiumcodereview.appspot.com/10049002/diff/14001/include/v8-profiler.h
File include/v8-profiler.h (right):

https://chromiumcodereview.appspot.com/10049002/diff/14001/include/v8-profile...
include/v8-profiler.h:421: * This method gives us an update of the heap state.
Comments look a bit weird. How about putting StartHeapObjectsTracking first, and
annotating it something like this:

"Starts tracking of heap objects population statistics. After calling this
method, all heap objects relocations done by the garbage collector are being
registered."

Then, for PushHeapObjectsStats we can write something like:

"Adds a new time interval entry to the aggregated statistics array. The time
interval entry contains information on the current heap objects population size.
The method also updates aggregated statistics and reports updates for all
previous time intervals via the OutputStream object. Updates on each time
interval are provided as pairs of time interval index and updated heap objects
count.

StartHeapObjectsTracking must be called before the first call to this method."

And for StopHeapObjectsTracking:

"Stops tracking of heap objects population statistics, cleans up all collected
data. StartHeapObjectsTracking must be called again prior to calling
PushHeapObjectsStats next time."

https://chromiumcodereview.appspot.com/10049002/diff/14001/include/v8-profile...
include/v8-profiler.h:428: static void PushHeapObjectsStats(OutputStream*
stream);
nit: add a blank line before the comment

https://chromiumcodereview.appspot.com/10049002/diff/14001/include/v8-profile...
include/v8-profiler.h:433: static void StartHeapObjectsTracking();
nit: add a blank line before the comment

https://chromiumcodereview.appspot.com/10049002/diff/14001/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/10049002/diff/14001/include/v8.h#newco...
include/v8.h:3769: virtual WriteResult WriteUint32Chunk(uint32_t* data, int
size) = 0;
Is this really data size or items count? Looking at the implementation I'd say
that this is count, not size.

https://chromiumcodereview.appspot.com/10049002/diff/14001/src/profile-genera...
File src/profile-generator.cc (right):

https://chromiumcodereview.appspot.com/10049002/diff/14001/src/profile-genera...
src/profile-generator.cc:1453: ASSERT(entries_->length());
entries_->length() != 0 or, alternatively !entries_->is_empty()

https://chromiumcodereview.appspot.com/10049002/diff/14001/src/profile-genera...
src/profile-generator.cc:1456: uint32_t count = 0;
count -> entries_count ?

https://chromiumcodereview.appspot.com/10049002/diff/14001/src/profile-genera...
src/profile-generator.cc:1470: stream->WriteUint32Chunk(&stats_buffer.first(),
Have you considered checking the result and aborting the serialization?

https://chromiumcodereview.appspot.com/10049002/diff/14001/src/profile-genera...
src/profile-generator.cc:1478: if (stats_buffer.length()) {
stats_buffer.length() > 0 or, alternatively !stats_buffer.is_empty()

https://chromiumcodereview.appspot.com/10049002/diff/14001/test/cctest/test-h...
File test/cctest/test-heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/10049002/diff/14001/test/cctest/test-h...
test/cctest/test-heap-profiler.cc:824: CHECK_EQ(2,
stats_update.numbers_written());
Why no check for entries_count?

[loislo, 2012-04-12 13:57:28]

comments addressed

[alexeif, 2012-04-12 14:55:23]

https://chromiumcodereview.appspot.com/10049002/diff/14003/src/profile-genera...
File src/profile-generator.cc (right):

https://chromiumcodereview.appspot.com/10049002/diff/14003/src/profile-genera...
src/profile-generator.cc:1435: int entries = 0;
never used?

https://chromiumcodereview.appspot.com/10049002/diff/14003/src/profile-genera...
src/profile-generator.cc:1456: uint32_t entries_count = 0;
move it inside the loop.

https://chromiumcodereview.appspot.com/10049002/diff/14003/src/profile-genera...
src/profile-generator.cc:1463: ++entries_count;
you can eliminate this ++. just calc the difference between end and start
entry_info.

[loislo, 2012-04-13 05:44:10]

On 2012/04/12 14:55:23, alexeif wrote:
>
https://chromiumcodereview.appspot.com/10049002/diff/14003/src/profile-genera...
> File src/profile-generator.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10049002/diff/14003/src/profile-genera...
> src/profile-generator.cc:1435: int entries = 0;
> never used?

done

> 
>
https://chromiumcodereview.appspot.com/10049002/diff/14003/src/profile-genera...
> src/profile-generator.cc:1456: uint32_t entries_count = 0;
> move it inside the loop.

done 

> 
>
https://chromiumcodereview.appspot.com/10049002/diff/14003/src/profile-genera...
> src/profile-generator.cc:1463: ++entries_count;
> you can eliminate this ++. just calc the difference between end and start
> entry_info.

I've a plan to calculate the size of the objects. That's why I have to iterate
over all the entries.

[mnaganov (inactive), 2012-04-13 08:44:51]

LGTM

https://chromiumcodereview.appspot.com/10049002/diff/16002/test/cctest/test-h...
File test/cctest/test-heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/10049002/diff/16002/test/cctest/test-h...
test/cctest/test-heap-profiler.cc:812: TestStatsStream stats_update =
GetHeapStatsUpdate();
nit: Might be worth adding a comment that prior to getting an update GC is
called, so previously added strings must be collected. Otherwise one needs to
get to the implementation in order to understand this.