A few comments and a general question:
Is it a problem for your tool if allocations of many objects happen using a
single allocation operation or is that fine? For example we allocate object
A,
B, and C together allocate(sizeof(A,B,C)) and then align the object start
pointers at runtime. I am not sure if you are interested in object
granularity
or allocation site granularity.
https://codereview.chromium.org/22852024/diff/84001/include/v8-profiler.h
File include/v8-profiler.h (right):
https://codereview.chromium.org/22852024/diff/84001/include/v8-profiler.h#newcode478
include/v8-profiler.h:478: * and tracking of heap objects population
statistics.
Please fill up the 80 characters line width.
https://codereview.chromium.org/22852024/diff/84001/include/v8-profiler.h#newcode485
include/v8-profiler.h:485: * population statistics data.
Please fill up the 80 characters line width.
https://codereview.chromium.org/22852024/diff/84001/src/heap-profiler.cc
File src/heap-profiler.cc (right):
https://codereview.chromium.org/22852024/diff/84001/src/heap-profiler.cc#newcode144
src/heap-profiler.cc:144: }
If you unify the implementation in snapshot, please do the same thing
here.
https://codereview.chromium.org/22852024/diff/84001/src/heap-snapshot-generator.cc
File src/heap-snapshot-generator.cc (right):
https://codereview.chromium.org/22852024/diff/84001/src/heap-snapshot-generator.cc#newcode447
src/heap-snapshot-generator.cc:447: FindOrAddEntry(addr, size, false);
Why do we have two implementations of the same functions?
https://codereview.chromium.org/22852024/diff/84001/src/heap.cc
File src/heap.cc (right):
https://codereview.chromium.org/22852024/diff/84001/src/heap.cc#newcode4979
src/heap.cc:4979: HeapProfiler* profiler = isolate()->heap_profiler();
Why do you track the allocation of the memento separately? And why don't
you account the allocation in the if branch before?
https://codereview.chromium.org/22852024/diff/84001/src/spaces-inl.h
File src/spaces-inl.h (right):
https://codereview.chromium.org/22852024/diff/84001/src/spaces-inl.h#newcode293
src/spaces-inl.h:293: } while (false);
I don't like the goto emulation here. Why don't we keep the original
method and before return object we call
if (event == NEW_OBJECT) profiler->NewObjectEvent(...)
NewObjectEvent could check internally if allocation tracking is turned
on or you keep the check outside, as you like.
https://codereview.chromium.org/22852024/diff/84001/src/x64/macro-assembler-x64.h
File src/x64/macro-assembler-x64.h (right):
https://codereview.chromium.org/22852024/diff/84001/src/x64/macro-assembler-x64.h#newcode1124
src/x64/macro-assembler-x64.h:1124: // Record a JS object allocation if
allocations tracking
Should fit in one line.
https://codereview.chromium.org/22852024/diff/84001/test/cctest/test-heap-profiler.cc
File test/cctest/test-heap-profiler.cc (right):
https://codereview.chromium.org/22852024/diff/84001/test/cctest/test-heap-profiler.cc#newcode2010
test/cctest/test-heap-profiler.cc:2010: // This is an example of using
checking of JS allocations tracking in a test.
Either remove the test if it is not testing anything or add a proper
test. I would prefer adding a proper test.
https://codereview.chromium.org/22852024/diff/84001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):
https://codereview.chromium.org/22852024/diff/84001/test/cctest/test-heap.cc#newcode3232
test/cctest/test-heap.cc:3232: HeapObjectsTracker tracker;
Why is that here?
https://codereview.chromium.org/22852024/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
