Merge the Merlin heap tracing to top-of-trunk.

Merge the Merlin heap tracing to top-of-trunk.

[cshapiro, 2012-12-04 03:59:20]

Ping?

[Ivan Posva, 2012-12-04 16:32:52]

I will not be able to get through all of this today. But here is at least one
comment about regularizing the file handling apis in the Dart C API.

-Ivan

https://codereview.chromium.org/11428067/diff/5001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/11428067/diff/5001/runtime/include/dart_api.h...
runtime/include/dart_api.h:2763: DART_EXPORT void
Dart_InitHeapTrace(Dart_FileOpenCallback open_function,
Shouldn't we just register a general open, write and close callback? And then
init heap trace or the other operations with a name?

[cshapiro, 2012-12-04 21:13:32]

https://codereview.chromium.org/11428067/diff/5001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/11428067/diff/5001/runtime/include/dart_api.h...
runtime/include/dart_api.h:2763: DART_EXPORT void
Dart_InitHeapTrace(Dart_FileOpenCallback open_function,
Sure, I can do that.  I would like the streams for the heap trace to do I/O
asynchronously.  This might be okay for other kinds of I/O but I am not entirely
certain.

[siva, 2012-12-05 16:06:40]

I uploaded the initial set of comments I had so far. I am still going through
the code.

As discussed offline it would be nice to do the dart API file support in a CL by
itself.

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/assemble...
File runtime/vm/assembler_ia32.cc (right):

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/assemble...
runtime/vm/assembler_ia32.cc:1699:
Immediate(reinterpret_cast<int32_t>(value.raw())));
This is not right, the raw value is directly being written into the instruction.
Not sure how this works.

Should be:
LoadObject(EAX, value);
movl(Address(ESP, 2 * kWordSize), EAX);

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/class_ta...
File runtime/vm/class_table.cc (right):

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/class_ta...
runtime/vm/class_table.cc:54:
Isolate::Current()->heap()->trace()->TraceRegisterClass(cls.raw());
Passing in a raw pointer to a function like this is not quite our style, we
always pass in a handle.

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/class_ta...
runtime/vm/class_table.cc:80:
Isolate::Current()->heap()->trace()->TraceRegisterClass(cls.raw());
Ditto.

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/code_gen...
File runtime/vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/code_gen...
runtime/vm/code_generator.cc:1911: if (!(object->IsHeapObject() &&
value->IsHeapObject())) {
I am a little confused by this check. Is it here to filter out stores to Smi?

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap.cc#...
runtime/vm/heap.cc:43: heap_trace_ = HeapTrace::is_enabled() ? new HeapTrace :
NULL;
new HeapTrace() ?

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap.h
File runtime/vm/heap.h (right):

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap.h#n...
runtime/vm/heap.h:154: HeapTrace* trace() { return heap_trace_; }
const

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap_tra...
File runtime/vm/heap_trace.cc (right):

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap_tra...
runtime/vm/heap_trace.cc:2: // Author: nricci@google.com (Nathan Ricci)
This copyright doesn't seem to be standard.

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap_tra...
runtime/vm/heap_trace.cc:4: #include "include/dart_api.h"
blank line.

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap_tra...
runtime/vm/heap_trace.cc:175: intptr_t len = OS::SNPrint(NULL, 0, format,
file_name_prefix_, isolate);
This seems kind of ad-hoc to use the isolate pointer in the name of the file.
Why not use the isolate->name() function which Todd added for the tracing
facility.

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap_tra...
File runtime/vm/heap_trace.h (right):

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap_tra...
runtime/vm/heap_trace.h:2: // Author: nricci@google.com (Nathan Ricci)
This copyright doesn't seem to be standard.

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap_tra...
runtime/vm/heap_trace.h:7: #include "include/dart_api.h"
blank line.

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap_tra...
runtime/vm/heap_trace.h:40: };
These constants don't seem to be an increasing sequence
why not declare them as
const intptr_t kRootSize = 5;
etc.
instead of an enum

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap_tra...
runtime/vm/heap_trace.h:59: };
Ditto.

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap_tra...
runtime/vm/heap_trace.h:79: intptr_t Length() { return cursor_; }
intptr_t Length() const { ... }

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap_tra...
runtime/vm/heap_trace.h:81: const uint8_t* Buffer() {
Ditto.

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/heap_tra...
runtime/vm/heap_trace.h:89: HeapTrace* trace_;
Missing DISALLOW stuff?

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/object_s...
File runtime/vm/object_set.h (right):

https://chromiumcodereview.appspot.com/11428067/diff/5001/runtime/vm/object_s...
runtime/vm/object_set.h:10: #include "platform/utils.h"
This order is not consistent with most other files.

[cshapiro, 2012-12-08 03:23:08]

PTAL

I would really like to get this checked in soon.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/assembler_ia32....
runtime/vm/assembler_ia32.cc:1699:
Immediate(reinterpret_cast<int32_t>(value.raw())));
This method turns out to not be needed so I will be avoiding this issue all
together by removing this method and its calls.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/class_table.cc
File runtime/vm/class_table.cc (right):

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/class_table.cc#...
runtime/vm/class_table.cc:54:
Isolate::Current()->heap()->trace()->TraceRegisterClass(cls.raw());
Fixed.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/class_table.cc#...
runtime/vm/class_table.cc:80:
Isolate::Current()->heap()->trace()->TraceRegisterClass(cls.raw());
Fixed.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/code_generator....
runtime/vm/code_generator.cc:1911: if (!(object->IsHeapObject() &&
value->IsHeapObject())) {
This check ensures that both the source and destination are values and rejects
all other cases.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/heap.cc#newcode43
runtime/vm/heap.cc:43: heap_trace_ = HeapTrace::is_enabled() ? new HeapTrace :
NULL;
Parenthesis are not required.  We do not put parenthesis around every 0 argument
construction.  Is there a reason to start now.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/heap.h
File runtime/vm/heap.h (right):

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/heap.h#newcode154
runtime/vm/heap.h:154: HeapTrace* trace() { return heap_trace_; }
Fixed.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/heap_trace.cc
File runtime/vm/heap_trace.cc (right):

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/heap_trace.cc#n...
runtime/vm/heap_trace.cc:2: // Author: nricci@google.com (Nathan Ricci)
On 2012/12/05 16:06:40, siva wrote:
> This copyright doesn't seem to be standard.

Done.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/heap_trace.cc#n...
runtime/vm/heap_trace.cc:4: #include "include/dart_api.h"
On 2012/12/05 16:06:40, siva wrote:
> blank line.

Done.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/heap_trace.cc#n...
runtime/vm/heap_trace.cc:175: intptr_t len = OS::SNPrint(NULL, 0, format,
file_name_prefix_, isolate);
Good idea.  Fixed.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/heap_trace.h
File runtime/vm/heap_trace.h (right):

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/heap_trace.h#ne...
runtime/vm/heap_trace.h:2: // Author: nricci@google.com (Nathan Ricci)
It is standard, but not the standard we use.  Fixed.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/heap_trace.h#ne...
runtime/vm/heap_trace.h:7: #include "include/dart_api.h"
Done.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/heap_trace.h#ne...
runtime/vm/heap_trace.h:40: };
I think the enum makes it a bit more descriptive what type is being admitted
some place.  I think this is pretty standard practice (to use enums as a way of
defining a bunch of named constants).  However, if this is really bothersome, I
will replace it with a bunch of static const ints.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/heap_trace.h#ne...
runtime/vm/heap_trace.h:79: intptr_t Length() { return cursor_; }
On 2012/12/05 16:06:40, siva wrote:
> intptr_t Length() const { ... }

