Track JS allocations as they arrive with no affection on performance when tracking is switched off

Track JS allocations as they arrive with no affection on performance when tracking is switched off.

BUG=277984
R=hpayer@chromium.org

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

[yurys, 2013-08-26 06:32:25]

Please provide more detailes in the description of the change. In particular,
link to the bug 277984 and description of how the test helper is supposed to be
used.

Also you need to provide the perf measurements either here or in the bug.

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

https://codereview.chromium.org/22852024/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:635:
heap()->isolate()->heap_profiler()->StartHeapObjectsTracking();
Heap objects tracking should be switched separately as we may want in some cases
just take heap snapshot and have as little perf impact as possible all the rest
time. It can be a part of v8::HeapProfiler::StartTrackingHeapObjects(); or we
could introduce v8::HeapProfiler::StartRecordingHeapAllocations().

I think it would also make sense not to set is_tracking_objects_ to true here as
we have a separate API for this. But this is a topic for a separate change.

https://codereview.chromium.org/22852024/diff/1/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/22852024/diff/1/src/x64/macro-assembler-x64.c...
src/x64/macro-assembler-x64.cc:4705: void
MacroAssembler::RecordObjectAllocation(Isolate* isolate,
I believe we should be able to turn this into a builtin function that can be
generated only once. This would allow us to:
1) Avoid code bloat due to RecordObjectAllocation code being replicated at every
allocation site.
2) Keep using near jumps in the allocation code in SubStringStub::Generate

https://codereview.chromium.org/22852024/diff/1/test/cctest/cctest.h
File test/cctest/cctest.h (right):

