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#newcode1400 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#newcode1684 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#newcode2200 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#newcode2243 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#newcode1083 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.cc#newcode1299 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.cc#newcode1322 test/cctest/test-heap-profiler.cc:1322: On 2011/03/09 11:38:47, Søren Gjesse wrote:
Missing empty line.
Done. http://codereview.chromium.org/6626043/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
