HeapProfiler: provide human readable names for code objects.

HeapProfiler: provide human readable names for code objects.

It is very hard to understand the structure of the heap even for about:blank page
because code objects in the heap have no names. This patch propagates the names
for Code::STUB and Code::BUILTIN code objects.
Also it assign function names from SharedFunctionInfo to the code objects.

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

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

[yurys, 2013-10-30 15:00:34]

Can you add a test for this?

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

https://codereview.chromium.org/52643002/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:1382: shared->inferred_name();
DebugName should already return inferred_name if there is no name.

https://codereview.chromium.org/52643002/diff/1/src/heap-snapshot-generator.c...
src/heap-snapshot-generator.cc:1481:
CodeStub::MajorName(static_cast<CodeStub::Major>(code->major_key()),
Please use CodeStub::GetName() instead, it would give more details.

[loislo, 2013-10-31 09:40:24]

> Can you add a test for this?

done

> 
> https://codereview.chromium.org/52643002/diff/1/src/heap-snapshot-generator.cc
> File src/heap-snapshot-generator.cc (right):
> 
>
https://codereview.chromium.org/52643002/diff/1/src/heap-snapshot-generator.c...
> src/heap-snapshot-generator.cc:1382: shared->inferred_name();
> DebugName should already return inferred_name if there is no name.

done

> 
>
https://codereview.chromium.org/52643002/diff/1/src/heap-snapshot-generator.c...
> src/heap-snapshot-generator.cc:1481:
> CodeStub::MajorName(static_cast<CodeStub::Major>(code->major_key()),
> Please use CodeStub::GetName() instead, it would give more details.

There is no way to get a pointer to a CodeStub object from Code object.
The common pattern for CodeStubs is to instantiate a CodeStub object,
force them to create the corresponding Code object and destroy the CodeStub
object.

[yurys, 2013-10-31 09:54:01]

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

https://codereview.chromium.org/52643002/diff/50001/src/heap-snapshot-generat...
src/heap-snapshot-generator.cc:1380: shared_name = !shared_name->IsFailure() ?
shared_name : shared->DebugName();
shared_name->IsFailure() will always be true here since we just visited the
name() field at line 1374 by calling SetInternalReference.

[loislo, 2013-10-31 10:59:39]

comments addressed

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

https://codereview.chromium.org/52643002/diff/50001/src/heap-snapshot-generat...
src/heap-snapshot-generator.cc:1380: shared_name = !shared_name->IsFailure() ?
shared_name : shared->DebugName();
On 2013/10/31 09:54:01, Yury Semikhatsky wrote:
> shared_name->IsFailure() will always be true here since we just visited the
> name() field at line 1374 by calling SetInternalReference.

Done.

[alph, 2013-10-31 11:42:34]

https://codereview.chromium.org/52643002/diff/100001/src/heap-snapshot-genera...
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/52643002/diff/100001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1377: shared_name = (shared_name != empty_string)
?
Doesn't DebugName already have the semantics you're implementing here?

https://codereview.chromium.org/52643002/diff/100001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1472: void V8HeapExplorer::TagCodeObject(Code*
code, const char* external_name) {
nit: looks like having an overload here is more appropriate. wdyt?

[loislo, 2013-10-31 12:33:14]

comments addressed

https://codereview.chromium.org/52643002/diff/100001/src/heap-snapshot-genera...
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/52643002/diff/100001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1377: shared_name = (shared_name != empty_string)
?
On 2013/10/31 11:42:34, alph wrote:
> Doesn't DebugName already have the semantics you're implementing here?

done

https://codereview.chromium.org/52643002/diff/100001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1472: void V8HeapExplorer::TagCodeObject(Code*
code, const char* external_name) {
On 2013/10/31 11:42:34, alph wrote:
> nit: looks like having an overload here is more appropriate. wdyt?

I don't think that we need additional function here.

[alph, 2013-10-31 12:55:46]

lgtm

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1375: Object* shared_name = shared->DebugName();
nit: DebugName returns String, so you should not need a type cast at 1378.

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1758: int strong_index = 0, all_index = 0,
tags_index = 0, prev_tag_index = 0;
maybe prev_tag_index -> tag_first_index ?

[yurys, 2013-10-31 14:24:02]

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1382:
Code::Kind2String(shared->code()->kind())));
style: indent 4 characters

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1476: case Code::STUB: {
if (code->kind() == Code::STUB) { would be more concise.

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1772: && all_references_[all_index]->IsCode()) {
This should always be a code. But we can leave this check to be on the safe
side.

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1774: builtins->name(all_index -
prev_tag_index));
Can we replace this prev_tag_index with builtin_index and use it here instead?
...
if (all_references_[all_index]->IsCode()) {
  explorer->TagCodeObject(Code::cast(all_references_[all_index]),
     builtins->name(builtin_index));
}
builtin_index++;
...

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1777: if (reference_tags_[tags_index].index ==
all_index) {
As discussed offline this should be 
while (reference_tags_[tags_index].index == all_index) ++tags_index;
to work correctly in case there are empty groups. This can be addressed
separately though with a separate test.

[alph, 2013-10-31 14:28:37]

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1476: case Code::STUB: {
On 2013/10/31 14:24:02, Yury Semikhatsky wrote:
> if (code->kind() == Code::STUB) { would be more concise.

... and make the function overloaded ;-)

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1774: builtins->name(all_index -
prev_tag_index));
On 2013/10/31 14:24:02, Yury Semikhatsky wrote:
> Can we replace this prev_tag_index with builtin_index and use it here instead?
> ...
> if (all_references_[all_index]->IsCode()) {
>   explorer->TagCodeObject(Code::cast(all_references_[all_index]),
>      builtins->name(builtin_index));
> }
> builtin_index++;
> ...

I'd event move the increment inside if

[loislo, 2013-10-31 14:41:13]

comments addressed

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1375: Object* shared_name = shared->DebugName();
On 2013/10/31 12:55:46, alph wrote:
> nit: DebugName returns String, so you should not need a type cast at 1378.

Done.

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1382:
Code::Kind2String(shared->code()->kind())));
On 2013/10/31 14:24:02, Yury Semikhatsky wrote:
> style: indent 4 characters

