Heap profiler should report implicit references

Heap profiler should report implicit references.

Implicit references reported to V8 with V8::AddImplicitReferences calls are now reported by heap profiler as 'native' references with type kInternal.

Original WebKit bug report: https://bugs.webkit.org/show_bug.cgi?id=77414
Committed: https://code.google.com/p/v8/source/detail?r=10603

[loislo, 2012-02-03 13:30:29]

On 2012/02/03 12:21:48, Yury Semikhatsky wrote:

looks good

[mnaganov (inactive), 2012-02-03 13:57:19]

https://chromiumcodereview.appspot.com/9316092/diff/3005/src/profile-generato...
File src/profile-generator.cc (right):

https://chromiumcodereview.appspot.com/9316092/diff/3005/src/profile-generato...
src/profile-generator.cc:2801: "native",
Does this look descriptive enough? If implicit refs are only for event handlers,
perhaps we can call this more specific?

https://chromiumcodereview.appspot.com/9316092/diff/3005/src/profile-generato...
src/profile-generator.cc:2825: if (EstimateObjectsCount() <= 0) {
Let's rewrite this as follows to avoid cleanup code duplication:

if (EstimateObjectsCount() > 0) {
  for(...) {
  }
  SetRootNative...
}
filler_ = NULL;
return true;

https://chromiumcodereview.appspot.com/9316092/diff/3005/test/cctest/test-hea...
File test/cctest/test-heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/9316092/diff/3005/test/cctest/test-hea...
test/cctest/test-heap-profiler.cc:813: for (int i = 0; i < 4; i++)
nit: brackets

https://chromiumcodereview.appspot.com/9316092/diff/3005/test/cctest/test-hea...
test/cctest/test-heap-profiler.cc:832:
v8::Persistent<v8::Object>::Cast(objects_[1]), &objects_[2], 2);
Have you forgot to add 1->3 reference, or what does this comment mean?

https://chromiumcodereview.appspot.com/9316092/diff/3005/test/cctest/test-hea...
test/cctest/test-heap-profiler.cc:835: v8::Persistent<v8::Value> objects_[4];
I think, using an enum for test objects and their count will be more readable
and robust.

https://chromiumcodereview.appspot.com/9316092/diff/3005/test/cctest/test-hea...
test/cctest/test-heap-profiler.cc:854: const v8::HeapGraphNode* o0 =
GetProperty(
This name looks weird. In other tests names like "obj0" are used.

[yurys, 2012-02-03 14:09:25]

https://chromiumcodereview.appspot.com/9316092/diff/3005/src/profile-generato...
File src/profile-generator.cc (right):

https://chromiumcodereview.appspot.com/9316092/diff/3005/src/profile-generato...
src/profile-generator.cc:2801: "native",
On 2012/02/03 13:57:19, Mikhail Naganov (Chromium) wrote:
> Does this look descriptive enough? If implicit refs are only for event
handlers,
> perhaps we can call this more specific?
For now it seems good enough. We cannot assume that all implicit references on
all embedders will always be for event handlers only even if it is so in
Chromium at the moment. We would like to extend information about such
references with e.g. event names and probably something else but for now just
having a strong reference reflected in the heap snapshot is enough.

https://chromiumcodereview.appspot.com/9316092/diff/3005/src/profile-generato...
src/profile-generator.cc:2825: if (EstimateObjectsCount() <= 0) {
On 2012/02/03 13:57:19, Mikhail Naganov (Chromium) wrote:
> Let's rewrite this as follows to avoid cleanup code duplication:
> 
> if (EstimateObjectsCount() > 0) {
>   for(...) {
>   }
>   SetRootNative...
> }
> filler_ = NULL;
> return true;

Done.

https://chromiumcodereview.appspot.com/9316092/diff/3005/test/cctest/test-hea...
File test/cctest/test-heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/9316092/diff/3005/test/cctest/test-hea...
test/cctest/test-heap-profiler.cc:813: for (int i = 0; i < 4; i++)
On 2012/02/03 13:57:19, Mikhail Naganov (Chromium) wrote:
> nit: brackets

Done. Are they required by V8 coding style?

https://chromiumcodereview.appspot.com/9316092/diff/3005/test/cctest/test-hea...
test/cctest/test-heap-profiler.cc:832:
v8::Persistent<v8::Object>::Cast(objects_[1]), &objects_[2], 2);
On 2012/02/03 13:57:19, Mikhail Naganov (Chromium) wrote:
> Have you forgot to add 1->3 reference, or what does this comment mean?
No, the comment is perfectly fine, although Ilya asked the same question. Note
that in the second call we pass array of length 2.

https://chromiumcodereview.appspot.com/9316092/diff/3005/test/cctest/test-hea...
test/cctest/test-heap-profiler.cc:835: v8::Persistent<v8::Value> objects_[4];
On 2012/02/03 13:57:19, Mikhail Naganov (Chromium) wrote:
> I think, using an enum for test objects and their count will be more readable
> and robust.
Well, I started with storing each object in its own field but it turned out that
the only thing we need of them is their ordinal number. What kind of enum
constants do you suggest? I'd leave this part as is.

https://chromiumcodereview.appspot.com/9316092/diff/3005/test/cctest/test-hea...
test/cctest/test-heap-profiler.cc:854: const v8::HeapGraphNode* o0 =
GetProperty(
On 2012/02/03 13:57:19, Mikhail Naganov (Chromium) wrote:
> This name looks weird. In other tests names like "obj0" are used.

Done.

[mnaganov (inactive), 2012-02-03 14:23:41]

LGTM with 2 comments to fix.

https://chromiumcodereview.appspot.com/9316092/diff/3005/test/cctest/test-hea...
File test/cctest/test-heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/9316092/diff/3005/test/cctest/test-hea...
test/cctest/test-heap-profiler.cc:813: for (int i = 0; i < 4; i++)
On 2012/02/03 14:09:26, Yury Semikhatsky wrote:
> On 2012/02/03 13:57:19, Mikhail Naganov (Chromium) wrote:
> > nit: brackets
> 
> Done. Are they required by V8 coding style?

Google C++ coding style says "conditional or loop statements with complex
conditions or statements may be more readable with curly braces". In V8, curly
braces are in general can be omitted only for trivial early returns from
functions.

https://chromiumcodereview.appspot.com/9316092/diff/2004/test/cctest/test-hea...
File test/cctest/test-heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/9316092/diff/2004/test/cctest/test-hea...
test/cctest/test-heap-profiler.cc:831: // 1 -> 2, 1 -> 3
Then please add "(note length = 2)" for clarity.

https://chromiumcodereview.appspot.com/9316092/diff/2004/test/cctest/test-hea...
test/cctest/test-heap-profiler.cc:836: v8::Persistent<v8::Value> objects_[4];
OK, at least, please introduce a constant for the array length.

[yurys, 2012-02-03 14:50:57]

https://chromiumcodereview.appspot.com/9316092/diff/2004/test/cctest/test-hea...
File test/cctest/test-heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/9316092/diff/2004/test/cctest/test-hea...
test/cctest/test-heap-profiler.cc:831: // 1 -> 2, 1 -> 3
On 2012/02/03 14:23:41, Mikhail Naganov (Chromium) wrote:
> Then please add "(note length = 2)" for clarity.

Done.

https://chromiumcodereview.appspot.com/9316092/diff/2004/test/cctest/test-hea...
test/cctest/test-heap-profiler.cc:836: v8::Persistent<v8::Value> objects_[4];
On 2012/02/03 14:23:41, Mikhail Naganov (Chromium) wrote:
> OK, at least, please introduce a constant for the array length.

Done.