https://codereview.chromium.org/22852024/diff/1/test/cctest/cctest.h#newcode334
test/cctest/cctest.h:334: int test() {
GetUntrackedObjectsCount

[yurys, 2013-08-26 06:34:28]

@danno, can you take a look at this please and provide your feedback?

[Alexandra Mikhaylova, 2013-08-26 15:48:40]

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

https://codereview.chromium.org/22852024/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:635:
heap()->isolate()->heap_profiler()->StartHeapObjectsTracking();
On 2013/08/26 06:32:25, Yury Semikhatsky wrote:
> Heap objects tracking should be switched separately as we may want in some
cases
> just take heap snapshot and have as little perf impact as possible all the
rest
> time. It can be a part of v8::HeapProfiler::StartTrackingHeapObjects(); or we
> could introduce v8::HeapProfiler::StartRecordingHeapAllocations().
> 
> I think it would also make sense not to set is_tracking_objects_ to true here
as
> we have a separate API for this. But this is a topic for a separate change.

Done.

https://codereview.chromium.org/22852024/diff/1/test/cctest/cctest.h
File test/cctest/cctest.h (right):

https://codereview.chromium.org/22852024/diff/1/test/cctest/cctest.h#newcode334
test/cctest/cctest.h:334: int test() {
On 2013/08/26 06:32:25, Yury Semikhatsky wrote:
> GetUntrackedObjectsCount

Done.

[loislo, 2013-08-27 09:04:57]

https://codereview.chromium.org/22852024/diff/8001/src/heap-snapshot-generato...
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/22852024/diff/8001/src/heap-snapshot-generato...
src/heap-snapshot-generator.cc:398: void HeapObjectsMap::MoveObject(Address
from, Address to, int object_size) {
I think that we need new method UpdateObjectSize and UpdateObjectSizeEvent in
the heap profiler API

or 

we need to change the signature of the method to UpdateObjectInfo.

WDYT?

https://codereview.chromium.org/22852024/diff/8001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/22852024/diff/8001/src/heap.cc#newcode5040
src/heap.cc:5040: profiler->NewObjectEvent(HeapObject::cast(clone)->address(),
this call actually just updates the size for the object that was allocated in
the line 4989. I think it could confuse other developers.
So probably we need to call a function that just updates the reported size.

https://codereview.chromium.org/22852024/diff/8001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/22852024/diff/8001/src/mark-compact.cc#newcod...
src/mark-compact.cc:2929: MaybeObject* maybe_result =
target_space->AllocateRawForMigration(object_size);
style: 80 symbols

https://codereview.chromium.org/22852024/diff/8001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/22852024/diff/8001/src/objects.cc#newcode2317
src/objects.cc:2317: // during GC, and size has to be adjusted nevetheless.
tipo: nevertheless

https://codereview.chromium.org/22852024/diff/8001/src/objects.cc#newcode2320
src/objects.cc:2320: profiler->NewObjectEvent(elms->address(), elms->Size());
Sounds like we need UpdateObjectSizeEvent method.

https://codereview.chromium.org/22852024/diff/8001/src/spaces-inl.h
File src/spaces-inl.h (right):

https://codereview.chromium.org/22852024/diff/8001/src/spaces-inl.h#newcode358
src/spaces-inl.h:358: HeapObject* obj = HeapObject::FromAddress(old_top);
Could this line be moved down into the if ("tracking") statement?

https://codereview.chromium.org/22852024/diff/8001/src/spaces.h
File src/spaces.h (right):

https://codereview.chromium.org/22852024/diff/8001/src/spaces.h#newcode1702
src/spaces.h:1702: MUST_USE_RESULT inline MaybeObject*
AllocateRawForMigration(int size_in_bytes);
style: 80 symbols

[Hannes Payer (out of office), 2013-08-28 09:51:32]

https://codereview.chromium.org/22852024/diff/8001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/22852024/diff/8001/src/assembler.cc#newcode1320
src/assembler.cc:1320: Isolate* isolate) {
4 spaces indent

https://codereview.chromium.org/22852024/diff/8001/src/assembler.cc#newcode1327
src/assembler.cc:1327: Isolate* isolate) {
4 spaces indent

https://codereview.chromium.org/22852024/diff/8001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/22852024/diff/8001/src/objects.cc#newcode2320
src/objects.cc:2320: profiler->NewObjectEvent(elms->address(), elms->Size());
yes, an update object size event would be the right thing here.

https://codereview.chromium.org/22852024/diff/8001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/22852024/diff/8001/src/serialize.cc#newcode602
src/serialize.cc:602: Add(address, LAZY_DEOPTIMIZATION, 64 + entry,
"lazy_deopt");
64 => 66

https://codereview.chromium.org/22852024/diff/8001/src/spaces.h
File src/spaces.h (right):

https://codereview.chromium.org/22852024/diff/8001/src/spaces.h#newcode1701
src/spaces.h:1701: MUST_USE_RESULT inline MaybeObject* AllocateRaw(int
size_in_bytes);
Instead of adding multiple allocation methods to space, can we do
AllocateRaw(int size_in_bytes, AllocationType event);? Where AllocationType is
an enum that encodes the type of allocation (new allocation or migration).

https://codereview.chromium.org/22852024/diff/8001/src/x64/macro-assembler-x64.h
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/22852024/diff/8001/src/x64/macro-assembler-x6...
src/x64/macro-assembler-x64.h:1058: int object_size);
Can we move these methods after the Allocate(...) methods and add a comment.

[Hannes Payer (out of office), 2013-08-28 10:35:05]

https://codereview.chromium.org/22852024/diff/8001/src/x64/macro-assembler-x6...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/22852024/diff/8001/src/x64/macro-assembler-x6...
src/x64/macro-assembler-x64.cc:4709: cmpb(ExternalOperand(
I am concerned about the performance impact of that.

[danno, 2013-09-09 10:33:18]

IMO, this approach is a no-go. I find it very hard to believe that there are no
regressions with this patch on the core benchmarks. It significantly bloats the
allocation path and adds instructions to the critical path in the optimized code
case. This is fundamentally not a wise strategy for a feature that will be not
used by the 99.99%+ of Chrome installs.

A much better way to do this is to add the instrumentation only to non-optimized
code and code stubs only. Crankshafted code should not have any additional
instructions in the allocation path by default. When tracking is turned on, all
optimized should be deoptimized so that all of the allocations go through the
instrumentation and newly generated Crankshaft code, too. If the option is
turned off again, another deopt-all should flush the instrumented optimized
code. 

There would be some further issues with crankshaft-generated stubs, but you
could build a system that tracks stubs generated with Crankshaft that use the
HAllocate instruction and invalidates them/temporarily patches them out to tail
call new versions with instrumentation (it's not an option to leave the stubs,
e.g. Array constructors with the instrumentation in them).

[Alexandra Mikhaylova, 2013-09-19 16:03:37]

https://codereview.chromium.org/22852024/diff/8001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/22852024/diff/8001/src/assembler.cc#newcode1320
src/assembler.cc:1320: Isolate* isolate) {
On 2013/08/28 09:51:32, Hannes Payer wrote:
> 4 spaces indent

Removed this function as it became redundant after new changes.

https://codereview.chromium.org/22852024/diff/8001/src/assembler.cc#newcode1327
src/assembler.cc:1327: Isolate* isolate) {
On 2013/08/28 09:51:32, Hannes Payer wrote:
> 4 spaces indent

Done.

https://codereview.chromium.org/22852024/diff/8001/src/heap-snapshot-generato...
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/22852024/diff/8001/src/heap-snapshot-generato...
src/heap-snapshot-generator.cc:398: void HeapObjectsMap::MoveObject(Address
from, Address to, int object_size) {
On 2013/08/27 09:04:57, loislo wrote:
> I think that we need new method UpdateObjectSize and UpdateObjectSizeEvent in
> the heap profiler API
> 
> or 
> 
> we need to change the signature of the method to UpdateObjectInfo.
> 
> WDYT?

Done.

https://codereview.chromium.org/22852024/diff/8001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/22852024/diff/8001/src/heap.cc#newcode5040
src/heap.cc:5040: profiler->NewObjectEvent(HeapObject::cast(clone)->address(),
On 2013/08/27 09:04:57, loislo wrote:
> this call actually just updates the size for the object that was allocated in
> the line 4989. I think it could confuse other developers.
> So probably we need to call a function that just updates the reported size.

Done.

https://codereview.chromium.org/22852024/diff/8001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/22852024/diff/8001/src/mark-compact.cc#newcod...
src/mark-compact.cc:2929: MaybeObject* maybe_result =
target_space->AllocateRawForMigration(object_size);
On 2013/08/27 09:04:57, loislo wrote:
> style: 80 symbols

Done.

https://codereview.chromium.org/22852024/diff/8001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/22852024/diff/8001/src/objects.cc#newcode2317
src/objects.cc:2317: // during GC, and size has to be adjusted nevetheless.
On 2013/08/27 09:04:57, loislo wrote:
> tipo: nevertheless

Done.

https://codereview.chromium.org/22852024/diff/8001/src/objects.cc#newcode2320
src/objects.cc:2320: profiler->NewObjectEvent(elms->address(), elms->Size());
On 2013/08/27 09:04:57, loislo wrote:
> Sounds like we need UpdateObjectSizeEvent method.

Done.

https://codereview.chromium.org/22852024/diff/8001/src/objects.cc#newcode2320
src/objects.cc:2320: profiler->NewObjectEvent(elms->address(), elms->Size());
On 2013/08/28 09:51:32, Hannes Payer wrote:
> yes, an update object size event would be the right thing here.

Done.

https://codereview.chromium.org/22852024/diff/8001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/22852024/diff/8001/src/serialize.cc#newcode602
src/serialize.cc:602: Add(address, LAZY_DEOPTIMIZATION, 64 + entry,
"lazy_deopt");
On 2013/08/28 09:51:32, Hannes Payer wrote:
> 64 => 66

Done.

https://codereview.chromium.org/22852024/diff/8001/src/spaces-inl.h
File src/spaces-inl.h (right):

https://codereview.chromium.org/22852024/diff/8001/src/spaces-inl.h#newcode358
src/spaces-inl.h:358: HeapObject* obj = HeapObject::FromAddress(old_top);
On 2013/08/27 09:04:57, loislo wrote:
> Could this line be moved down into the if ("tracking") statement?

I don't think so. Could you please explain why we would want to move it?
Tracking may be switched off but the allocation should be performed I guess.

https://codereview.chromium.org/22852024/diff/8001/src/spaces.h
File src/spaces.h (right):

https://codereview.chromium.org/22852024/diff/8001/src/spaces.h#newcode1702
src/spaces.h:1702: MUST_USE_RESULT inline MaybeObject*
AllocateRawForMigration(int size_in_bytes);
On 2013/08/27 09:04:57, loislo wrote:
> style: 80 symbols

Done.

https://codereview.chromium.org/22852024/diff/8001/src/x64/macro-assembler-x64.h
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/22852024/diff/8001/src/x64/macro-assembler-x6...
src/x64/macro-assembler-x64.h:1058: int object_size);
On 2013/08/28 09:51:32, Hannes Payer wrote:
> Can we move these methods after the Allocate(...) methods and add a comment.

Done.

[Alexandra Mikhaylova, 2013-09-19 16:09:04]

https://codereview.chromium.org/22852024/diff/28001/test/cctest/cctest.cc
File test/cctest/cctest.cc (right):

https://codereview.chromium.org/22852024/diff/28001/test/cctest/cctest.cc#new...
test/cctest/cctest.cc:195: LocalContext env;
All we need to do here is to quickly initialize the isolate. This is not the
best way to do it and needs to be fixed.

[Alexandra Mikhaylova, 2013-09-24 14:56:19]

https://codereview.chromium.org/22852024/diff/8001/src/spaces.h
File src/spaces.h (right):

https://codereview.chromium.org/22852024/diff/8001/src/spaces.h#newcode1701
src/spaces.h:1701: MUST_USE_RESULT inline MaybeObject* AllocateRaw(int
size_in_bytes);
On 2013/08/28 09:51:32, Hannes Payer wrote:
> Instead of adding multiple allocation methods to space, can we do
> AllocateRaw(int size_in_bytes, AllocationType event);? Where AllocationType is
> an enum that encodes the type of allocation (new allocation or migration).

Done.

[yurys, 2013-09-25 10:41:12]

https://codereview.chromium.org/22852024/diff/40001/src/heap-profiler.cc
File src/heap-profiler.cc (right):

https://codereview.chromium.org/22852024/diff/40001/src/heap-profiler.cc#newc...
src/heap-profiler.cc:162: DropCompiledCode();
Would it make sense to run GC and collect dropped code objects before updating
the map?

https://codereview.chromium.org/22852024/diff/40001/src/heap-profiler.cc#newc...
src/heap-profiler.cc:182: Heap* heap_ = heap();
style: heap_ -> heap or use heap() directly, without assigning to a local
variable.

https://codereview.chromium.org/22852024/diff/40001/src/heap-profiler.cc#newc...
src/heap-profiler.cc:193:
Handle<Code>(isolate->builtins()->builtin(Builtins::kLazyCompile));
style: 4 spaces indent

https://codereview.chromium.org/22852024/diff/40001/src/heap-profiler.cc#newc...
src/heap-profiler.cc:208: if (!shared->script()->IsScript()) continue;
We will need to do something with the precompiled functions that have no script,
otherwise we will miss their allocations but I think it's fine for the first
iteration.

https://codereview.chromium.org/22852024/diff/40001/src/heap-profiler.h
File src/heap-profiler.h (right):

https://codereview.chromium.org/22852024/diff/40001/src/heap-profiler.h#newco...
src/heap-profiler.h:106: private:
style: blank line above "private:"

https://codereview.chromium.org/22852024/diff/40001/src/heap-profiler.h#newco...
src/heap-profiler.h:109: void UpdateMap() { snapshots_->UpdateMap(); }
Just use snapshots_->UpdateMap() instead.

https://codereview.chromium.org/22852024/diff/40001/src/heap-snapshot-generat...
File src/heap-snapshot-generator.h (right):

https://codereview.chromium.org/22852024/diff/40001/src/heap-snapshot-generat...
src/heap-snapshot-generator.h:256: void UpdateMap();
There is already UpdateHeapObjectsMap which does almost the same, I believe we
can reuse it.

https://codereview.chromium.org/22852024/diff/40001/src/spaces.h
File src/spaces.h (right):

https://codereview.chromium.org/22852024/diff/40001/src/spaces.h#newcode1909
src/spaces.h:1909: inline HeapObject* AllocateRawHelper(int size_in_bytes);
What's the point in the inline modifier here?

https://codereview.chromium.org/22852024/diff/40001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/22852024/diff/40001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:4945: // Make a long jump due to
RecordObjectAllocation inside
You could calculate

Label::Distance distance =
masm->isolate()->heap_profiler()->is_tracking_allocations() ? Label::kFar :
Label::kNear;

once and use it in the two jumps below.

https://codereview.chromium.org/22852024/diff/40001/test/cctest/cctest.h
File test/cctest/cctest.h (left):

https://codereview.chromium.org/22852024/diff/40001/test/cctest/cctest.h#oldc...
test/cctest/cctest.h:81: 
revert this

https://codereview.chromium.org/22852024/diff/40001/test/cctest/cctest.h
File test/cctest/cctest.h (right):

https://codereview.chromium.org/22852024/diff/40001/test/cctest/cctest.h#newc...
test/cctest/cctest.h:357: virtual ~HeapObjectsTracker() {
No need to make it virtual.

https://codereview.chromium.org/22852024/diff/40001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (left):

https://codereview.chromium.org/22852024/diff/40001/test/cctest/test-heap.cc#...
test/cctest/test-heap.cc:3255: 
Revert this.

[Alexandra Mikhaylova, 2013-09-25 14:33:39]

https://codereview.chromium.org/22852024/diff/40001/src/heap-profiler.cc
File src/heap-profiler.cc (right):

https://codereview.chromium.org/22852024/diff/40001/src/heap-profiler.cc#newc...
src/heap-profiler.cc:162: DropCompiledCode();
On 2013/09/25 10:41:12, Yury Semikhatsky wrote:
> Would it make sense to run GC and collect dropped code objects before updating
> the map?

Done.

https://codereview.chromium.org/22852024/diff/40001/src/heap-profiler.cc#newc...
src/heap-profiler.cc:182: Heap* heap_ = heap();
On 2013/09/25 10:41:12, Yury Semikhatsky wrote:
> style: heap_ -> heap or use heap() directly, without assigning to a local
> variable.

Done.

https://codereview.chromium.org/22852024/diff/40001/src/heap-profiler.cc#newc...
src/heap-profiler.cc:193:
Handle<Code>(isolate->builtins()->builtin(Builtins::kLazyCompile));
On 2013/09/25 10:41:12, Yury Semikhatsky wrote:
> style: 4 spaces indent

Done.

https://codereview.chromium.org/22852024/diff/40001/src/heap-profiler.h
File src/heap-profiler.h (right):

https://codereview.chromium.org/22852024/diff/40001/src/heap-profiler.h#newco...
src/heap-profiler.h:106: private:
On 2013/09/25 10:41:12, Yury Semikhatsky wrote:
> style: blank line above "private:"

Done.

https://codereview.chromium.org/22852024/diff/40001/src/heap-profiler.h#newco...
src/heap-profiler.h:109: void UpdateMap() { snapshots_->UpdateMap(); }
On 2013/09/25 10:41:12, Yury Semikhatsky wrote:
> Just use snapshots_->UpdateMap() instead.

Done.

https://codereview.chromium.org/22852024/diff/40001/src/heap-snapshot-generat...
File src/heap-snapshot-generator.h (right):

https://codereview.chromium.org/22852024/diff/40001/src/heap-snapshot-generat...
src/heap-snapshot-generator.h:256: void UpdateMap();
On 2013/09/25 10:41:12, Yury Semikhatsky wrote:
> There is already UpdateHeapObjectsMap which does almost the same, I believe we
> can reuse it.

Done.

https://codereview.chromium.org/22852024/diff/40001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/22852024/diff/40001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:4945: // Make a long jump due to
RecordObjectAllocation inside
On 2013/09/25 10:41:12, Yury Semikhatsky wrote:
> You could calculate
> 
> Label::Distance distance =
> masm->isolate()->heap_profiler()->is_tracking_allocations() ? Label::kFar :
> Label::kNear;
> 
> once and use it in the two jumps below.

Done.

https://codereview.chromium.org/22852024/diff/40001/test/cctest/cctest.h
File test/cctest/cctest.h (left):

https://codereview.chromium.org/22852024/diff/40001/test/cctest/cctest.h#oldc...
test/cctest/cctest.h:81: 
On 2013/09/25 10:41:12, Yury Semikhatsky wrote:
> revert this

Done.

https://codereview.chromium.org/22852024/diff/40001/test/cctest/cctest.h
File test/cctest/cctest.h (right):

https://codereview.chromium.org/22852024/diff/40001/test/cctest/cctest.h#newc...
test/cctest/cctest.h:357: virtual ~HeapObjectsTracker() {
On 2013/09/25 10:41:12, Yury Semikhatsky wrote:
> No need to make it virtual.

Done.

https://codereview.chromium.org/22852024/diff/40001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (left):

https://codereview.chromium.org/22852024/diff/40001/test/cctest/test-heap.cc#...
test/cctest/test-heap.cc:3255: 
On 2013/09/25 10:41:12, Yury Semikhatsky wrote:
> Revert this.

Done.

[yurys, 2013-09-25 14:51:51]

Looks ok to me except that we will need to create ExitFrame when calling into
the HeapProfiler to make JavaScriptFrameIterator to work properly but I believe
this can be addressed later.

@danno, @svenpanne, @hpayer can you take a look please?

https://codereview.chromium.org/22852024/diff/72001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/22852024/diff/72001/src/heap.cc#newcode4982
src/heap.cc:4982: object_size);
style: wrong alignment

[loislo, 2013-09-26 09:11:33]

sgtm

https://codereview.chromium.org/22852024/diff/72001/src/spaces-inl.h
File src/spaces-inl.h (right):

https://codereview.chromium.org/22852024/diff/72001/src/spaces-inl.h#newcode310
src/spaces-inl.h:310: MaybeObject* PagedSpace::AllocateRaw(int size_in_bytes,
I'd rewrite this the next way:

// Raw allocation.
MaybeObject* PagedSpace::AllocateRaw(int size_in_bytes,
                                     AllocationType event) {
  HeapObject* object = NULL;
  do {
    object = AllocateLinearly(size_in_bytes);
    if (object != NULL) break;

    ASSERT(!heap()->linear_allocation() ||
           (anchor_.next_chunk() == &anchor_ &&
            anchor_.prev_chunk() == &anchor_));

    object = free_list_.Allocate(size_in_bytes);
    if (object != NULL) break;

    object = SlowAllocateRaw(size_in_bytes);
  } while (false);
  
  if (object == NULL) return Failure::RetryAfterGC(identity());

  if (identity() == CODE_SPACE) {
    SkipList::Update(object->address(), size_in_bytes);
  }
  if (event == NEW_OBJECT) {
    HeapProfiler* profiler = heap()->isolate()->heap_profiler();
    if (profiler->is_tracking_allocations()) {
      profiler->NewObjectEvent(object->address(), size_in_bytes);
    }
  }
  return object;
}

[loislo, 2013-09-26 10:49:38]

On 2013/09/26 09:11:33, loislo wrote:
> sgtm
> 
> https://codereview.chromium.org/22852024/diff/72001/src/spaces-inl.h
> File src/spaces-inl.h (right):
> 
>
https://codereview.chromium.org/22852024/diff/72001/src/spaces-inl.h#newcode310
> src/spaces-inl.h:310: MaybeObject* PagedSpace::AllocateRaw(int size_in_bytes,
> I'd rewrite this the next way:
> 
> // Raw allocation.
> MaybeObject* PagedSpace::AllocateRaw(int size_in_bytes,
>                                      AllocationType event) {
>   HeapObject* object = NULL;
>   do {
>     object = AllocateLinearly(size_in_bytes);
>     if (object != NULL) break;
> 
>     ASSERT(!heap()->linear_allocation() ||
>            (anchor_.next_chunk() == &anchor_ &&
>             anchor_.prev_chunk() == &anchor_));
> 
>     object = free_list_.Allocate(size_in_bytes);
>     if (object != NULL) break;
> 
>     object = SlowAllocateRaw(size_in_bytes);
>   } while (false);
>   
>   if (object == NULL) return Failure::RetryAfterGC(identity());
> 
>   if (identity() == CODE_SPACE) {
>     SkipList::Update(object->address(), size_in_bytes);
>   }
>   if (event == NEW_OBJECT) {
>     HeapProfiler* profiler = heap()->isolate()->heap_profiler();
>     if (profiler->is_tracking_allocations()) {
>       profiler->NewObjectEvent(object->address(), size_in_bytes);
>     }
>   }
>   return object;
> }

BTW profiler pointer might be null.

[loislo, 2013-09-26 11:53:06]

https://codereview.chromium.org/22852024/diff/72001/src/spaces-inl.h
File src/spaces-inl.h (right):

https://codereview.chromium.org/22852024/diff/72001/src/spaces-inl.h#newcode357
src/spaces-inl.h:357: if (profiler->is_tracking_allocations()) {
profiler is null here at test-spaces/NewSpace

[yurys, 2013-09-26 12:17:06]

https://codereview.chromium.org/22852024/diff/72001/src/spaces-inl.h
File src/spaces-inl.h (right):

https://codereview.chromium.org/22852024/diff/72001/src/spaces-inl.h#newcode357
src/spaces-inl.h:357: if (profiler->is_tracking_allocations()) {
On 2013/09/26 11:53:07, loislo wrote:
> profiler is null here at test-spaces/NewSpace
> 

The profiler is created in Isolate::Init so it should always exist here except
for some unit tests probably.

[Alexandra Mikhaylova, 2013-09-26 13:23:40]

https://codereview.chromium.org/22852024/diff/72001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/22852024/diff/72001/src/heap.cc#newcode4982
src/heap.cc:4982: object_size);
On 2013/09/25 14:51:52, Yury Semikhatsky wrote:
> style: wrong alignment

Done.

https://codereview.chromium.org/22852024/diff/72001/src/spaces-inl.h
File src/spaces-inl.h (right):

https://codereview.chromium.org/22852024/diff/72001/src/spaces-inl.h#newcode310
src/spaces-inl.h:310: MaybeObject* PagedSpace::AllocateRaw(int size_in_bytes,
On 2013/09/26 09:11:33, loislo wrote:
> I'd rewrite this the next way:
> 
> // Raw allocation.
> MaybeObject* PagedSpace::AllocateRaw(int size_in_bytes,
>                                      AllocationType event) {
>   HeapObject* object = NULL;
>   do {
>     object = AllocateLinearly(size_in_bytes);
>     if (object != NULL) break;
> 
>     ASSERT(!heap()->linear_allocation() ||
>            (anchor_.next_chunk() == &anchor_ &&
>             anchor_.prev_chunk() == &anchor_));
> 
>     object = free_list_.Allocate(size_in_bytes);
>     if (object != NULL) break;
> 
>     object = SlowAllocateRaw(size_in_bytes);
>   } while (false);
>   
>   if (object == NULL) return Failure::RetryAfterGC(identity());
> 
>   if (identity() == CODE_SPACE) {
>     SkipList::Update(object->address(), size_in_bytes);
>   }
>   if (event == NEW_OBJECT) {
>     HeapProfiler* profiler = heap()->isolate()->heap_profiler();
>     if (profiler->is_tracking_allocations()) {
>       profiler->NewObjectEvent(object->address(), size_in_bytes);
>     }
>   }
>   return object;
> }

Done.

[yurys, 2013-09-27 11:55:35]

Does the link to perf results in the description include measurements for patch
set #9 or for some earlier version where the tracking flag was checked right in
the generated code?

[Hannes Payer (out of office), 2013-10-02 18:00:28]

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#new...
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#new...
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#newc...
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-generat...
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/22852024/diff/84001/src/heap-snapshot-generat...
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-x...
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/22852024/diff/84001/src/x64/macro-assembler-x...
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-pro...
File test/cctest/test-heap-profiler.cc (right):

https://codereview.chromium.org/22852024/diff/84001/test/cctest/test-heap-pro...
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#...
test/cctest/test-heap.cc:3232: HeapObjectsTracker tracker;
Why is that here?

[Hannes Payer (out of office), 2013-10-02 18:20:03]

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: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?

[Alexandra Mikhaylova, 2013-10-03 16:27:37]

On 2013/10/02 18:20:03, Hannes Payer wrote:
> 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: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?

Hi Hannes,

Thanks for your feedback. I've made some corrections.

Allocation of several objects in one go may be a problem as there appear
untracked allocations. For example, if both objects A and B are allocated in one
go (and B goes right after A), we will have A tracked with the wrong size
(sizeof(A, B)) and B untracked. The size of A may be adjusted later during
migration, but B will still remain untracked.
We try to mind such situations. A good example is allocation of an object and an
allocation memento together
(https://codereview.chromium.org/22852024/diff/84001/src/heap.cc). After the
allocation itself, we adjust the object size and track the memento.

[Alexandra Mikhaylova, 2013-10-03 16:27:54]

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#new...
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#new...
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-generat...
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/22852024/diff/84001/src/heap-snapshot-generat...
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-x...
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/22852024/diff/84001/src/x64/macro-assembler-x...
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-pro...
File test/cctest/test-heap-profiler.cc (right):

https://codereview.chromium.org/22852024/diff/84001/test/cctest/test-heap-pro...
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#...
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.

[loislo, 2013-10-10 13:40:13]

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#new...
> 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#new...
> 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-generat...
> File src/heap-snapshot-generator.cc (right):
> 
>
https://codereview.chromium.org/22852024/diff/84001/src/heap-snapshot-generat...
> 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-x...
> File src/x64/macro-assembler-x64.h (right):
> 
>
https://codereview.chromium.org/22852024/diff/84001/src/x64/macro-assembler-x...
> 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-pro...
> File test/cctest/test-heap-profiler.cc (right):
> 
>
https://codereview.chromium.org/22852024/diff/84001/test/cctest/test-heap-pro...
> 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#...
> 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.

[Hannes Payer (out of office), 2013-10-10 18:54:39]

I would propose to turn off allocation folding when you turn on object tracking
and vice versa. FLAG_use_allocation_folding = false
Doing that will give you all the objects with corresponding sizes.

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

https://codereview.chromium.org/22852024/diff/84001/src/heap-snapshot-generat...
src/heap-snapshot-generator.cc:447: FindOrAddEntry(addr, size, false);
On 2013/10/03 16:27:55, Alexandra Mikhaylova wrote:
> 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.

Since both functions implement the same functionality, it should really be just
one function. Let's find a more abstract name for the function. We could call it
CreateOrUpdateObjectSize which would be similar to FindOrAddEntry...

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/03 16:27:55, Alexandra Mikhaylova wrote:
> 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.

OK. You could also ignore the mementos since they are VM specific and never
visible to the user. Mementos allocated in optimized code are allocated together
with their parent object, so you cannot track them. They are manually folded
into the parent object.

https://codereview.chromium.org/22852024/diff/84001/test/cctest/test-heap-pro...
File test/cctest/test-heap-profiler.cc (right):

https://codereview.chromium.org/22852024/diff/84001/test/cctest/test-heap-pro...
test/cctest/test-heap-profiler.cc:2010: // This is an example of using checking
of JS allocations tracking in a test.
On 2013/10/03 16:27:55, Alexandra Mikhaylova wrote:
> 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).

Sure, but wouldn't it make sense to check which objects are found?

[loislo, 2013-10-11 05:59:32]

On 2013/10/10 18:54:39, Hannes Payer wrote:
> I would propose to turn off allocation folding when you turn on object
tracking
> and vice versa. FLAG_use_allocation_folding = false
> Doing that will give you all the objects with corresponding sizes.

If the flag doesn't change the sizes or count of the objects in the heap we will
use it.
Otherwise that trick will give us the results that doesn't reflect the real
'allocations picture' for the page.



> 
> 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/03 16:27:55, Alexandra Mikhaylova wrote:
> > 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.
> 
> OK. You could also ignore the mementos since they are VM specific and never
> visible to the user. Mementos allocated in optimized code are allocated
together
> with their parent object, so you cannot track them. They are manually folded
> into the parent object.

It doesn't matter that memento couldn't be accessible from js.

If memento is an object (from GC point of view i.e. could be moved or collected)
then AllocationProfiler has to track it as a separate allocation.

The scenario:
1) enable allocation tracking
2) take heap snapshot,
3) do some action that allocates memento together with an object
4) take another snapshot
5) compare the snapshots
6) select memento object in the diff

Expected result:
we show the time and js callstack of the allocation point for this memento
object.

[Hannes Payer (out of office), 2013-10-11 08:27:11]

On 2013/10/11 05:59:32, loislo wrote:
> On 2013/10/10 18:54:39, Hannes Payer wrote:
> > I would propose to turn off allocation folding when you turn on object
> tracking
> > and vice versa. FLAG_use_allocation_folding = false
> > Doing that will give you all the objects with corresponding sizes.
> 
> If the flag doesn't change the sizes or count of the objects in the heap we
will
> use it.
> Otherwise that trick will give us the results that doesn't reflect the real
> 'allocations picture' for the page.

The flag does not change the size and objects that get allocated. It may just
result
in more memory fragmentation when folding is turned on. For example,
new A()
if (condition)
new B()
else
new C()
Folding will allocate A+B+C together but will later at runtime only use A+B or
A+C. B or C will be fragmentation, there will not be an objects at that
location,
just free space filler memory.

I guess it is fine to turn it off.

> 
> 
> 
> > 
> > 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/03 16:27:55, Alexandra Mikhaylova wrote:
> > > 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.
> > 
> > OK. You could also ignore the mementos since they are VM specific and never
> > visible to the user. Mementos allocated in optimized code are allocated
> together
> > with their parent object, so you cannot track them. They are manually folded
> > into the parent object.
> 
> It doesn't matter that memento couldn't be accessible from js.
> 
> If memento is an object (from GC point of view i.e. could be moved or
collected)
> then AllocationProfiler has to track it as a separate allocation.
> 
> The scenario:
> 1) enable allocation tracking
> 2) take heap snapshot,
> 3) do some action that allocates memento together with an object
> 4) take another snapshot
> 5) compare the snapshots
> 6) select memento object in the diff
> 
> Expected result:
> we show the time and js callstack of the allocation point for this memento
> object.

