Add an interface for an embedder to provide information about native

Add an interface for an embedder to provide information about native
objects retained by object groups and global handles.

This information is then used during heap snapshot generation
to provide a more complete memory picture.

This patch will be needed to fix https://bugs.webkit.org/show_bug.cgi?id=53659.

Committed: http://code.google.com/p/v8/source/detail?r=7125

[mnaganov (inactive), 2011-03-05 18:42:12]

Mads, can you please look at the V8 API part. We don't want to bother you with
implementation details ;)

[Søren Thygesen Gjesse, 2011-03-09 11:38:47]

LGTM

http://codereview.chromium.org/6626043/diff/1/test/cctest/test-heap-profiler.cc
File test/cctest/test-heap-profiler.cc (right):

http://codereview.chromium.org/6626043/diff/1/test/cctest/test-heap-profiler....
test/cctest/test-heap-profiler.cc:1322: 
Missing empty line.

[Vitaly Repeshko, 2011-03-09 14:15:49]

http://codereview.chromium.org/6626043/diff/1/include/v8-profiler.h
File include/v8-profiler.h (right):

http://codereview.chromium.org/6626043/diff/1/include/v8-profiler.h#newcode403
include/v8-profiler.h:403: * the given JavaScript wrapper object.
We should document that its prohibited to enter V8 while the callback is
running: only getters on the handle and GetPointerFromInternalField on the
objects are allowed.

http://codereview.chromium.org/6626043/diff/1/include/v8-profiler.h#newcode426
include/v8-profiler.h:426: /** Binds a callback to embedder's class ID. */
Move the "no class" constant here and document that it must not be used to
define a class and that it could be used to reset a class of a persistent
handle.

http://codereview.chromium.org/6626043/diff/1/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/6626043/diff/1/include/v8.h#newcode400
include/v8.h:400: * Assigns a class ID to the handle. See RetainedObjectInfo
"a class" -> "a wrapper class"

http://codereview.chromium.org/6626043/diff/1/include/v8.h#newcode2549
include/v8.h:2549: * Interface for providing information about embedder's
objects
Is there a good reason to keep this interface here and not in v8-profile.h? Also
please mention in the very first sentence here that the interface is used by the
heap profiler.

http://codereview.chromium.org/6626043/diff/1/include/v8.h#newcode2568
include/v8.h:2568: * Returned RetainedObjectInfo instances are kept alive only
during
Let's change the first sentence to "V8 takes ownership of RetainedObjectInfo
instances passed to it and keeps them alive only during snapshot collection".

http://codereview.chromium.org/6626043/diff/1/include/v8.h#newcode2586
include/v8.h:2586: /** Returns human-readable label. */
Please document the expected encoding (and whether it should be NUL-terminated)
and the ownership of the returned pointer.

http://codereview.chromium.org/6626043/diff/1/src/global-handles.h
File src/global-handles.h (right):

http://codereview.chromium.org/6626043/diff/1/src/global-handles.h#newcode57
src/global-handles.h:57: v8::RetainedObjectInfo* info_;
Needs DISALLOW_COPY_AND_ASSIGN to prevent accidental double dispose of "info".

http://codereview.chromium.org/6626043/diff/1/src/global-handles.h#newcode113
src/global-handles.h:113: // Iterates over all handles that has
embedder-assigned class ID.
"has" -> "have"

