Heavy cleanup of the external pointer API. (issue 11190050)

[sven...@chromium.org]

Reviewers: Michael Starzinger,

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

Description:
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.

Please review this at https://codereview.chromium.org/11190050/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M include/v8.h
M src/api.cc
M src/bootstrapper.cc
M src/compiler.cc
M src/contexts.h
M src/factory.h
M src/factory.cc
M src/heap.h
M src/heap.cc
M src/objects-inl.h
M src/objects.h
M src/profile-generator.cc
M test/cctest/test-api.cc
M test/cctest/test-debug.cc
M test/cctest/test-decls.cc
M tools/grokdump.py

[aba...@chromium.org]

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.

https://codereview.chromium.org/11190050/

[sven...@chromium.org]

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...

https://codereview.chromium.org/11190050/

[aba...@chromium.org]

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

https://codereview.chromium.org/11190050/

[sven...@chromium.org]

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.

https://codereview.chromium.org/11190050/

[sven...@chromium.org]

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

https://codereview.chromium.org/11190050/

[mstar...@chromium.org]

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#newcode1244
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#newcode2060
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!

https://codereview.chromium.org/11190050/

[sven...@chromium.org]

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#newcode1244
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#newcode2060
test/cctest/test-api.cc:2060:

On 2012/10/25 09:41:53, Michael Starzinger wrote:
> Add second empty newline for readability.

Done.

https://codereview.chromium.org/11190050/