OK, fine.

[yurys, 2013-10-11 09:20:14]

On 2013/10/11 08:27:11, Hannes Payer wrote:
> On 2013/10/11 05:59:32, loislo wrote:
> > On 2013/10/10 18:54:39, Hannes Payer wrote:
> > > I would propose to turn off allocation folding when you turn on object
> > tracking
> > > and vice versa. FLAG_use_allocation_folding = false
> > > Doing that will give you all the objects with corresponding sizes.
> > 
> > If the flag doesn't change the sizes or count of the objects in the heap we
> will
> > use it.
> > Otherwise that trick will give us the results that doesn't reflect the real
> > 'allocations picture' for the page.
> 
> The flag does not change the size and objects that get allocated. It may just
> result
> in more memory fragmentation when folding is turned on. For example,
> new A()
> if (condition)
> new B()
> else
> new C()
> Folding will allocate A+B+C together but will later at runtime only use A+B or
> A+C. B or C will be fragmentation, there will not be an objects at that
> location,
> just free space filler memory.
> 
> I guess it is fine to turn it off.
> 
I agree that it would be fine to turn this optimization off when tracking
allocations. Can you provide some pointers on where to put the flag disabling
coalesced allocations in the optimizing copiler?

[Hannes Payer (out of office), 2013-10-11 13:01:43]