Done.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/heap_trace.h#ne...
runtime/vm/heap_trace.h:81: const uint8_t* Buffer() {
On 2012/12/05 16:06:40, siva wrote:
> Ditto.

Done.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/heap_trace.h#ne...
runtime/vm/heap_trace.h:89: HeapTrace* trace_;
On 2012/12/05 16:06:40, siva wrote:
> Missing DISALLOW stuff?

Done.

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/object_set.h
File runtime/vm/object_set.h (right):

https://codereview.chromium.org/11428067/diff/5001/runtime/vm/object_set.h#ne...
runtime/vm/object_set.h:10: #include "platform/utils.h"
Correct.  Done.

[cshapiro, 2012-12-13 00:41:58]

Ping?

[siva, 2012-12-14 02:11:54]

lgtm with some nits.

I haven't gone over the heap_trace* files in detail. I will
do that offline.

I also noted that the x64 version of changes in the
assembler files are missing.

https://chromiumcodereview.appspot.com/11428067/diff/17001/runtime/vm/assembl...
File runtime/vm/assembler_ia32.cc (right):

https://chromiumcodereview.appspot.com/11428067/diff/17001/runtime/vm/assembl...
runtime/vm/assembler_ia32.cc:1657: const Object& value) {
Got rid of the TraceStoreIntoObject from here?
Maybe we should add a comment here stating that tracing does not care about old
objects being stored.

https://chromiumcodereview.appspot.com/11428067/diff/17001/runtime/vm/handles.h
File runtime/vm/handles.h (right):

https://chromiumcodereview.appspot.com/11428067/diff/17001/runtime/vm/handles...
runtime/vm/handles.h:88: // Visit only the scoped handles
missing period.

https://chromiumcodereview.appspot.com/11428067/diff/17001/runtime/vm/handles...
File runtime/vm/handles_impl.h (right):

https://chromiumcodereview.appspot.com/11428067/diff/17001/runtime/vm/handles...
runtime/vm/handles_impl.h:84: } while (block != NULL);
Can we use the new VisitScopedHandles(visitor) here and avoid the code
duplication.

