HeapProfiler: for the test purposes we would like to trace object movements.

HeapProfiler: for the test purposes we would like to trace object movements.

When we have an untracked allocations we need to fix it.
It is a bit tricky process due to lack of logging.

I added FLAG_heap_profiler_trace_objects that enables log for object movements.

Drive by fix: with help of logging I found a bug in LeftTrim method for FixedArray.


BUG=none
R=svenpanne@chromium.org, yurys@chromium.org

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

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

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

[loislo, 2013-10-17 13:12:42]

On 2013/10/17 13:02:32, loislo wrote:

sample of output

New object         : 0x1b1d7e1532b8    120. Next address is 0x1b1d7e153330
New object         : 0x35e399122fe8     40. Next address is 0x35e399123010
New object         : 0x62fdc64fe40    224. Next address is 0x62fdc64ff20
New object         : 0xeb93c006ca0     32. Next address is 0xeb93c006cc0
Move object from 0xeb93c0063a0 to 0xeb93c0063a8 old size    152 new size    144
New object         : 0x35e399123010     24. Next address is 0x35e399123028
New object         : 0x62fdc64ff20    224. Next address is 0x62fdc650000
New object         : 0xeb93c006cc0     32. Next address is 0xeb93c006ce0
Move object from 0xeb93c0063a8 to 0xeb93c0063b0 old size    144 new size    136
New object         : 0x1b1d7e153330     16. Next address is 0x1b1d7e153340
Move object from 0xeb93c0063b0 to 0xeb93c0063b8 old size    136 new size    128

Begin HeapObjectsMap::FindUntrackedObjects. 8385 entries in map.
15 objects were skipped
Good object        : 0xeb93c006380     32. Next address is 0xeb93c0063a0
Untracked object   : 0xeb93c0063a0      8. Next address is 0xeb93c0063a8
Untracked object   : 0xeb93c0063a8      8. Next address is 0xeb93c0063b0
Untracked object   : 0xeb93c0063b0      8. Next address is 0xeb93c0063b8
Good object        : 0xeb93c0063b8    128. Next address is 0xeb93c006438
Last 8369 objects were skipped
End HeapObjectsMap::FindUntrackedObjects. 8385 entries in map.


#
# Fatal error in ../test/cctest/cctest.h, line 364
# CHECK_EQ(0, heap_profiler_->FindUntrackedObjects()) failed
#   Expected: 0
#   Found: 3
#

==== C stack trace ===============================

 1: V8_Fatal
 2: CheckEqualsHelper(char const*, int, char const*, int, char const*, int)
 3: HeapObjectsTracker::~HeapObjectsTracker()
 4: ??
 5: CcTest::Run()
 6: main
 7: __libc_start_main
 8: ??
(END)

[alph, 2013-10-17 13:17:24]

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.cc
File src/heap-snapshot-generator.cc (left):

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:517: CHECK_EQ(obj->Size(),
static_cast<int>(entry_info.size));
Why did you remove the size check?

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.cc
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:573: return a->obj - b->obj;
might need a static cast on x64.

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:612: for (int i = 0; i< heap_objects.length();
++i) {
nit: missing space at i<

[yurys, 2013-10-17 13:20:54]

lgtm

https://codereview.chromium.org/27694004/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/27694004/diff/1/src/flag-definitions.h#newcod...
src/flag-definitions.h:546: DEFINE_bool(heap_profiler_trace_objects, false,
May be log_heap_profiler

https://codereview.chromium.org/27694004/diff/1/src/flag-definitions.h#newcod...
src/flag-definitions.h:547: "dump heap object
allocations/movements/size_updates")
style: dump -> Dump

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.cc
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:522: PrintF("Update object      : %p %6d. Next
address is %p\n",
Next address -> End address?

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:523: obj->address(),
style: wrong alignment

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:548: bool isValid() const { return expected_size
== obj->Size(); }
style: IsValid

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:551: if (expected_size == 0)
Please add {}.

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:572: static int comparer(const HeapObjectInfo* a,
const HeapObjectInfo* b) {
comparer -> comparator

I'd also make it a member of HeapObjectInfo.

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:591: if (FLAG_heap_profiler_trace_objects)
Please use {}.

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:602: untracked++;
untracked++ should be called if sizes don't match regardless of the flag state.

[loislo, 2013-10-17 13:34:12]

https://codereview.chromium.org/27694004/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/27694004/diff/1/src/flag-definitions.h#newcod...
src/flag-definitions.h:546: DEFINE_bool(heap_profiler_trace_objects, false,
On 2013/10/17 13:20:54, Yury Semikhatsky wrote:
> May be log_heap_profiler

I think it is more precise name. Heap objects tracking is a specific task so it
definitely need a separate flag.

https://codereview.chromium.org/27694004/diff/1/src/flag-definitions.h#newcod...
src/flag-definitions.h:547: "dump heap object
allocations/movements/size_updates")
On 2013/10/17 13:20:54, Yury Semikhatsky wrote:
> style: dump -> Dump

Done.

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.cc
File src/heap-snapshot-generator.cc (left):

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:517: CHECK_EQ(obj->Size(),
static_cast<int>(entry_info.size));
On 2013/10/17 13:17:25, alph wrote:
> Why did you remove the size check?

reverted

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.cc
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:522: PrintF("Update object      : %p %6d. Next
address is %p\n",
On 2013/10/17 13:20:54, Yury Semikhatsky wrote:
> Next address -> End address?

We don't need to know the end address. 
Usually we would like to know what would be the address of the next object in
the memory. With proposed implementation we will simple search for an address
and see the object with the address and the previous object. It is very useful
information for the allocations folding case.

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:523: obj->address(),
On 2013/10/17 13:20:54, Yury Semikhatsky wrote:
> style: wrong alignment

Done.

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:548: bool isValid() const { return expected_size
== obj->Size(); }
On 2013/10/17 13:20:54, Yury Semikhatsky wrote:
> style: IsValid

Done.

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:551: if (expected_size == 0)
On 2013/10/17 13:20:54, Yury Semikhatsky wrote:
> Please add {}.

Done.

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:573: return a->obj - b->obj;
On 2013/10/17 13:17:25, alph wrote:
> might need a static cast on x64.

It is working fine on x64 and ia32

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:591: if (FLAG_heap_profiler_trace_objects)
On 2013/10/17 13:20:54, Yury Semikhatsky wrote:
> Please use {}.

Done.

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:602: untracked++;
On 2013/10/17 13:20:54, Yury Semikhatsky wrote:
> untracked++ should be called if sizes don't match regardless of the flag
state.

Done.

https://codereview.chromium.org/27694004/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:612: for (int i = 0; i< heap_objects.length();
++i) {
On 2013/10/17 13:17:25, alph wrote:
> nit: missing space at i<

Done.

[alph, 2013-10-17 16:31:46]

https://codereview.chromium.org/27694004/diff/28001/src/heap-snapshot-generat...
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/27694004/diff/28001/src/heap-snapshot-generat...
src/heap-snapshot-generator.cc:619: if (last_printed_object != i - 1) {
nit:
int objects_skipped = i - last_printed_object - 1;
if (objects_skipped > 0) {
...

[Sven Panne, 2013-10-18 06:48:56]

LGTM, although I can't really comment on the profiler part...

[loislo, 2013-10-18 07:43:59]

Committed patchset #8 manually as r17258 (presubmit successful).

[loislo, 2013-10-18 09:40:08]

Committed patchset #8 manually as r17263 (presubmit successful).

[loislo, 2013-10-18 11:00:03]

Committed patchset #9 manually as r17269 (presubmit successful).