Externalization API for ArrayBuffer

Externalization API for ArrayBuffer

R=svenpanne@chromium.org

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

[Dmitry Lomov (no reviews), 2013-05-22 17:08:07]

Preparing new ArrayBuffer for use in Blink
- add new externalization API for ArrayBuffer. Externalization transfers the
ownership of the backing store to v8::ArrayBufferContents.
- allocate some internal fields to ArrayBuffer. The number of external fields is
controlled by a #define. No gyp plumbing for the define yet.

[Sven Panne, 2013-05-23 07:57:01]

https://chromiumcodereview.appspot.com/15001041/diff/7001/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/15001041/diff/7001/include/v8.h#newcod...
include/v8.h:2342: ArrayBufferContents() : data_(NULL), byte_length_(0) {}
Nit: We usually put the constructor/destructor at the beginning of the
corresponding public/protected/private section.

https://chromiumcodereview.appspot.com/15001041/diff/7001/include/v8.h#newcod...
include/v8.h:2348: { }
Nit: Put this at the end of the initializer list.

https://chromiumcodereview.appspot.com/15001041/diff/7001/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/15001041/diff/7001/src/api.cc#newcode6023
src/api.cc:6023: i::Isolate* isolate = Utils::OpenHandle(this)->GetIsolate();
Just remove this line and the following IsDeadCheck. This idiom is fundamentally
broken, anyway, and we want to remove it completely in the future, so no need
for it in new code.

https://chromiumcodereview.appspot.com/15001041/diff/7001/src/api.cc#newcode6025
src/api.cc:6025: i::Handle<i::JSArrayBuffer> obj = Utils::OpenHandle(this);
Nit: No need to name this temporary.

