Account Options

  1. Sign in
The old Google Groups will be going away soon, but your browser is incompatible with the new version.
Google Groups Home
« Groups Home
v8-dev
Home
+ new post
About this group
Join this group
Heavy cleanup of the external pointer API. (issue 11190050)
Options
There are currently too many topics in this group that display first. To make this topic appear first, remove this option from another topic.
There was an error processing your request. Please try again.
flag
   8 messages - Collapse all  -  Translate all to Translated (View all originals)
The group you are posting to is a Usenet group. Messages posted to this group will make your email address visible to anyone on the Internet.
Your reply message has not been sent.
Your post was successful
 
From:
To:
Cc:
Followup To:
Add Cc | Add Followup-to | Edit Subject
Subject:
Validation:
For verification purposes please type the characters you see in the picture below or the numbers you hear by clicking the accessibility icon. Listen and type the numbers you hear
 
svenpa...@chromium.org  
View profile  
 More options Oct 23 2012, 10:01 am
From: svenpa...@chromium.org
Date: Tue, 23 Oct 2012 14:01:13 +0000
Local: Tues, Oct 23 2012 10:01 am
Subject: Heavy cleanup of the external pointer API. (issue 11190050)
Print | Individual message | Show original | Report this message | Find messages by this author
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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
aba...@chromium.org  
View profile  
 More options Oct 23 2012, 11:16 am
From: aba...@chromium.org
Date: Tue, 23 Oct 2012 15:16:30 +0000
Local: Tues, Oct 23 2012 11:16 am
Subject: Re: Heavy cleanup of the external pointer API. (issue 11190050)
Print | Individual message | Show original | Report this message | Find messages by this author
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#newco...
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#newco...
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/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
svenpa...@chromium.org  
View profile  
 More options Oct 24 2012, 2:31 am
From: svenpa...@chromium.org
Date: Wed, 24 Oct 2012 06:31:35 +0000
Local: Wed, Oct 24 2012 2:31 am
Subject: Re: Heavy cleanup of the external pointer API. (issue 11190050)
Print | Individual message | Show original | Report this message | Find messages by this author

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#newco...
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#newco...
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/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
aba...@chromium.org  
View profile  
 More options Oct 24 2012, 2:45 am
From: aba...@chromium.org
Date: Wed, 24 Oct 2012 06:45:01 +0000
Local: Wed, Oct 24 2012 2:45 am
Subject: Re: Heavy cleanup of the external pointer API. (issue 11190050)
Print | Individual message | Show original | Report this message | Find messages by this author
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/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
svenpa...@chromium.org  
View profile  
 More options Oct 24 2012, 3:37 am
From: svenpa...@chromium.org
Date: Wed, 24 Oct 2012 07:36:58 +0000
Local: Wed, Oct 24 2012 3:36 am
Subject: Re: Heavy cleanup of the external pointer API. (issue 11190050)
Print | Individual message | Show original | Report this message | Find messages by this author

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#newco...
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/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
svenpa...@chromium.org  
View profile  
 More options Oct 24 2012, 8:12 am
From: svenpa...@chromium.org
Date: Wed, 24 Oct 2012 12:12:33 +0000
Local: Wed, Oct 24 2012 8:12 am
Subject: Re: Heavy cleanup of the external pointer API. (issue 11190050)
Print | Individual message | Show original | Report this message | Find messages by this author
Added missing unit tests, so this CL should be reviewable in its full  
glory...
:-)

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
mstarzin...@chromium.org  
View profile  
 More options Oct 25 2012, 5:41 am
From: mstarzin...@chromium.org
Date: Thu, 25 Oct 2012 09:41:53 +0000
Local: Thurs, Oct 25 2012 5:41 am
Subject: Re: Heavy cleanup of the external pointer API. (issue 11190050)
Print | Individual message | Show original | Report this message | Find messages by this author
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#newc...
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#newcod...
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#newcod...
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#newcod...
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#newcod...
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....
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#n...
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#new...
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-...
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/11190050/diff/14009/test/cctest/test-...
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...
tools/grokdump.py:866: 0x08081: (128, "MetaMap"),
Wow! Thanks for updating Grokdump as well. Kudos!

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
svenpa...@chromium.org  
View profile  
 More options Oct 25 2012, 10:23 am
From: svenpa...@chromium.org
Date: Thu, 25 Oct 2012 14:23:08 +0000
Local: Thurs, Oct 25 2012 10:23 am
Subject: Re: Heavy cleanup of the external pointer API. (issue 11190050)
Print | Individual message | Show original | Report this message | Find messages by this author
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#newc...
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#newcod...
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#newcod...
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#newcod...
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#newcod...
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....
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#n...
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#new...
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-...
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/11190050/diff/14009/test/cctest/test-...
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/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
End of messages
« Back to Discussions « Newer topic     Older topic »

Create a group - Google Groups - Google Home - Terms of Service - Privacy Policy
©2013 Google