On 2013/10/11 09:20:14, Yury Semikhatsky wrote:
> On 2013/10/11 08:27:11, Hannes Payer wrote:
> > On 2013/10/11 05:59:32, loislo wrote:
> > > On 2013/10/10 18:54:39, Hannes Payer wrote:
> > > > I would propose to turn off allocation folding when you turn on object
> > > tracking
> > > > and vice versa. FLAG_use_allocation_folding = false
> > > > Doing that will give you all the objects with corresponding sizes.
> > > 
> > > If the flag doesn't change the sizes or count of the objects in the heap
we
> > will
> > > use it.
> > > Otherwise that trick will give us the results that doesn't reflect the
real
> > > 'allocations picture' for the page.
> > 
> > The flag does not change the size and objects that get allocated. It may
just
> > result
> > in more memory fragmentation when folding is turned on. For example,
> > new A()
> > if (condition)
> > new B()
> > else
> > new C()
> > Folding will allocate A+B+C together but will later at runtime only use A+B
or
> > A+C. B or C will be fragmentation, there will not be an objects at that
> > location,
> > just free space filler memory.
> > 
> > I guess it is fine to turn it off.
> > 
> I agree that it would be fine to turn this optimization off when tracking
> allocations. Can you provide some pointers on where to put the flag disabling
> coalesced allocations in the optimizing copiler?

