Introduce faster utilty methods for storing and retrieving native pointers

Introduce faster utilty methods for storing and retrieving native pointers
in internal fields.

[Dean McNamee, 2009-07-16 14:26:09]

No where do you document that the pointer is expected to be two byte aligned. 
This wouldn't work very well for a char* pointer into the middle of a string,
for example.

On 2009/07/16 14:23:08, antonm wrote:
>

[Kasper Lund, 2009-07-16 14:34:11]

I think this needs a bit more work. It feels fishy that it doesn't deal
gracefully with non-aligned pointers and I don't like that this will not work on
64-bit systems because the number semantics of JavaScript will probably force us
to have less than 63-bits for the value part of a smi.

[Kasper Lund, 2009-07-17 08:06:02]

If you get rid of the bit fiddling and start using the IsSmi operations instead,
this LGTM.

http://codereview.chromium.org/155635/diff/8/1010
File include/v8.h (right):

http://codereview.chromium.org/155635/diff/8/1010#newcode1118
Line 1118: void SetPointerInInternalField(int index, void* ptr);
ptr -> value?

http://codereview.chromium.org/155635/diff/8/1011
File src/api.cc (right):

http://codereview.chromium.org/155635/diff/8/1011#newcode2457
Line 2457: if (reinterpret_cast<uintptr_t>(pointer) & 0x1 == 0) {
Why not just say: if (pointer->IsSmi())?

http://codereview.chromium.org/155635/diff/8/1011#newcode2462
Line 2462: // Read from uninitialzied field
There's also a chance of getting a non-proxy object out. You need to deal with
that instead of checking for undefined. I would say if (!pointer->IsProxy())
return NULL;

http://codereview.chromium.org/155635/diff/8/1011#newcode2462
Line 2462: // Read from uninitialzied field
uninitialzied -> uninitialized.

http://codereview.chromium.org/155635/diff/8/1011#newcode2484
Line 2484: void v8::Object::SetPointerInInternalField(int index, void* ptr) {
Rename ptr to value?

http://codereview.chromium.org/155635/diff/8/1011#newcode2486
Line 2486: if (reinterpret_cast<uintptr_t>(ptr) & 0x1 == 0) {
How about if (reinterpret_cast<Object*>(ptr)->IsSmi())?

http://codereview.chromium.org/155635/diff/8/1011#newcode2487
Line 2487: // Aligned pointer, store as is
Terminate comment with .

http://codereview.chromium.org/155635/diff/8/1011#newcode2491
Line 2491: ASSERT(reinterpret_cast<uintptr_t>(proxy) & 0x1 == 1);
ASSERT(!proxy->IsSmi())? Maybe not really needed though.

http://codereview.chromium.org/155635/diff/8/1012
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/155635/diff/8/1012#newcode1297
Line 1297: delete [] data;
delete [] -> delete[]

[Kasper Lund, 2009-07-17 08:14:38]

One extra comment:

http://codereview.chromium.org/155635/diff/8/1011
File src/api.cc (right):

http://codereview.chromium.org/155635/diff/8/1011#newcode2490
Line 2490: i::Proxy* proxy =
*i::Factory::NewProxy(reinterpret_cast<i::Address>(ptr));
Maybe we should always allocate this in new space?

[antonm, 2009-07-17 09:37:56]

Thanks a lot, Kasper!

http://codereview.chromium.org/155635/diff/8/1010
File include/v8.h (right):

http://codereview.chromium.org/155635/diff/8/1010#newcode1118
Line 1118: void SetPointerInInternalField(int index, void* ptr);
On 2009/07/17 08:06:02, Kasper Lund wrote:
> ptr -> value?

Done.

http://codereview.chromium.org/155635/diff/8/1011
File src/api.cc (right):

http://codereview.chromium.org/155635/diff/8/1011#newcode2457
Line 2457: if (reinterpret_cast<uintptr_t>(pointer) & 0x1 == 0) {
On 2009/07/17 08:06:02, Kasper Lund wrote:
> Why not just say: if (pointer->IsSmi())?

Done.

http://codereview.chromium.org/155635/diff/8/1011#newcode2462
Line 2462: // Read from uninitialzied field
On 2009/07/17 08:06:02, Kasper Lund wrote:
> There's also a chance of getting a non-proxy object out. You need to deal with
> that instead of checking for undefined. I would say if (!pointer->IsProxy())
> return NULL;
> 

Good catch.  I've added an ASSERT to check that it should be either undefined or
proxy---setting handles and reading them as pointers might be not the best use
for this API :)

http://codereview.chromium.org/155635/diff/8/1011#newcode2484
Line 2484: void v8::Object::SetPointerInInternalField(int index, void* ptr) {
On 2009/07/17 08:06:02, Kasper Lund wrote:
> Rename ptr to value?

Done.

http://codereview.chromium.org/155635/diff/8/1011#newcode2486
Line 2486: if (reinterpret_cast<uintptr_t>(ptr) & 0x1 == 0) {
On 2009/07/17 08:06:02, Kasper Lund wrote:
> How about if (reinterpret_cast<Object*>(ptr)->IsSmi())?

Done.  I must admit though, that I find explicit alignment checks easier to
understand.

http://codereview.chromium.org/155635/diff/8/1011#newcode2487
Line 2487: // Aligned pointer, store as is
On 2009/07/17 08:06:02, Kasper Lund wrote:
> Terminate comment with .

Done.

http://codereview.chromium.org/155635/diff/8/1011#newcode2490
Line 2490: i::Proxy* proxy =
*i::Factory::NewProxy(reinterpret_cast<i::Address>(ptr));
On 2009/07/17 08:14:38, Kasper Lund wrote:
> Maybe we should always allocate this in new space?

Done.

http://codereview.chromium.org/155635/diff/8/1011#newcode2491
Line 2491: ASSERT(reinterpret_cast<uintptr_t>(proxy) & 0x1 == 1);
On 2009/07/17 08:06:02, Kasper Lund wrote:
> ASSERT(!proxy->IsSmi())? Maybe not really needed though.

Done.

[Kasper Lund, 2009-07-17 10:01:38]

LGTM.

http://codereview.chromium.org/155635/diff/2001/15
File src/api.cc (right):

http://codereview.chromium.org/155635/diff/2001/15#newcode2458
Line 2458: // Fast case, aligned native pointer
Terminate comment with .

http://codereview.chromium.org/155635/diff/2001/15#newcode2464
Line 2464: // Play safe even if it's something unexpected
Terminate comment with .

http://codereview.chromium.org/155635/diff/2001/15#newcode2469
Line 2469: // Unaligned native pointer
Terminate comment with .

http://codereview.chromium.org/155635/diff/2001/15#newcode2490
Line 2490: if (reinterpret_cast<i::Object*>(value)->IsSmi()) {
Introduce a local variable to avoid reinterpret_cast'ing the value to i::Object*
twice.

http://codereview.chromium.org/155635/diff/2001/15#newcode2494
Line 2494: // Currently internal fields used by DOM wrappers which
used -> are used
which got GCed by mark phases -> which only get GCed by the mark-sweep collector

[antonm, 2009-07-17 10:22:43]

Thanks a lot, Kasper!

Submitting.

http://codereview.chromium.org/155635/diff/2001/15
File src/api.cc (right):

http://codereview.chromium.org/155635/diff/2001/15#newcode2458
Line 2458: // Fast case, aligned native pointer
On 2009/07/17 10:01:39, Kasper Lund wrote:
> Terminate comment with .

Done.

http://codereview.chromium.org/155635/diff/2001/15#newcode2464
Line 2464: // Play safe even if it's something unexpected
On 2009/07/17 10:01:39, Kasper Lund wrote:
> Terminate comment with .

Done.

http://codereview.chromium.org/155635/diff/2001/15#newcode2469
Line 2469: // Unaligned native pointer
On 2009/07/17 10:01:39, Kasper Lund wrote:
> Terminate comment with .

Done.

http://codereview.chromium.org/155635/diff/2001/15#newcode2490
Line 2490: if (reinterpret_cast<i::Object*>(value)->IsSmi()) {
On 2009/07/17 10:01:39, Kasper Lund wrote:
> Introduce a local variable to avoid reinterpret_cast'ing the value to
i::Object*
> twice.

Done.

http://codereview.chromium.org/155635/diff/2001/15#newcode2494
Line 2494: // Currently internal fields used by DOM wrappers which
On 2009/07/17 10:01:39, Kasper Lund wrote:
> used -> are used
> which got GCed by mark phases -> which only get GCed by the mark-sweep
collector

Done.