Heavy cleanup of the external pointer API.

Heavy cleanup of the external pointer API.

Added highly efficient Object::SetAlignedPointerInInternalField and
Object::GetAlignedPointerFromInternalField functions for 2-byte-aligned
pointers. Their non-aligned counterparts Object::GetPointerFromInternalField and
Object::SetPointerInInternalField are now deprecated utility functions.

External is now a true Value again, with New/Value/Cast using a JSObject with an
internal field containing a Foreign. External::Wrap, and External::Unwrap are now
deprecated utility functions.

Added Context::GetEmbedderData and Context::SetEmbedderData. Deprecated
Context::GetData and Context::SetData, these are now only wrappers to access
internal field 0.

Added highly efficient Context::SetAlignedPointerInEmbedderData and
Context::GetAlignedPointerFromEmbedderData functions for 2-byte-aligned
pointers.

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

[Sven Panne, 2012-10-23 14:01:13]

Unit tests for the Context API part are still missing, but the rest is ready for
a first round of comments...

[abarth-chromium, 2012-10-23 15:16:30]

I mostly looked at the API side of this CL because that's the part I understand
best.  The API looks like what we need.  It looks like we need to use the
aligned pointer APIs in order for things to be fast, which should be fine.

https://codereview.chromium.org/11190050/diff/2034/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/11190050/diff/2034/include/v8.h#newcode1987
include/v8.h:1987: V8_DEPRECATED(static inline void* Unwrap(Handle<Value> obj));
It looks like Unwrap used to be inline, whereas Value is out-of-line.  I guess
that means we should be using the pointer-based Get/Set APIs when performance is
critical rather than using External?

https://codereview.chromium.org/11190050/diff/2034/include/v8.h#newcode4729
include/v8.h:4729: O** result =
HandleScope::CreateHandle(I::ReadEmbedderData<O*>(this, index));
The old implementation of GetData used to call Utils::ToLocal, which I remember
as just being a pointer move, where as now we're calling
HandleScope::CreateHandle, which I believe is much slower.  I guess that means
we need to use GetAlignedPointerFromEmbedderData for anything performance
sensitive.

[Sven Panne, 2012-10-24 06:31:35]

https://codereview.chromium.org/11190050/diff/2034/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/11190050/diff/2034/include/v8.h#newcode1987
include/v8.h:1987: V8_DEPRECATED(static inline void* Unwrap(Handle<Value> obj));
On 2012/10/23 15:16:30, abarth wrote:
> It looks like Unwrap used to be inline, whereas Value is out-of-line.  I guess
> that means we should be using the pointer-based Get/Set APIs when performance
is
> critical rather than using External?

Yes, and I've even explicitly marked Wrap/Unwrap as deprecated, because they
made "class External: public Value" a lie. Externals were not guaranteed to be
valid JavaScript objects, and I've seen them escaping into the wild, leading to
wrong bug reports for v8.

As you already found out, the new story for fast access is: Ensure that your
pointers are aligned and use {Get,Set}AlignedFoo. If you really need to store
general void* values, use External for (un-)wrapping these into true JS objects
plus {Get,Set}{InternalField,EmbedderData}Foo.

https://codereview.chromium.org/11190050/diff/2034/include/v8.h#newcode4729
include/v8.h:4729: O** result =
HandleScope::CreateHandle(I::ReadEmbedderData<O*>(this, index));
On 2012/10/23 15:16:30, abarth wrote:
> The old implementation of GetData used to call Utils::ToLocal, which I
remember
> as just being a pointer move, where as now we're calling
> HandleScope::CreateHandle, which I believe is much slower.  I guess that means
> we need to use GetAlignedPointerFromEmbedderData for anything performance
> sensitive.

Although you are on a slow path already when you are in api.cc, I'll take a
look...

[abarth-chromium, 2012-10-24 06:45:01]

That all makes sense.  I'll be sure to move WebKit over to the aligned pointer
APIs once this change rolls into Chromium.

[Sven Panne, 2012-10-24 07:36:58]