Setting the flag to false does not really work, because if it is set by the
embedder
to false StopHeapAllocationsRecording would turn it on again.
In void HAllocate::HandleSideEffectDominator
I would change the line
  if (!FLAG_use_allocation_folding) return;
to 
  if (!FLAG_use_allocation_folding && !profiler->is_tracking_allocations())

[yurys, 2013-10-14 07:54:15]

On 2013/10/11 13:01:43, Hannes Payer wrote:
> On 2013/10/11 09:20:14, Yury Semikhatsky wrote:
> > On 2013/10/11 08:27:11, Hannes Payer wrote:
> > > On 2013/10/11 05:59:32, loislo wrote:
> > > > On 2013/10/10 18:54:39, Hannes Payer wrote:
> > > > > I would propose to turn off allocation folding when you turn on object
> > > > tracking
> > > > > and vice versa. FLAG_use_allocation_folding = false
> > > > > Doing that will give you all the objects with corresponding sizes.
> > > > 
> > > > If the flag doesn't change the sizes or count of the objects in the heap
> we
> > > will
> > > > use it.
> > > > Otherwise that trick will give us the results that doesn't reflect the
> real
> > > > 'allocations picture' for the page.
> > > 
> > > The flag does not change the size and objects that get allocated. It may
> just
> > > result
> > > in more memory fragmentation when folding is turned on. For example,
> > > new A()
> > > if (condition)
> > > new B()
> > > else
> > > new C()
> > > Folding will allocate A+B+C together but will later at runtime only use
A+B
> or
> > > A+C. B or C will be fragmentation, there will not be an objects at that
> > > location,
> > > just free space filler memory.
> > > 
> > > I guess it is fine to turn it off.
> > > 
> > I agree that it would be fine to turn this optimization off when tracking
> > allocations. Can you provide some pointers on where to put the flag
disabling
> > coalesced allocations in the optimizing copiler?
> 
> Setting the flag to false does not really work, because if it is set by the
> embedder
> to false StopHeapAllocationsRecording would turn it on again.
> In void HAllocate::HandleSideEffectDominator
> I would change the line
>   if (!FLAG_use_allocation_folding) return;
> to 
>   if (!FLAG_use_allocation_folding && !profiler->is_tracking_allocations())


I see there is already a flag for this, I was under the impression that you were
suggesting us add it. Thanks for clarifying this.

I don't mind turning allocation folding off when using the tracker. If we find
that there is a noticeable effect of turning the folding off we can address this
later in a separate change. Is it the only major concern about the CL? This
change has been up for review for 1.5 month already and it's been blocking
further development of the feature on v8 side. I believe that the main criterion
for the implementation is 0-impact on the performance when the tracking is off
and it was addressed. There are certainly other issues like tracking coalesced
allocations that we will have to resolve but I don't think we should try to
address everything in one CL.

[Hannes Payer (out of office), 2013-10-14 08:19:32]

If you do not think that the folding issue should be resolved now than LGTM from
my side.

[yurys, 2013-10-14 12:41:47]

Committed patchset #13 manually as r17191 (presubmit successful).