http://codereview.chromium.org/6626043/diff/1/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6626043/diff/1/src/heap.h#newcode1144
src/heap.h:1144: static void call_global_gc_prologue_callback() {
CallGlobalGCPrologueCallback()

http://codereview.chromium.org/6626043/diff/1/src/heap.h#newcode1148
src/heap.h:1148: static void call_global_gc_epilogue_callback() {
CallGlobalGCEpilogueCallback()

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.cc
File src/profile-generator.cc (right):

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.cc#newcod...
src/profile-generator.cc:1400: const uint64_t
HeapObjectsMap::kInternalRootObjectId = 1;
Document why these have to be odd.

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.cc#newcod...
src/profile-generator.cc:1684: reinterpret_cast<HeapObject*>(3);
Shouldn't the constants defined above be reused here?

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.cc#newcod...
src/profile-generator.cc:2200: reinterpret_cast<HeapThing>(5);
Same as above.

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.cc#newcod...
src/profile-generator.cc:2243: info->GetLabel(),
What if the returned label's lifetime is tied to the retained object info's
lifetime?

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.h
File src/profile-generator.h (right):

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.h#newcode69
src/profile-generator.h:69: const char* PrintF(const char* format, ...);
"PrintF" is a bad name. We don't actually print anything here. "StringPrintF" or
"AddFormatted"?

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.h#newcode843
src/profile-generator.h:843: const char* GetName(String* name) { return
names_.GetName(name); }
I think it'd be much better to expose the string storage object directly instead
of adding its functions here.

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.h#newcode...
src/profile-generator.h:1083: ~NativeObjectsExplorer();
Please make the implicit "virtual" explicit.

http://codereview.chromium.org/6626043/diff/1/test/cctest/test-heap-profiler.cc
File test/cctest/test-heap-profiler.cc (right):

http://codereview.chromium.org/6626043/diff/1/test/cctest/test-heap-profiler....
test/cctest/test-heap-profiler.cc:1299: return new TestRetainedObjectInfo(1,
"aaa", 100);
"bbb"?

[mnaganov (inactive), 2011-03-09 15:26:32]

http://codereview.chromium.org/6626043/diff/1/include/v8-profiler.h
File include/v8-profiler.h (right):

http://codereview.chromium.org/6626043/diff/1/include/v8-profiler.h#newcode403
include/v8-profiler.h:403: * the given JavaScript wrapper object.
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> We should document that its prohibited to enter V8 while the callback is
> running: only getters on the handle and GetPointerFromInternalField on the
> objects are allowed.

Done.

http://codereview.chromium.org/6626043/diff/1/include/v8-profiler.h#newcode426
include/v8-profiler.h:426: /** Binds a callback to embedder's class ID. */
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> Move the "no class" constant here and document that it must not be used to
> define a class and that it could be used to reset a class of a persistent
> handle.

Done.

http://codereview.chromium.org/6626043/diff/1/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/6626043/diff/1/include/v8.h#newcode400
include/v8.h:400: * Assigns a class ID to the handle. See RetainedObjectInfo
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> "a class" -> "a wrapper class"

Done.

http://codereview.chromium.org/6626043/diff/1/include/v8.h#newcode2549
include/v8.h:2549: * Interface for providing information about embedder's
objects
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> Is there a good reason to keep this interface here and not in v8-profile.h?
Also
> please mention in the very first sentence here that the interface is used by
the
> heap profiler.

After I have moved to v8-profiler.h near HeapProfiler this is no more needed, I
think.

http://codereview.chromium.org/6626043/diff/1/include/v8.h#newcode2568
include/v8.h:2568: * Returned RetainedObjectInfo instances are kept alive only
during
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> Let's change the first sentence to "V8 takes ownership of RetainedObjectInfo
> instances passed to it and keeps them alive only during snapshot collection".

Done.

http://codereview.chromium.org/6626043/diff/1/include/v8.h#newcode2586
include/v8.h:2586: /** Returns human-readable label. */
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> Please document the expected encoding (and whether it should be
NUL-terminated)
> and the ownership of the returned pointer.

Done.

http://codereview.chromium.org/6626043/diff/1/src/global-handles.h
File src/global-handles.h (right):

http://codereview.chromium.org/6626043/diff/1/src/global-handles.h#newcode57
src/global-handles.h:57: v8::RetainedObjectInfo* info_;
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> Needs DISALLOW_COPY_AND_ASSIGN to prevent accidental double dispose of "info".

Done.

http://codereview.chromium.org/6626043/diff/1/src/global-handles.h#newcode113
src/global-handles.h:113: // Iterates over all handles that has
embedder-assigned class ID.
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> "has" -> "have"

Done.

http://codereview.chromium.org/6626043/diff/1/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6626043/diff/1/src/heap.h#newcode1144
src/heap.h:1144: static void call_global_gc_prologue_callback() {
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> CallGlobalGCPrologueCallback()

Done.

http://codereview.chromium.org/6626043/diff/1/src/heap.h#newcode1148
src/heap.h:1148: static void call_global_gc_epilogue_callback() {
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> CallGlobalGCEpilogueCallback()

Done.

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.cc
File src/profile-generator.cc (right):

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.cc#newcod...
src/profile-generator.cc:1400: const uint64_t
HeapObjectsMap::kInternalRootObjectId = 1;
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> Document why these have to be odd.

Done.

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.cc#newcod...
src/profile-generator.cc:1684: reinterpret_cast<HeapObject*>(3);
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> Shouldn't the constants defined above be reused here?

Done.

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.cc#newcod...
src/profile-generator.cc:2200: reinterpret_cast<HeapThing>(5);
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> Same as above.

Done.

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.cc#newcod...
src/profile-generator.cc:2243: info->GetLabel(),
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> What if the returned label's lifetime is tied to the retained object info's
> lifetime?

Fixed.

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.h
File src/profile-generator.h (right):

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.h#newcode69
src/profile-generator.h:69: const char* PrintF(const char* format, ...);
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> "PrintF" is a bad name. We don't actually print anything here. "StringPrintF"
or
> "AddFormatted"?

Renamed to GetFormatted and GetVFormatted, as it doesn't always add -- it can
return a existing string.

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.h#newcode843
src/profile-generator.h:843: const char* GetName(String* name) { return
names_.GetName(name); }
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> I think it'd be much better to expose the string storage object directly
instead
> of adding its functions here.

Right. There are now too many functions to export.

http://codereview.chromium.org/6626043/diff/1/src/profile-generator.h#newcode...
src/profile-generator.h:1083: ~NativeObjectsExplorer();
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> Please make the implicit "virtual" explicit.

Done.

http://codereview.chromium.org/6626043/diff/1/test/cctest/test-heap-profiler.cc
File test/cctest/test-heap-profiler.cc (right):

http://codereview.chromium.org/6626043/diff/1/test/cctest/test-heap-profiler....
test/cctest/test-heap-profiler.cc:1299: return new TestRetainedObjectInfo(1,
"aaa", 100);
On 2011/03/09 14:15:49, Vitaly Repeshko wrote:
> "bbb"?

No. I'm intentionally returning an equivalent object.

http://codereview.chromium.org/6626043/diff/1/test/cctest/test-heap-profiler....
test/cctest/test-heap-profiler.cc:1322: 
On 2011/03/09 11:38:47, Søren Gjesse wrote:
> Missing empty line.

Done.

[Vitaly Repeshko, 2011-03-09 19:10:08]

LGTM

http://codereview.chromium.org/6626043/diff/3002/src/heap-profiler.cc
File src/heap-profiler.cc (right):

http://codereview.chromium.org/6626043/diff/3002/src/heap-profiler.cc#newcode368
src/heap-profiler.cc:368: uint16_t class_id,
v8::HeapProfiler::WrapperInfoCallback callback) {
Assert class_id is not kNoClassId.

http://codereview.chromium.org/6626043/diff/3002/src/profile-generator.h
File src/profile-generator.h (right):

http://codereview.chromium.org/6626043/diff/3002/src/profile-generator.h#newc...
src/profile-generator.h:683: HeapEntry* dom_subtrees_root() { return
dom_subtrees_root_entry_; }
"dom" -> "native"?

[mnaganov (inactive), 2011-03-10 12:23:35]

Committed separately as http://code.google.com/p/v8/source/detail?r=7126

http://codereview.chromium.org/6626043/diff/3002/src/heap-profiler.cc
File src/heap-profiler.cc (right):

http://codereview.chromium.org/6626043/diff/3002/src/heap-profiler.cc#newcode368
src/heap-profiler.cc:368: uint16_t class_id,
v8::HeapProfiler::WrapperInfoCallback callback) {
On 2011/03/09 19:10:08, Vitaly Repeshko wrote:
> Assert class_id is not kNoClassId.

Done.

http://codereview.chromium.org/6626043/diff/3002/src/profile-generator.h
File src/profile-generator.h (right):

http://codereview.chromium.org/6626043/diff/3002/src/profile-generator.h#newc...
src/profile-generator.h:683: HeapEntry* dom_subtrees_root() { return
dom_subtrees_root_entry_; }
On 2011/03/09 19:10:08, Vitaly Repeshko wrote:
> "dom" -> "native"?

Done.