Create a internal, global native context used only for generated code stubs

Create a internal native context used only for TF-generated code stubs

Until now, TF-generated code stubs piggy-backed off of the builtin
context. Since generation of code stubs is lazy, stubs generated at
different times in different native contexts would contain embedded
pointers different builtin contexts, leading to cross-context references
and memory leaks.

After this CL, all TF-generated code stubs are generated inside a
internal thinned-out, native context that lives solely for the
purpose of hosting generated code stubs.

Committed: https://crrev.com/a1475dae5d147cac78812cd4fd09f886b51a88ef
Cr-Commit-Position: refs/heads/master@{#29593}

[danno, 2015-07-07 12:07:30]

danno@chromium.org changed reviewers:
+ yangguo@chromium.org

[danno, 2015-07-07 12:26:06]

danno@chromium.org changed reviewers:
+ bmeurer@chromium.org, mstarzinger@chromium.org

[danno, 2015-07-07 12:26:07]

Please take a look, with special focus on:
- Yang: Serialization changes
- Michi: Boostrapper changes
- Benedikt: Code stub generation changes

[danno, 2015-07-07 13:51:30]

The CQ bit was checked by danno@chromium.org

[danno, 2015-07-07 13:51:31]

The CQ bit was unchecked by danno@chromium.org

[Yang, 2015-07-08 08:32:54]

some comments.

https://codereview.chromium.org/1213203007/diff/220001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1213203007/diff/220001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:1106: 
Do you really need Number, Boolean, String, Symbol, Date and RegExp objects?

https://codereview.chromium.org/1213203007/diff/220001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:2075: int i = Natives::GetDebuggerCount();
I'd rename 'i' into 'js_builtins_script_index'.

https://codereview.chromium.org/1213203007/diff/220001/src/snapshot/serialize.cc
File src/snapshot/serialize.cc (right):

https://codereview.chromium.org/1213203007/diff/220001/src/snapshot/serialize...
src/snapshot/serialize.cc:1197: int index = source_.Get();
This duplicate code could be refactored into a helper function that takes a
Vector<const char>. Or you could handle both cases at once and then choose the
script source depending on the value of |data|.

https://codereview.chromium.org/1213203007/diff/220001/src/snapshot/serialize...
src/snapshot/serialize.cc:1443: context->set(Context::NEXT_CONTEXT_LINK,
Some comment here would be great.

https://codereview.chromium.org/1213203007/diff/220001/src/snapshot/serialize...
src/snapshot/serialize.cc:1653: // Make sure that all contexts are derived from
the code-stub context
... all *functions* are derived ...

https://codereview.chromium.org/1213203007/diff/220001/src/snapshot/serialize...
src/snapshot/serialize.cc:1655: Context* current =
JSFunction::cast(obj)->context();
Can you replace this with

DCHECK(!obj->IsJSFunction() || obj->GetCreationContext() ==
isolate()->heap()->code_stub_context())

There is a small difference between the creation context and the function
context, but the native context of the latter should be the same. Either way you
save yourself that explicit context walk.

https://codereview.chromium.org/1213203007/diff/220001/src/snapshot/serialize...
src/snapshot/serialize.cc:2184: for (int i = 0; i <
CodeStubNatives::GetBuiltinsCount(); i++) {
Could you refactor this duplicate code into a helper function that takes the
cache and the builtins count as arguments?

[danno, 2015-07-08 20:25:59]

Please take another look

https://codereview.chromium.org/1213203007/diff/220001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1213203007/diff/220001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:1106: 
On 2015/07/08 08:32:53, Yang wrote:
> Do you really need Number, Boolean, String, Symbol, Date and RegExp objects?

Yes. Unfortunately the rest of the initialization sequence requires some of
these. I could selectively remove one or two, but this seemed more consistent.

https://codereview.chromium.org/1213203007/diff/220001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:2075: int i = Natives::GetDebuggerCount();
On 2015/07/08 08:32:53, Yang wrote:
> I'd rename 'i' into 'js_builtins_script_index'.

Done.

https://codereview.chromium.org/1213203007/diff/220001/src/snapshot/serialize.cc
File src/snapshot/serialize.cc (right):

https://codereview.chromium.org/1213203007/diff/220001/src/snapshot/serialize...
src/snapshot/serialize.cc:1197: int index = source_.Get();
On 2015/07/08 08:32:54, Yang wrote:
> This duplicate code could be refactored into a helper function that takes a
> Vector<const char>. Or you could handle both cases at once and then choose the
> script source depending on the value of |data|.

Done.

https://codereview.chromium.org/1213203007/diff/220001/src/snapshot/serialize...
src/snapshot/serialize.cc:1443: context->set(Context::NEXT_CONTEXT_LINK,
On 2015/07/08 08:32:54, Yang wrote:
> Some comment here would be great.

Done.

https://codereview.chromium.org/1213203007/diff/220001/src/snapshot/serialize...
src/snapshot/serialize.cc:1653: // Make sure that all contexts are derived from
the code-stub context
On 2015/07/08 08:32:54, Yang wrote:
> ... all *functions* are derived ...

Done.

https://codereview.chromium.org/1213203007/diff/220001/src/snapshot/serialize...
src/snapshot/serialize.cc:1655: Context* current =
JSFunction::cast(obj)->context();
On 2015/07/08 08:32:54, Yang wrote:
> Can you replace this with
> 
> DCHECK(!obj->IsJSFunction() || obj->GetCreationContext() ==
> isolate()->heap()->code_stub_context())
> 
> There is a small difference between the creation context and the function
> context, but the native context of the latter should be the same. Either way
you
> save yourself that explicit context walk.

Done.

https://codereview.chromium.org/1213203007/diff/220001/src/snapshot/serialize...
src/snapshot/serialize.cc:2184: for (int i = 0; i <
CodeStubNatives::GetBuiltinsCount(); i++) {
On 2015/07/08 08:32:54, Yang wrote:
> Could you refactor this duplicate code into a helper function that takes the
> cache and the builtins count as arguments?

Done.

[danno, 2015-07-08 20:27:22]

Patchset #5 (id:80001) has been deleted

[danno, 2015-07-08 20:27:31]

Patchset #4 (id:60001) has been deleted

[danno, 2015-07-08 20:27:36]

Patchset #3 (id:40001) has been deleted

[danno, 2015-07-08 20:44:06]

Patchset #13 (id:300001) has been deleted

[Yang, 2015-07-10 08:18:57]

serializer part lgtm with comments.

https://codereview.chromium.org/1213203007/diff/280001/src/snapshot/serialize.cc
File src/snapshot/serialize.cc (right):

https://codereview.chromium.org/1213203007/diff/280001/src/snapshot/serialize...
src/snapshot/serialize.cc:1194: case kNativesStringResource: {
no need for the scoping brackets anymore.

https://codereview.chromium.org/1213203007/diff/280001/src/snapshot/serialize...
src/snapshot/serialize.cc:1440: // The bootstrap already snapshot has a
code-stub context, when serializing
weird sentence...

https://codereview.chromium.org/1213203007/diff/280001/src/snapshot/serialize...
src/snapshot/serialize.cc:2192: kNativesStringResource, "NativesStringResource",
Those string arguments are really just for documentation. sink_->Put doesn't
actually use them. (We used to use them for when we add printf debugging to
sink_->Put). So you could just omit them as arguments and use "StringResource"
and "StringResourceEnd" in the method itself.

[danno, 2015-07-10 12:29:28]

On 2015/07/10 at 08:18:57, yangguo wrote:
> serializer part lgtm with comments.
> 
>
https://codereview.chromium.org/1213203007/diff/280001/src/snapshot/serialize.cc
> File src/snapshot/serialize.cc (right):
> 
>
https://codereview.chromium.org/1213203007/diff/280001/src/snapshot/serialize...
> src/snapshot/serialize.cc:1194: case kNativesStringResource: {
> no need for the scoping brackets anymore.
> 
>
https://codereview.chromium.org/1213203007/diff/280001/src/snapshot/serialize...
> src/snapshot/serialize.cc:1440: // The bootstrap already snapshot has a
code-stub context, when serializing
> weird sentence...
> 
>
https://codereview.chromium.org/1213203007/diff/280001/src/snapshot/serialize...
> src/snapshot/serialize.cc:2192: kNativesStringResource,
"NativesStringResource",
> Those string arguments are really just for documentation. sink_->Put doesn't
actually use them. (We used to use them for when we add printf debugging to
sink_->Put). So you could just omit them as arguments and use "StringResource"
and "StringResourceEnd" in the method itself.

Feedback addressed. Any feedback mstarzinger@ or bmeurer@?

[Michael Starzinger, 2015-07-10 13:26:30]

LGTM, mostly nits, didn't look at the serializer.

https://codereview.chromium.org/1213203007/diff/320001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/1213203007/diff/320001/src/code-stubs.cc#newc...
src/code-stubs.cc:471: isolate,
Handle<JSObject>(isolate->heap()->code_stub_exports_object()),
nit: Just use isolate->factory()->code_stub_exports_object() here.

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-api.c...
test/cctest/test-api.cc:11694: CheckSurvivingGlobalObjectsCount(1);
Can we move this off-by-one into the GetGlobalObjectsCount() helper, together
with a comment explaining why we are always seeing one global object too many?

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-debug.cc
File test/cctest/test-debug.cc (right):

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-debug...
test/cctest/test-debug.cc:7344: CHECK_EQ(2, CountNativeContexts());
Same comment as in test-api.cc applies for CountNativeContexts() as well.

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-heap-...
File test/cctest/test-heap-profiler.cc (right):

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-heap-...
test/cctest/test-heap-profiler.cc:99: // The 0th-child is (GC Roots), 1st is the
user root.
nit: Comment is outdated.

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-heap....
test/cctest/test-heap.cc:1562: CHECK_EQ(1, CountNativeContexts());
Same comment as in test-api.cc applies for CountNativeContexts() as well.

[danno, 2015-07-13 09:43:36]

The CQ bit was checked by danno@chromium.org

[danno, 2015-07-13 09:43:37]

Feedback addressed, landing

https://codereview.chromium.org/1213203007/diff/280001/src/snapshot/serialize.cc
File src/snapshot/serialize.cc (right):

https://codereview.chromium.org/1213203007/diff/280001/src/snapshot/serialize...
src/snapshot/serialize.cc:1194: case kNativesStringResource: {
On 2015/07/10 at 08:18:57, Yang wrote:
> no need for the scoping brackets anymore.

Done

https://codereview.chromium.org/1213203007/diff/280001/src/snapshot/serialize...
src/snapshot/serialize.cc:1440: // The bootstrap already snapshot has a
code-stub context, when serializing
On 2015/07/10 at 08:18:57, Yang wrote:
> weird sentence...

Done

https://codereview.chromium.org/1213203007/diff/280001/src/snapshot/serialize...
src/snapshot/serialize.cc:2192: kNativesStringResource, "NativesStringResource",
On 2015/07/10 at 08:18:57, Yang wrote:
> Those string arguments are really just for documentation. sink_->Put doesn't
actually use them. (We used to use them for when we add printf debugging to
sink_->Put). So you could just omit them as arguments and use "StringResource"
and "StringResourceEnd" in the method itself.

Done

https://codereview.chromium.org/1213203007/diff/320001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/1213203007/diff/320001/src/code-stubs.cc#newc...
src/code-stubs.cc:471: isolate,
Handle<JSObject>(isolate->heap()->code_stub_exports_object()),
On 2015/07/10 at 13:26:30, Michael Starzinger wrote:
> nit: Just use isolate->factory()->code_stub_exports_object() here.

Done

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-api.c...
test/cctest/test-api.cc:11694: CheckSurvivingGlobalObjectsCount(1);
On 2015/07/10 at 13:26:30, Michael Starzinger wrote:
> Can we move this off-by-one into the GetGlobalObjectsCount() helper, together
with a comment explaining why we are always seeing one global object too many?

Done

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-debug.cc
File test/cctest/test-debug.cc (right):

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-debug...
test/cctest/test-debug.cc:7344: CHECK_EQ(2, CountNativeContexts());
On 2015/07/10 at 13:26:30, Michael Starzinger wrote:
> Same comment as in test-api.cc applies for CountNativeContexts() as well.

Done.

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-heap-...
File test/cctest/test-heap-profiler.cc (right):

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-heap-...
test/cctest/test-heap-profiler.cc:99: // The 0th-child is (GC Roots), 1st is the
user root.
On 2015/07/10 at 13:26:30, Michael Starzinger wrote:
> nit: Comment is outdated.

Done

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/1213203007/diff/320001/test/cctest/test-heap....
test/cctest/test-heap.cc:1562: CHECK_EQ(1, CountNativeContexts());
On 2015/07/10 at 13:26:30, Michael Starzinger wrote:
> Same comment as in test-api.cc applies for CountNativeContexts() as well.

Done

[danno, 2015-07-13 09:43:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
yangguo@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1213203007/#ps340001
(title: "Review feedback")

[commit-bot: I haz the power, 2015-07-13 09:43:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1213203007/340001

[commit-bot: I haz the power, 2015-07-13 09:45:47]

Committed patchset #14 (id:340001)

[commit-bot: I haz the power, 2015-07-13 09:46:02]

Patchset 14 (id:??) landed as
https://crrev.com/a1475dae5d147cac78812cd4fd09f886b51a88ef
Cr-Commit-Position: refs/heads/master@{#29593}