https://chromiumcodereview.appspot.com/11428067/diff/17001/runtime/vm/heap_tr...
File runtime/vm/heap_trace.cc (right):

https://chromiumcodereview.appspot.com/11428067/diff/17001/runtime/vm/heap_tr...
runtime/vm/heap_trace.cc:38: void VisitPointers(RawObject** first, RawObject**
last) {
Can this be a const function? Here and other classes in this file.

https://chromiumcodereview.appspot.com/11428067/diff/17001/runtime/vm/heap_tr...
runtime/vm/heap_trace.cc:60: // TODO(nricci): replace this with a map or
something else sparse.
Do you want to retain Nathan's handle in the TODO here?

https://chromiumcodereview.appspot.com/11428067/diff/17001/runtime/vm/heap_tr...
runtime/vm/heap_trace.cc:61: ObjectSet* object_set_;
Missing DISALLOW stuff.

https://chromiumcodereview.appspot.com/11428067/diff/17001/runtime/vm/heap_tr...
runtime/vm/heap_trace.cc:88: HeapTrace* heap_trace_;
Ditto.

https://chromiumcodereview.appspot.com/11428067/diff/17001/runtime/vm/heap_tr...
runtime/vm/heap_trace.cc:113: HeapTrace* heap_trace_;
Ditto.

https://chromiumcodereview.appspot.com/11428067/diff/17001/runtime/vm/heap_tr...
runtime/vm/heap_trace.cc:127: HeapTrace* heap_trace_;
Ditto.

[cshapiro, 2012-12-15 19:56:49]

https://codereview.chromium.org/11428067/diff/17001/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

https://codereview.chromium.org/11428067/diff/17001/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32.cc:1657: const Object& value) {
Comment added.

https://codereview.chromium.org/11428067/diff/17001/runtime/vm/handles.h
File runtime/vm/handles.h (right):

https://codereview.chromium.org/11428067/diff/17001/runtime/vm/handles.h#newc...
runtime/vm/handles.h:88: // Visit only the scoped handles
On 2012/12/14 02:11:54, siva wrote:
> missing period.

Done.

https://codereview.chromium.org/11428067/diff/17001/runtime/vm/handles_impl.h
File runtime/vm/handles_impl.h (right):

https://codereview.chromium.org/11428067/diff/17001/runtime/vm/handles_impl.h...
runtime/vm/handles_impl.h:84: } while (block != NULL);
Sure can.  Done.

https://codereview.chromium.org/11428067/diff/17001/runtime/vm/heap_trace.cc
File runtime/vm/heap_trace.cc (right):

https://codereview.chromium.org/11428067/diff/17001/runtime/vm/heap_trace.cc#...
runtime/vm/heap_trace.cc:38: void VisitPointers(RawObject** first, RawObject**
last) {
Probably not, at least not without defining a const VisitPointers method in
ObjectPointerVisitor.  We could add that, but it would probably require some
changes to ObjectPointerVisitor.

https://codereview.chromium.org/11428067/diff/17001/runtime/vm/heap_trace.cc#...
runtime/vm/heap_trace.cc:60: // TODO(nricci): replace this with a map or
something else sparse.
Fixed.

https://codereview.chromium.org/11428067/diff/17001/runtime/vm/heap_trace.cc#...
runtime/vm/heap_trace.cc:61: ObjectSet* object_set_;
Done.

https://codereview.chromium.org/11428067/diff/17001/runtime/vm/heap_trace.cc#...
runtime/vm/heap_trace.cc:88: HeapTrace* heap_trace_;
Done.

https://codereview.chromium.org/11428067/diff/17001/runtime/vm/heap_trace.cc#...
runtime/vm/heap_trace.cc:113: HeapTrace* heap_trace_;
Done.

https://codereview.chromium.org/11428067/diff/17001/runtime/vm/heap_trace.cc#...
runtime/vm/heap_trace.cc:127: HeapTrace* heap_trace_;
Done.