https://chromiumcodereview.appspot.com/15001041/diff/7001/src/api.cc#newcode6030
src/api.cc:6030: if (data_ != NULL) {
No need for this guard, free(NULL) is by definition a no-op.

https://chromiumcodereview.appspot.com/15001041/diff/7001/src/api.cc#newcode6039
src/api.cc:6039: i::Isolate* isolate = Utils::OpenHandle(this)->GetIsolate();
Kill the IsDeadCheck here, too.

https://chromiumcodereview.appspot.com/15001041/diff/7001/src/api.cc#newcode6047
src/api.cc:6047: contents->data_ = obj->backing_store();
We silently overwrite any previous backing store in contents here. This part of
the API looks very fragile and will easily lead to programming errors. Can we do
this differently?

https://chromiumcodereview.appspot.com/15001041/diff/7001/src/d8.cc
File src/d8.cc (right):

https://chromiumcodereview.appspot.com/15001041/diff/7001/src/d8.cc#newcode1080
src/d8.cc:1080: weak_handle.MarkIndependent(isolate);
IIRC, dcarney wants to move to a non-Isolate version in the long run, so we
should remove parameter here.

https://chromiumcodereview.appspot.com/15001041/diff/7001/src/runtime.cc
File src/runtime.cc (right):

https://chromiumcodereview.appspot.com/15001041/diff/7001/src/runtime.cc#newc...
src/runtime.cc:668: if (data != NULL)
No need for this guard here, either.

https://chromiumcodereview.appspot.com/15001041/diff/7001/test/cctest/test-ap...
File test/cctest/test-api.cc (right):

https://chromiumcodereview.appspot.com/15001041/diff/7001/test/cctest/test-ap...
test/cctest/test-api.cc:2496: THREADED_TEST(ArrayBuffer) {
Can we please split this test into several tests? Currently it is very hard to
see what the various parts are actually testing. Furthermore, do we have simple
test to see what happens on the JavaScript side when an ArrayBuffer moves to the
"externalized" state? If there is one, I can't see it...

https://chromiumcodereview.appspot.com/15001041/diff/7001/test/cctest/test-ap...
test/cctest/test-api.cc:15477: ElementType* backing_store = new
ElementType[kElementCount+2];
Using 'ScopedVector<ElementType> backing_store(kElementCount+2)' simplifies
things here.

[Dmitry Lomov (no reviews), 2013-05-23 08:42:15]

Thanks for review, all comments addressed

https://codereview.chromium.org/15001041/diff/7001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/15001041/diff/7001/include/v8.h#newcode2342
include/v8.h:2342: ArrayBufferContents() : data_(NULL), byte_length_(0) {}
On 2013/05/23 07:57:01, Sven Panne wrote:
> Nit: We usually put the constructor/destructor at the beginning of the
> corresponding public/protected/private section.

Done.

https://codereview.chromium.org/15001041/diff/7001/include/v8.h#newcode2348
include/v8.h:2348: { }
On 2013/05/23 07:57:01, Sven Panne wrote:
> Nit: Put this at the end of the initializer list.

Done.

https://codereview.chromium.org/15001041/diff/7001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/15001041/diff/7001/src/api.cc#newcode6023
src/api.cc:6023: i::Isolate* isolate = Utils::OpenHandle(this)->GetIsolate();
On 2013/05/23 07:57:01, Sven Panne wrote:
> Just remove this line and the following IsDeadCheck. This idiom is
fundamentally
> broken, anyway, and we want to remove it completely in the future, so no need
> for it in new code.

Done.

https://codereview.chromium.org/15001041/diff/7001/src/api.cc#newcode6025
src/api.cc:6025: i::Handle<i::JSArrayBuffer> obj = Utils::OpenHandle(this);
On 2013/05/23 07:57:01, Sven Panne wrote:
> Nit: No need to name this temporary.

Done.

https://codereview.chromium.org/15001041/diff/7001/src/api.cc#newcode6030
src/api.cc:6030: if (data_ != NULL) {
On 2013/05/23 07:57:01, Sven Panne wrote:
> No need for this guard, free(NULL) is by definition a no-op.

Done.

https://codereview.chromium.org/15001041/diff/7001/src/api.cc#newcode6039
src/api.cc:6039: i::Isolate* isolate = Utils::OpenHandle(this)->GetIsolate();
On 2013/05/23 07:57:01, Sven Panne wrote:
> Kill the IsDeadCheck here, too.

Done.

https://codereview.chromium.org/15001041/diff/7001/src/api.cc#newcode6047
src/api.cc:6047: contents->data_ = obj->backing_store();
As discussed offline, added an ApiCheck
On 2013/05/23 07:57:01, Sven Panne wrote:
> We silently overwrite any previous backing store in contents here. This part
of
> the API looks very fragile and will easily lead to programming errors. Can we
do
> this differently?

https://codereview.chromium.org/15001041/diff/7001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/15001041/diff/7001/src/d8.cc#newcode1080
src/d8.cc:1080: weak_handle.MarkIndependent(isolate);
On 2013/05/23 07:57:01, Sven Panne wrote:
> IIRC, dcarney wants to move to a non-Isolate version in the long run, so we
> should remove parameter here.

Done.

https://codereview.chromium.org/15001041/diff/7001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/15001041/diff/7001/src/runtime.cc#newcode668
src/runtime.cc:668: if (data != NULL)
On 2013/05/23 07:57:01, Sven Panne wrote:
> No need for this guard here, either.

Done.

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

https://codereview.chromium.org/15001041/diff/7001/test/cctest/test-api.cc#ne...
test/cctest/test-api.cc:2496: THREADED_TEST(ArrayBuffer) {
On 2013/05/23 07:57:01, Sven Panne wrote:
> Can we please split this test into several tests? Currently it is very hard to
> see what the various parts are actually testing. Furthermore, do we have
simple
> test to see what happens on the JavaScript side when an ArrayBuffer moves to
the
> "externalized" state? If there is one, I can't see it...

Done.

https://codereview.chromium.org/15001041/diff/7001/test/cctest/test-api.cc#ne...
test/cctest/test-api.cc:15477: ElementType* backing_store = new
ElementType[kElementCount+2];
On 2013/05/23 07:57:01, Sven Panne wrote:
> Using 'ScopedVector<ElementType> backing_store(kElementCount+2)' simplifies
> things here.

Done.

[Sven Panne, 2013-05-23 09:08:41]

LGTM with a nit

https://chromiumcodereview.appspot.com/15001041/diff/17001/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/15001041/diff/17001/include/v8.h#newco...
include/v8.h:2346: ArrayBufferContents(void* data, size_t byte_length)
No callers, remove it.

[Dmitry Lomov (no reviews), 2013-05-23 10:01:53]

Committed patchset #5 manually as r14770 (presubmit successful).