On 2012/04/11 14:35:40, Mikhail Naganov (Chromium) wrote:
https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8-profiler.h
File include/v8-profiler.h (right):
https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8-profiler.h#newcode420
include/v8-profiler.h:420: static void StartHeapObjectsTracking();
Comments are required.
Done.
https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8-profiler.h#newcode422
include/v8-profiler.h:422: static void PushHeapObjectsStats(OutputStream*
stream);
Why "Push"? Maybe "Serialize"?
It is not a serialization. I'm not sure that I like this term.
https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8.h
File include/v8.h (right):
https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8.h#newcode3745
include/v8.h:3745: kUint32 = 1
Any particular reason for not sending this data as ASCII / JSON? This is
just
a
stream of numbers, AFAICT.
I'd like to upstream serialization part of the profiler code to WebCore.
It will significantly increase our productivity and simplify the business
process.
https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8.h#newcode3764
include/v8.h:3764:
nit: no blank line
done
https://chromiumcodereview.appspot.com/10049002/diff/4001/include/v8.h#newcode3766
include/v8.h:3766: * Writes the next chunk of snapshot data into the
stream.
Writing
Does it really write snapshot data?
done
https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generator.cc
File src/profile-generator.cc (right):
https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generator.cc#newcode1409
src/profile-generator.cc:1409: SnapshotObjectId
HeapObjectsMap::FindOrAddEntry(Address addr) {
This functions seems to reuse code from FindEntry, FindObject and
AddEntry.
Can
you please try to avoid code duplication?
It has single look-up call.
https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generator.cc#newcode1455
src/profile-generator.cc:1455: int collected_fragment_updates = 0;
This variable seems to be write-only.
done.
https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generator.h
File src/profile-generator.h (right):
https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generator.h#newcode758
src/profile-generator.h:758: struct FragmentInfo {
Please move under EntryInfo definition.
done
https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generator.h#newcode760
src/profile-generator.h:760: SnapshotObjectId id;
'id' isn't very descriptive. Should it be something like 'last_id'?
mmm
https://chromiumcodereview.appspot.com/10049002/diff/4001/src/profile-generator.h#newcode778
src/profile-generator.h:778: void StartHeapObjectsTracking() {
I think, non-trivial methods should be implemented in the .cc file.
it is trivial now.
https://chromiumcodereview.appspot.com/10049002/diff/4001/test/cctest/test-heap-profiler.cc
File test/cctest/test-heap-profiler.cc (right):
https://chromiumcodereview.appspot.com/10049002/diff/4001/test/cctest/test-heap-profiler.cc#newcode749
test/cctest/test-heap-profiler.cc:749: v8::Persistent<v8::String>
p_string =
This handle can be local, you have a HandleScope at the function level,
so the
string will be kept alive until you return.
done
https://chromiumcodereview.appspot.com/10049002/diff/4001/test/cctest/test-heap-profiler.cc#newcode766
test/cctest/test-heap-profiler.cc:766:
I think, you should also test stats update on object deletion.
The code with persistent handles is very complicated. I did that with Local
and
HandleScope.
https://chromiumcodereview.appspot.com/10049002/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