https://codereview.chromium.org/11190050/diff/2034/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/11190050/diff/2034/include/v8.h#newcode4729
include/v8.h:4729: O** result =
HandleScope::CreateHandle(I::ReadEmbedderData<O*>(this, index));
On 2012/10/23 15:16:30, abarth wrote:
> The old implementation of GetData used to call Utils::ToLocal, which I
remember
> as just being a pointer move, where as now we're calling
> HandleScope::CreateHandle, which I believe is much slower.  I guess that means
> we need to use GetAlignedPointerFromEmbedderData for anything performance
> sensitive.

Actually, GetData implicitly did a HandleScope:Create, because it explicitly
constructed a Handle and then converted it via ToLocal. What you see here in
GetEmbedderData is basically an inlined combined version of Handle()+ToLocal(),
so we are even slightly more efficient than before.

[Sven Panne, 2012-10-24 12:12:33]

Added missing unit tests, so this CL should be reviewable in its full glory...
:-)

[Michael Starzinger, 2012-10-25 09:41:53]

LGTM (if comments are addressed).

https://codereview.chromium.org/11190050/diff/14009/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/11190050/diff/14009/include/v8.h#newcode1632
include/v8.h:1632: * a field, you must use GetAlignedPointerFromInternalField,
everything else
Can we rephrase the comment so that it doesn't use personal pronouns. E.g. "Such
a field must be retrieved via GetAlignedPointerFromInternalField, everything
else [...]"

https://codereview.chromium.org/11190050/diff/14009/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/11190050/diff/14009/src/api.cc#newcode827
src/api.cc:827: data->set(index, *Utils::OpenHandle(*value));
Even though this pattern is safe, GCMole might report it as a false positive.

