Add support for per-isolate private symbols

Add support for per-isolate private symbols

R=mstarzinger@chromium.org
BUG=
LOG=Y

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

[Michael Starzinger, 2014-03-24 10:04:33]

First round.

https://codereview.chromium.org/196103004/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/196103004/diff/20001/include/v8.h#newcode2001
include/v8.h:2001: // Create a symbol. If data is not NULL, it will be used as a
print name.
nit: Comment is outdated.

https://codereview.chromium.org/196103004/diff/20001/include/v8.h#newcode2024
include/v8.h:2024: // Create a private symbol. If data is not NULL, it will be
the print name.
nit: Likewise.

https://codereview.chromium.org/196103004/diff/20001/include/v8.h#newcode2035
include/v8.h:2035: static Local<Private> Global(Isolate *isolate, Local<String>
name);
Using this name there is the risk that it gets confused with the "global object"
and Context::Global() specifically.

https://codereview.chromium.org/196103004/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/196103004/diff/20001/src/runtime.cc#newcode637
src/runtime.cc:637: isolate->factory()->NewJSObject(isolate->object_function(),
TENURED);
As discussed offline: This will create a JSObject bound to a specific context
(i.e. obj->map->constructor) and hence keep the native context which happened to
trigger this lazy initialization alive forever.

And you are right, we have the same problem already in the lazy initialization
in the symbol.js file.

[rossberg, 2014-03-24 15:09:49]

https://codereview.chromium.org/196103004/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/196103004/diff/20001/include/v8.h#newcode2001
include/v8.h:2001: // Create a symbol. If data is not NULL, it will be used as a
print name.
On 2014/03/24 10:04:33, Michael Starzinger wrote:
> nit: Comment is outdated.

Done.

https://codereview.chromium.org/196103004/diff/20001/include/v8.h#newcode2024
include/v8.h:2024: // Create a private symbol. If data is not NULL, it will be
the print name.
On 2014/03/24 10:04:33, Michael Starzinger wrote:
> nit: Likewise.

Done.

https://codereview.chromium.org/196103004/diff/20001/include/v8.h#newcode2035
include/v8.h:2035: static Local<Private> Global(Isolate *isolate, Local<String>
name);
On 2014/03/24 10:04:33, Michael Starzinger wrote:
> Using this name there is the risk that it gets confused with the "global
object"
> and Context::Global() specifically.

Renamed to ForApi. Also added Symbol::For and Symbol::ForApi.

https://codereview.chromium.org/196103004/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/196103004/diff/20001/src/runtime.cc#newcode637
src/runtime.cc:637: isolate->factory()->NewJSObject(isolate->object_function(),
TENURED);
On 2014/03/24 10:04:33, Michael Starzinger wrote:
> As discussed offline: This will create a JSObject bound to a specific context
> (i.e. obj->map->constructor) and hence keep the native context which happened
to
> trigger this lazy initialization alive forever.
> 
> And you are right, we have the same problem already in the lazy initialization
> in the symbol.js file.

I moved the whole registry creation and initialization to a new method
Isolate::GetSymbolRegistry, which is now shared by all relevant code sites.

[Michael Starzinger, 2014-03-24 15:17:18]

LGTM, just a couple of nits to address.

https://codereview.chromium.org/196103004/diff/40001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/196103004/diff/40001/src/api.cc#newcode6154
src/api.cc:6154: i::Handle<i::String> part =
i::handle(i_isolate->heap()->for_string());
nit: Just use i_isolate->factory()->for_string() instead.

https://codereview.chromium.org/196103004/diff/40001/src/api.cc#newcode6172
src/api.cc:6172: i::Handle<i::String> part =
i::handle(i_isolate->heap()->for_api_string());
nit: Just use i_isolate->factory()->for_api_string() instead.

https://codereview.chromium.org/196103004/diff/40001/src/api.cc#newcode6203
src/api.cc:6203: i::handle(i_isolate->heap()->private_api_string());
nit: Just use i_isolate->factory()->private_api_string() instead.

https://codereview.chromium.org/196103004/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/196103004/diff/40001/src/isolate.cc#newcode2348
src/isolate.cc:2348: return handle(JSObject::cast(heap()->symbol_registry()));
nit: Better use "return Handle<JSObject>::cast(factory()->symbol_registry())".