Done.

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1476: case Code::STUB: {
On 2013/10/31 14:24:02, Yury Semikhatsky wrote:
> if (code->kind() == Code::STUB) { would be more concise.

Done.

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1476: case Code::STUB: {
On 2013/10/31 14:28:38, alph wrote:
> On 2013/10/31 14:24:02, Yury Semikhatsky wrote:
> > if (code->kind() == Code::STUB) { would be more concise.
> 
> ... and make the function overloaded ;-)

Done.

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1772: && all_references_[all_index]->IsCode()) {
On 2013/10/31 14:24:02, Yury Semikhatsky wrote:
> This should always be a code. But we can leave this check to be on the safe
> side.

I'll make ASSERT(all_references_[all_index]->IsCode())

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1774: builtins->name(all_index -
prev_tag_index));
On 2013/10/31 14:24:02, Yury Semikhatsky wrote:
> Can we replace this prev_tag_index with builtin_index and use it here instead?
> ...
> if (all_references_[all_index]->IsCode()) {
>   explorer->TagCodeObject(Code::cast(all_references_[all_index]),
>      builtins->name(builtin_index));
> }
> builtin_index++;
> ...

Done.

[yurys, 2013-10-31 15:11:24]

lgtm

[yurys, 2013-11-01 14:55:04]

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/52643002/diff/190001/src/heap-snapshot-genera...
src/heap-snapshot-generator.cc:1777: if (reference_tags_[tags_index].index ==
all_index) {
On 2013/10/31 14:24:02, Yury Semikhatsky wrote:
> As discussed offline this should be 
> while (reference_tags_[tags_index].index == all_index) ++tags_index;
> to work correctly in case there are empty groups. This can be addressed
> separately though with a separate test.

On the second look the current implementation is correct as the tags for empty
groups will be skipped in  RootsReferencesExtractor::Synchronize

[Sven Panne, 2013-11-05 13:13:11]

LGTM (rubberstamped)

[loislo, 2013-11-05 13:23:13]

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