Fix huge heap snapshot when a heavily shared context has many variables

Fix huge heap snapshot when a heavily shared context has many variables

Prevously v8 put a link to each context variable into a function where
the variable is visible.
Because of that if there are N functions sharing a context having M variables
then N*M links were created for the snapshot.
The fix makes v8 to put the links into the context object.

BUG=145687
TEST=test-heap-snapshot/ManyLocalsInSharedContext

Committed: 13936

[Yury Semikhatsky, 2013-03-13 13:40:50]

https://codereview.appspot.com/7715044/diff/1/src/heap-snapshot-generator.cc
File src/heap-snapshot-generator.cc (left):

https://codereview.appspot.com/7715044/diff/1/src/heap-snapshot-generator.cc#...
src/heap-snapshot-generator.cc:1301: if (scope_info->HasFunctionName()) {
What was the intent of this code and why is it not needed any more?

https://codereview.appspot.com/7715044/diff/1/src/heap-snapshot-generator.h
File src/heap-snapshot-generator.h (right):

https://codereview.appspot.com/7715044/diff/1/src/heap-snapshot-generator.h#n...
src/heap-snapshot-generator.h:486: int field_offset = -1);
Why do you need default value here? It seems that the code will fail in case of
field_offset == -1.

https://codereview.appspot.com/7715044/diff/1/test/cctest/test-heap-profiler.cc
File test/cctest/test-heap-profiler.cc (right):

https://codereview.appspot.com/7715044/diff/1/test/cctest/test-heap-profiler....
test/cctest/test-heap-profiler.cc:1730: 
Can we also test that context variables are referenced by links with
user-friendly names?

[alph, 2013-03-13 16:08:11]

Comments addressed.

https://codereview.appspot.com/7715044/diff/1/src/heap-snapshot-generator.cc
File src/heap-snapshot-generator.cc (left):

https://codereview.appspot.com/7715044/diff/1/src/heap-snapshot-generator.cc#...
src/heap-snapshot-generator.cc:1301: if (scope_info->HasFunctionName()) {
On 2013/03/13 13:40:51, yurys wrote:
> What was the intent of this code and why is it not needed any more?
It is needed. Put it back.
For some reason names of named closures are not listed in the scope_info object,
so we have to add them explicitly.

https://codereview.appspot.com/7715044/diff/1/src/heap-snapshot-generator.h
File src/heap-snapshot-generator.h (right):

https://codereview.appspot.com/7715044/diff/1/src/heap-snapshot-generator.h#n...
src/heap-snapshot-generator.h:486: int field_offset = -1);
On 2013/03/13 13:40:51, yurys wrote:
> Why do you need default value here? It seems that the code will fail in case
of
> field_offset == -1.
It won't fail. MarkVisitedField checks the offset and if it is negative it does
nothing. In this case the field will also be added as a hidden reference by
IndexedReferencesExtractor. I've made a default here to be uniform to other
Set*Reference functions that accept field_offset.

https://codereview.appspot.com/7715044/diff/1/test/cctest/test-heap-profiler.cc
File test/cctest/test-heap-profiler.cc (right):

https://codereview.appspot.com/7715044/diff/1/test/cctest/test-heap-profiler....
test/cctest/test-heap-profiler.cc:1730: 
On 2013/03/13 13:40:51, yurys wrote:
> Can we also test that context variables are referenced by links with
> user-friendly names?
ok

[Yury Semikhatsky, 2013-03-13 16:22:18]

lgtm

https://codereview.appspot.com/7715044/diff/7001/test/cctest/test-heap-profil...
File test/cctest/test-heap-profiler.cc (right):

https://codereview.appspot.com/7715044/diff/7001/test/cctest/test-heap-profil...
test/cctest/test-heap-profiler.cc:1734: int num_objects = 6500;
File a bug that in case of 10 000 objects it crashes?

https://codereview.appspot.com/7715044/diff/7001/test/cctest/test-heap-profil...
test/cctest/test-heap-profiler.cc:1760:
CHECK_LE(context_object->GetChildrenCount(), num_objects + 10);
10 ->  Context::MIN_CONTEXT_SLOTS?

[alph, 2013-03-13 16:56:46]

1. addressed comments
2. made contexts non-hidden by default
3. reduced number of functions in test down to 5000, as 6500 turned out to crash
x64.debug

TBR=yurys

[alph, 2013-03-13 17:20:44]

Please take a look

[Jakob, 2013-03-13 17:22:56]

lgtm

[alph, 2013-03-13 17:38:12]

Committed patchset #4 manually as r13936 (presubmit successful).

[alph, 2013-03-13 18:03:09]

https://codereview.appspot.com/7715044/diff/7001/test/cctest/test-heap-profil...
File test/cctest/test-heap-profiler.cc (right):

https://codereview.appspot.com/7715044/diff/7001/test/cctest/test-heap-profil...
test/cctest/test-heap-profiler.cc:1734: int num_objects = 6500;
On 2013/03/13 16:22:18, yurys wrote:
> File a bug that in case of 10 000 objects it crashes?
sure. https://code.google.com/p/v8/issues/detail?id=2576

https://codereview.appspot.com/7715044/diff/7001/test/cctest/test-heap-profil...
test/cctest/test-heap-profiler.cc:1760:
CHECK_LE(context_object->GetChildrenCount(), num_objects + 10);
On 2013/03/13 16:22:18, yurys wrote:
> 10 ->  Context::MIN_CONTEXT_SLOTS?

Done.

[Yury Semikhatsky, 2013-03-14 06:45:31]

https://codereview.appspot.com/7715044/diff/19002/include/v8-profiler.h
File include/v8-profiler.h (right):

https://codereview.appspot.com/7715044/diff/19002/include/v8-profiler.h#newco...
include/v8-profiler.h:263: kContext = 10     // Context
Why do we have to introduce new edge type for the link? We were thinking about
reducing the number to simplify snapshots.

[alph, 2013-03-14 10:36:40]

https://codereview.appspot.com/7715044/diff/19002/include/v8-profiler.h
File include/v8-profiler.h (right):

https://codereview.appspot.com/7715044/diff/19002/include/v8-profiler.h#newco...
include/v8-profiler.h:263: kContext = 10     // Context
On 2013/03/14 06:45:31, yurys wrote:
> Why do we have to introduce new edge type for the link? We were thinking about
> reducing the number to simplify snapshots.
This is not a new edge type. This is node type.