https://codereview.chromium.org/196103004/diff/40001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/196103004/diff/40001/src/isolate.h#newcode1129
src/isolate.h:1129: Handle<JSObject> GetSymbolRegistry();
nit: Can we add a short one-line comment of the intended use?

https://codereview.chromium.org/196103004/diff/40001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/196103004/diff/40001/src/runtime.cc#newcode629
src/runtime.cc:629: Handle<String> part =
handle(isolate->heap()->private_intern_string());
nit: You can just use isolate->factory()->private_intern_string() here.

https://codereview.chromium.org/196103004/diff/40001/src/runtime.cc#newcode659
src/runtime.cc:659: HandleScope hs(isolate);
nit: s/hs/scope/ for consistency and grep-ability.

https://codereview.chromium.org/196103004/diff/40001/src/symbol.js
File src/symbol.js (right):

https://codereview.chromium.org/196103004/diff/40001/src/symbol.js#newcode67
src/symbol.js:67: var registry = %SymbolRegistry().for_intern;
nit: The usage of the local variable "registry" is different here and below,
this is a little bit confusing IMHO.

[rossberg, 2014-03-24 15:54:46]

https://codereview.chromium.org/196103004/diff/40001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/196103004/diff/40001/src/api.cc#newcode6154
src/api.cc:6154: i::Handle<i::String> part =
i::handle(i_isolate->heap()->for_string());
On 2014/03/24 15:17:19, Michael Starzinger wrote:
> nit: Just use i_isolate->factory()->for_string() instead.

Done.

https://codereview.chromium.org/196103004/diff/40001/src/api.cc#newcode6172
src/api.cc:6172: i::Handle<i::String> part =
i::handle(i_isolate->heap()->for_api_string());
On 2014/03/24 15:17:19, Michael Starzinger wrote:
> nit: Just use i_isolate->factory()->for_api_string() instead.

Done.

https://codereview.chromium.org/196103004/diff/40001/src/api.cc#newcode6203
src/api.cc:6203: i::handle(i_isolate->heap()->private_api_string());
On 2014/03/24 15:17:19, Michael Starzinger wrote:
> nit: Just use i_isolate->factory()->private_api_string() instead.

Done.

https://codereview.chromium.org/196103004/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/196103004/diff/40001/src/isolate.cc#newcode2348
src/isolate.cc:2348: return handle(JSObject::cast(heap()->symbol_registry()));
On 2014/03/24 15:17:19, Michael Starzinger wrote:
> nit: Better use "return Handle<JSObject>::cast(factory()->symbol_registry())".

Done.

https://codereview.chromium.org/196103004/diff/40001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/196103004/diff/40001/src/isolate.h#newcode1129
src/isolate.h:1129: Handle<JSObject> GetSymbolRegistry();
On 2014/03/24 15:17:19, Michael Starzinger wrote:
> nit: Can we add a short one-line comment of the intended use?

Done.

https://codereview.chromium.org/196103004/diff/40001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/196103004/diff/40001/src/runtime.cc#newcode629
src/runtime.cc:629: Handle<String> part =
handle(isolate->heap()->private_intern_string());
On 2014/03/24 15:17:19, Michael Starzinger wrote:
> nit: You can just use isolate->factory()->private_intern_string() here.

Done.

https://codereview.chromium.org/196103004/diff/40001/src/runtime.cc#newcode659
src/runtime.cc:659: HandleScope hs(isolate);
On 2014/03/24 15:17:19, Michael Starzinger wrote:
> nit: s/hs/scope/ for consistency and grep-ability.

Done.

https://codereview.chromium.org/196103004/diff/40001/src/symbol.js
File src/symbol.js (right):

https://codereview.chromium.org/196103004/diff/40001/src/symbol.js#newcode67
src/symbol.js:67: var registry = %SymbolRegistry().for_intern;
On 2014/03/24 15:17:19, Michael Starzinger wrote:
> nit: The usage of the local variable "registry" is different here and below,
> this is a little bit confusing IMHO.

Done.

https://codereview.chromium.org/196103004/diff/60001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/196103004/diff/60001/src/isolate.cc#newcode2345
src/isolate.cc:2345: JSObject::NormalizeProperties(obj,
KEEP_INOBJECT_PROPERTIES, 8);
Now forcing into dictionary mode.

[rossberg, 2014-03-24 16:34:30]

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