On 2013/10/03 16:27:54, Alexandra Mikhaylova wrote:
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.
On 2013/10/02 18:00:29, Hannes Payer wrote:
> Please fill up the 80 characters line width.
Done.
https://codereview.chromium.org/22852024/diff/84001/include/v8-profiler.h#newcode485
include/v8-profiler.h:485: * population statistics data.
On 2013/10/02 18:00:29, Hannes Payer wrote:
> Please fill up the 80 characters line width.
Done.
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);
On 2013/10/02 18:00:29, Hannes Payer wrote:
> Why do we have two implementations of the same functions?
Sometimes we want just to update the information (the size) of the object
that
has already been tracked. This works pretty much the same as reqistering
a new
object, but one more call of NewObjectEvent (and NewObject) can confuse
other
developers, so we decided to create separate methods for it.
For example, here
https://codereview.chromium.org/22852024/diff/84001/src/heap.cc both an
object
and its allocation memento are allocated in one go. So then we track the
allocation memento as a new object and adjust information about the size
of
the
original object itself.
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();
On 2013/10/02 18:20:04, Hannes Payer wrote:
> On 2013/10/02 18:00:29, Hannes Payer wrote:
> > Why do you track the allocation of the memento separately? And why
don't
you
> > account the allocation in the if branch before?
> Ok, ignore question two, because you allocate using AllocateRaw. But
why do
you
> track the memento separately?
The memento is treated as a separate HeapObject when the heap is being
iterated,
so if we don't track it, we'll have untracked allocations.
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);
On 2013/10/02 18:00:29, Hannes Payer wrote:
> 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.
Ok, corrected it keeping the original method.
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
On 2013/10/02 18:00:29, Hannes Payer wrote:
> Should fit in one line.
Done.
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.
On 2013/10/02 18:00:29, Hannes Payer wrote:
> Either remove the test if it is not testing anything or add a proper
test. I
> would prefer adding a proper test.
This test checks that we don't miss some simplest allocations and
HeapObjectsTracker works correctly. In the HeapObjectsTracker constructor,
allocations tracking is switched on. In the destructor, allocations
tracking
is
checked and untracked objects are found (if any).
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;
On 2013/10/02 18:00:29, Hannes Payer wrote:
> Why is that here?
I used HeapObjectsTracker here to find some untracked allocations that
occur
during this test. In fact, HeapObjectsTracker can be used in all tests
where
JS
allocations are performed.
I've removed it here now as there's a special test for it.
Hi Hannes, comments have been addressed, so could you please take another
look.
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.