https://codereview.chromium.org/11190050/diff/14009/src/api.cc#newcode4239
src/api.cc:4239: static bool ObjectFieldOK(i::Handle<i::JSObject> obj,
Can we call this InternalFieldOK?

https://codereview.chromium.org/11190050/diff/14009/src/api.cc#newcode4262
src/api.cc:4262: obj->SetInternalField(index, *Utils::OpenHandle(*value));
Even though this pattern is safe, GCMole might report it as a false positive.

https://codereview.chromium.org/11190050/diff/14009/src/api.cc#newcode4742
src/api.cc:4742: // TODO(svenpanne) This should somehow be Utils::ToLocal.
You can get a ToLocal by calling it ExternalToLocal and adding the following
line to api.h, I would prefer that:

MAKE_TO_LOCAL(ExternalToLocal, JSObject, External)

https://codereview.chromium.org/11190050/diff/14009/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/11190050/diff/14009/src/bootstrapper.cc#newco...
src/bootstrapper.cc:1244:
native_context()->set_embedder_data(*factory->NewFixedArray(2));
This pattern is not GC-safe and GCMole will (hopefully) complain about it. First
allocate the array, and then take the native_context dereference.

Handle<FixedArray> embedder_data = factory->NewFixedArray(2);
native_context()->set_embedder_data(embedder_data);

https://codereview.chromium.org/11190050/diff/14009/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/11190050/diff/14009/src/compiler.cc#newcode436
src/compiler.cc:436: FixedArray* array =
(*isolate->native_context())->embedder_data();
This can just be "isolate->native_context()->embedder_data()" without the
explicit dereference.

https://codereview.chromium.org/11190050/diff/14009/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11190050/diff/14009/src/objects.h#newcode2414
src/objects.h:2414: STATIC_CHECK(kHeaderSize ==
Internals::kFixedArrayHeaderSize);
Can we move this to FixedArrayBase, right after the constant is actually
defined? Closer is better.

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

https://codereview.chromium.org/11190050/diff/14009/test/cctest/test-api.cc#n...
test/cctest/test-api.cc:2060: 
Add second empty newline for readability.

https://codereview.chromium.org/11190050/diff/14009/tools/grokdump.py
File tools/grokdump.py (right):

https://codereview.chromium.org/11190050/diff/14009/tools/grokdump.py#newcode866
tools/grokdump.py:866: 0x08081: (128, "MetaMap"),
Wow! Thanks for updating Grokdump as well. Kudos!

[Sven Panne, 2012-10-25 14:23:08]

Addressed feedback, waiting to land this until our bleeding_edge is open for
non-bug-fixes again...

https://codereview.chromium.org/11190050/diff/14009/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/11190050/diff/14009/include/v8.h#newcode1632
include/v8.h:1632: * a field, you must use GetAlignedPointerFromInternalField,
everything else
On 2012/10/25 09:41:53, Michael Starzinger wrote:
> Can we rephrase the comment so that it doesn't use personal pronouns. E.g.
"Such
> a field must be retrieved via GetAlignedPointerFromInternalField, everything
> else [...]"

Done here and at similar places.

https://codereview.chromium.org/11190050/diff/14009/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/11190050/diff/14009/src/api.cc#newcode827
src/api.cc:827: data->set(index, *Utils::OpenHandle(*value));
On 2012/10/25 09:41:53, Michael Starzinger wrote:
> Even though this pattern is safe, GCMole might report it as a false positive.

Done.

https://codereview.chromium.org/11190050/diff/14009/src/api.cc#newcode4239
src/api.cc:4239: static bool ObjectFieldOK(i::Handle<i::JSObject> obj,
On 2012/10/25 09:41:53, Michael Starzinger wrote:
> Can we call this InternalFieldOK?

Done.

https://codereview.chromium.org/11190050/diff/14009/src/api.cc#newcode4262
src/api.cc:4262: obj->SetInternalField(index, *Utils::OpenHandle(*value));
On 2012/10/25 09:41:53, Michael Starzinger wrote:
> Even though this pattern is safe, GCMole might report it as a false positive.

Done.

https://codereview.chromium.org/11190050/diff/14009/src/api.cc#newcode4742
src/api.cc:4742: // TODO(svenpanne) This should somehow be Utils::ToLocal.
On 2012/10/25 09:41:53, Michael Starzinger wrote:
> You can get a ToLocal by calling it ExternalToLocal and adding the following
> line to api.h, I would prefer that:
> 
> MAKE_TO_LOCAL(ExternalToLocal, JSObject, External)

Ooops, forgot that TODO. Good point, one can even remove the old incorrect
conversion From Foreign to External.

https://codereview.chromium.org/11190050/diff/14009/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/11190050/diff/14009/src/bootstrapper.cc#newco...
src/bootstrapper.cc:1244:
native_context()->set_embedder_data(*factory->NewFixedArray(2));
On 2012/10/25 09:41:53, Michael Starzinger wrote:
> This pattern is not GC-safe and GCMole will (hopefully) complain about it.
First
> allocate the array, and then take the native_context dereference.
> 
> Handle<FixedArray> embedder_data = factory->NewFixedArray(2);
> native_context()->set_embedder_data(embedder_data);

Done.

https://codereview.chromium.org/11190050/diff/14009/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/11190050/diff/14009/src/compiler.cc#newcode436
src/compiler.cc:436: FixedArray* array =
(*isolate->native_context())->embedder_data();
On 2012/10/25 09:41:53, Michael Starzinger wrote:
> This can just be "isolate->native_context()->embedder_data()" without the
> explicit dereference.

Done. Excessive overloading FTW! :-P

https://codereview.chromium.org/11190050/diff/14009/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11190050/diff/14009/src/objects.h#newcode2414
src/objects.h:2414: STATIC_CHECK(kHeaderSize ==
Internals::kFixedArrayHeaderSize);
On 2012/10/25 09:41:53, Michael Starzinger wrote:
> Can we move this to FixedArrayBase, right after the constant is actually
> defined? Closer is better.

I put the check here intentionally, and I think it is actually a better place
than the one in FixedArrayBase: The user of kFixedArrayHeaderSize only cares
about the fact that the object in question is a FixedArray and not about the
implementation hierarchy. I really want to assert that FixedArray::kHeaderSize
is equal to kFixedArrayHeaderSize, so it belongs here.

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

https://codereview.chromium.org/11190050/diff/14009/test/cctest/test-api.cc#n...
test/cctest/test-api.cc:2060: 
On 2012/10/25 09:41:53, Michael Starzinger wrote:
> Add second empty newline for readability.

Done.