- A prototype which allows to expose CanvasPixelArray functionality...

- Expose CanvasPixelArray functionality directly in JavaScript
  indexed property accesses.
- The IC stubs have not been updated to handle these directly, but
  at least we do not have to leave the VM to access bytes.


Committed: http://code.google.com/p/v8/source/detail?r=2549

[iposva, 2009-07-23 08:56:51]

This is the current state of the CanvasPixelArray prototype. It is now wired all
the way up into the V8 embedding API, although I am not sure I like the name
just yet.

Early feedback is appreciated.

Thanks,
-Ivan

[Mads Ager (chromium), 2009-07-23 14:21:45]

This looks like the right approach to me.  I'm unsure about the API side of the
change, but I fail to come up with something better. :)

http://codereview.chromium.org/159263/diff/1/7
File src/handles.cc (right):

http://codereview.chromium.org/159263/diff/1/7#newcode348
Line 348: if (has_exception) return Handle<Object>(Failure::Exception());
We never return failures in handles because we should be able to assume that if
we have a handle there is a valid thing in it.  This should hit an assertion.

The handle-based operations are guarded (or should be) at the call site.  If an
empty handle is returned from one of them, it is because of an exception.

http://codereview.chromium.org/159263/diff/1/8
File src/objects.cc (right):

http://codereview.chromium.org/159263/diff/1/8#newcode2419
Line 2419: // TODO(iposva): Cleanup.
What is the right action for pixel arrays here anyway?  Ignoring the request to
delete obviously, but do we return 'true' or 'false'?

http://codereview.chromium.org/159263/diff/1/8#newcode2525
Line 2525: ? Smi::cast(JSArray::cast(this)->length())->value()
Is this what the Google code style says?  I actually thought it was correctly
4-space indented before.

[iposva, 2009-07-27 07:01:53]

http://codereview.chromium.org/159263/diff/1/7
File src/handles.cc (right):

http://codereview.chromium.org/159263/diff/1/7#newcode348
Line 348: if (has_exception) return Handle<Object>(Failure::Exception());
On 2009/07/23 14:21:45, Mads Ager wrote:
> We never return failures in handles because we should be able to assume that
if
> we have a handle there is a valid thing in it.  This should hit an assertion.
> 
> The handle-based operations are guarded (or should be) at the call site.  If
an
> empty handle is returned from one of them, it is because of an exception.

Done.

http://codereview.chromium.org/159263/diff/1/8
File src/objects.cc (right):

http://codereview.chromium.org/159263/diff/1/8#newcode2525
Line 2525: ? Smi::cast(JSArray::cast(this)->length())->value()
On 2009/07/23 14:21:45, Mads Ager wrote:
> Is this what the Google code style says?  I actually thought it was correctly
> 4-space indented before.

Done.

[Kasper Lund, 2009-07-27 07:43:40]

It might be nice to have a runtime call for attaching a PixelArray to an object
from JS code to facilitate easier testing. First round of comments:

http://codereview.chromium.org/159263/diff/1012/1015
File include/v8.h (right):

http://codereview.chromium.org/159263/diff/1012/1015#newcode1172
Line 1172: void SetElementsToPixelData(uint8_t* data, int length);
I think this deserves a comment. Do we use the concept of 'Elements' in the API
elsewhere? Maybe it could be something like AssociateWithPixelData instead? Who
owns the data array? There seems to be no weak callbacks, so is that entirely up
to the embedder to deal with?

http://codereview.chromium.org/159263/diff/1012/1022
File src/api.h (right):

http://codereview.chromium.org/159263/diff/1012/1022#newcode102
Line 102: ASSERT(value()->GetElementsKind() ==
Why not keep the HasFastElements checker?

http://codereview.chromium.org/159263/diff/1012/1018
File src/handles.cc (right):

http://codereview.chromium.org/159263/diff/1012/1018#newcode349
Line 349: *value.location() = *number;  // Overwrite the value being passed in.
This seems fishy to me. Why not just do 'value = number'?

http://codereview.chromium.org/159263/diff/1012/1017
File src/heap.cc (right):

http://codereview.chromium.org/159263/diff/1012/1017#newcode1874
Line 1874: size > MaxObjectSizeInPagedSpace() ? LO_SPACE : OLD_DATA_SPACE;
Do we ever get pixel arrays in LO_SPACE? Aren't they super small? Maybe change
to an assert?

http://codereview.chromium.org/159263/diff/1012/1017#newcode1889
Line 1889: AllocationSpace space =
Do we ever get pixel arrays in LO_SPACE? Aren't they super small? Maybe change
to an assert?

http://codereview.chromium.org/159263/diff/1012/1017#newcode2062
Line 2062: ASSERT(JSObject::cast(result)->GetElementsKind() ==
JSObject::FAST_ELEMENTS);
I would keep HasFastElements.

http://codereview.chromium.org/159263/diff/1012/1029
File src/heap.h (right):

http://codereview.chromium.org/159263/diff/1012/1029#newcode434
Line 434: static Object* AllocatePixelArray(int lenght, uint8_t*
external_pointer);
lenght -> length

http://codereview.chromium.org/159263/diff/1012/1029#newcode434
Line 434: static Object* AllocatePixelArray(int lenght, uint8_t*
external_pointer);
Do we really need this variant? Is it because it's faster than having a default
value for pretenure?

http://codereview.chromium.org/159263/diff/1012/1021
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/159263/diff/1012/1021#newcode294
Line 294: __ cmp(eax, FieldOperand(ecx, FixedArray::kLengthOffset));
Shouldn't this be PixelArray::kLengthOffset?

http://codereview.chromium.org/159263/diff/1012/1021#newcode422
Line 422: __ cmp(ebx, FieldOperand(ecx, FixedArray::kLengthOffset));
PixelArray::kLengthOffset?

http://codereview.chromium.org/159263/diff/1012/1019
File src/objects.cc (right):

http://codereview.chromium.org/159263/diff/1012/1019#newcode1153
Line 1153: break;
Remove this break?

http://codereview.chromium.org/159263/diff/1012/1019#newcode2249
Line 2249: CHECK(!HasPixelElements());
Maybe change to an ASSERT?

http://codereview.chromium.org/159263/diff/1012/1019#newcode2336
Line 2336: CHECK(!HasPixelElements());
ASSERT?

http://codereview.chromium.org/159263/diff/1012/1019#newcode2419
Line 2419: // TODO(iposva): Cleanup.
What kind of cleanup is missing here?

http://codereview.chromium.org/159263/diff/1012/1019#newcode2762
Line 2762: // TODO(iposva): Cleanup
?

http://codereview.chromium.org/159263/diff/1012/1019#newcode2884
Line 2884: JSObject* jsObject = JSObject::cast(obj);
Weird naming here? jsObject -> js_object (not your fault, but anyway)

http://codereview.chromium.org/159263/diff/1012/1019#newcode3110
Line 3110: UNIMPLEMENTED();
I think you have to deal with this and add a test case that proves that the code
works.

http://codereview.chromium.org/159263/diff/1012/1019#newcode5323
Line 5323: break;
Having a break after return seems a bit weird.

http://codereview.chromium.org/159263/diff/1012/1019#newcode5370
Line 5370: }
Missing break (and test case).

http://codereview.chromium.org/159263/diff/1012/1019#newcode5759
Line 5759: UNIMPLEMENTED();
This needs implementing - and a test case ;-)

http://codereview.chromium.org/159263/diff/1012/1019#newcode7068
Line 7068: // TODO(iposva): Check for NaN values here.
Needs an implementation and a test case.

http://codereview.chromium.org/159263/diff/1012/1024
File src/objects.h (right):

http://codereview.chromium.org/159263/diff/1012/1024#newcode2460
Line 2460: // In particular write access do clamp the values to 0 or 255 if the
value
do clamp -> clamps

http://codereview.chromium.org/159263/diff/1012/1024#newcode2472
Line 2472: // This accessor does apply the correct conversion and clamping on
the value.
does apply -> applies

http://codereview.chromium.org/159263/diff/1012/1023
File src/runtime.cc (right):

http://codereview.chromium.org/159263/diff/1012/1023#newcode174
Line 174: UNIMPLEMENTED();
Implementation?

http://codereview.chromium.org/159263/diff/1012/1023#newcode3733
Line 3733: if (array->GetElementsKind() != JSObject::FAST_ELEMENTS) {
Keep as !HasFastElements.

http://codereview.chromium.org/159263/diff/1012/1023#newcode5209
Line 5209: UNIMPLEMENTED();
This probably needs an implementation too. It seems to be used by the debugger.

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

http://codereview.chromium.org/159263/diff/1012/1013#newcode7585
Line 7585: "};"
Why ;? (repeated)

http://codereview.chromium.org/159263/diff/1012/1013#newcode7633
Line 7633: result = CompileRun("pixels[7] = undefined;"
I might be nice to make sure this also works from an IC (add loop).

http://codereview.chromium.org/159263/diff/1012/1013#newcode7638
Line 7638: result = CompileRun("pixels[6] = '2.3';"
I might be nice to make sure this also works from an IC (add loop).

[iposva, 2009-07-28 03:28:12]

Uploaded a new version, ready for the next round.

-Ivan

http://codereview.chromium.org/159263/diff/1012/1022
File src/api.h (right):

http://codereview.chromium.org/159263/diff/1012/1022#newcode102
Line 102: ASSERT(value()->GetElementsKind() ==
On 2009/07/27 07:43:41, Kasper Lund wrote:
> Why not keep the HasFastElements checker?
 Part of my approach was to remove HasFastElement from the JSObject interface to
make sure that I caught all uses of it. Apparently I did not put some back...

http://codereview.chromium.org/159263/diff/1012/1018
File src/handles.cc (right):

http://codereview.chromium.org/159263/diff/1012/1018#newcode349
Line 349: *value.location() = *number;  // Overwrite the value being passed in.
On 2009/07/27 07:43:41, Kasper Lund wrote:
> This seems fishy to me. Why not just do 'value = number'?

Would have been too easy. Done.

http://codereview.chromium.org/159263/diff/1012/1017
File src/heap.cc (right):

http://codereview.chromium.org/159263/diff/1012/1017#newcode1874
Line 1874: size > MaxObjectSizeInPagedSpace() ? LO_SPACE : OLD_DATA_SPACE;
On 2009/07/27 07:43:41, Kasper Lund wrote:
> Do we ever get pixel arrays in LO_SPACE? Aren't they super small? Maybe change
> to an assert?

Done.

Initially pixel arrays contained the data instead of an external pointer. At
that point they would grow pretty quickly. I missed this change here, thanks for
pointing it out.

http://codereview.chromium.org/159263/diff/1012/1017#newcode1889
Line 1889: AllocationSpace space =
On 2009/07/27 07:43:41, Kasper Lund wrote:
> Do we ever get pixel arrays in LO_SPACE? Aren't they super small? Maybe change
> to an assert?

Done. This method has been removed.

http://codereview.chromium.org/159263/diff/1012/1017#newcode2062
Line 2062: ASSERT(JSObject::cast(result)->GetElementsKind() ==
JSObject::FAST_ELEMENTS);
On 2009/07/27 07:43:41, Kasper Lund wrote:
> I would keep HasFastElements.

Done.

http://codereview.chromium.org/159263/diff/1012/1029
File src/heap.h (right):

http://codereview.chromium.org/159263/diff/1012/1029#newcode434
Line 434: static Object* AllocatePixelArray(int lenght, uint8_t*
external_pointer);
On 2009/07/27 07:43:41, Kasper Lund wrote:
> Do we really need this variant? Is it because it's faster than having a
default
> value for pretenure?

The only reason is because I modeled it after the ByteArray code. The redundant
definition is gone.

http://codereview.chromium.org/159263/diff/1012/1021
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/159263/diff/1012/1021#newcode294
Line 294: __ cmp(eax, FieldOperand(ecx, FixedArray::kLengthOffset));
On 2009/07/27 07:43:41, Kasper Lund wrote:
> Shouldn't this be PixelArray::kLengthOffset?

Done.

http://codereview.chromium.org/159263/diff/1012/1021#newcode422
Line 422: __ cmp(ebx, FieldOperand(ecx, FixedArray::kLengthOffset));
On 2009/07/27 07:43:41, Kasper Lund wrote:
> PixelArray::kLengthOffset?

Done.

http://codereview.chromium.org/159263/diff/1012/1019
File src/objects.cc (right):

http://codereview.chromium.org/159263/diff/1012/1019#newcode1153
Line 1153: break;
On 2009/07/27 07:43:41, Kasper Lund wrote:
> Remove this break?

Done.

http://codereview.chromium.org/159263/diff/1012/1019#newcode2249
Line 2249: CHECK(!HasPixelElements());
On 2009/07/27 07:43:41, Kasper Lund wrote:
> Maybe change to an ASSERT?

Done.

http://codereview.chromium.org/159263/diff/1012/1019#newcode2419
Line 2419: // TODO(iposva): Cleanup.
On 2009/07/27 07:43:41, Kasper Lund wrote:
> What kind of cleanup is missing here?

Done. Needed to check whether this code could be reached. It can.

http://codereview.chromium.org/159263/diff/1012/1019#newcode2762
Line 2762: // TODO(iposva): Cleanup
On 2009/07/27 07:43:41, Kasper Lund wrote:
> ?

Done. Covered by the assert.

http://codereview.chromium.org/159263/diff/1012/1019#newcode2884
Line 2884: JSObject* jsObject = JSObject::cast(obj);
On 2009/07/27 07:43:41, Kasper Lund wrote:
> Weird naming here? jsObject -> js_object (not your fault, but anyway)

Done.

http://codereview.chromium.org/159263/diff/1012/1019#newcode3110
Line 3110: UNIMPLEMENTED();
On 2009/07/27 07:43:41, Kasper Lund wrote:
> I think you have to deal with this and add a test case that proves that the
code
> works.

I don't think JSArray can have its elements set to contain a pixel array. I
added an ASSERT to that extent at the beginning of the function.

http://codereview.chromium.org/159263/diff/1012/1019#newcode5323
Line 5323: break;
On 2009/07/27 07:43:41, Kasper Lund wrote:
> Having a break after return seems a bit weird.

Done. Old habit...

http://codereview.chromium.org/159263/diff/1012/1024
File src/objects.h (right):

http://codereview.chromium.org/159263/diff/1012/1024#newcode2460
Line 2460: // In particular write access do clamp the values to 0 or 255 if the
value
On 2009/07/27 07:43:41, Kasper Lund wrote:
> do clamp -> clamps

Done.

http://codereview.chromium.org/159263/diff/1012/1024#newcode2472
Line 2472: // This accessor does apply the correct conversion and clamping on
the value.
On 2009/07/27 07:43:41, Kasper Lund wrote:
> does apply -> applies

Done.

http://codereview.chromium.org/159263/diff/1012/1023
File src/runtime.cc (right):

http://codereview.chromium.org/159263/diff/1012/1023#newcode174
Line 174: UNIMPLEMENTED();
On 2009/07/27 07:43:41, Kasper Lund wrote:
> Implementation?

Actually since pixel arrays cannot be created using an object literal I added an
ASSERT(!copy->HasPixelElements()) before the switch statement.

http://codereview.chromium.org/159263/diff/1012/1023#newcode3733
Line 3733: if (array->GetElementsKind() != JSObject::FAST_ELEMENTS) {
On 2009/07/27 07:43:41, Kasper Lund wrote:
> Keep as !HasFastElements.

Done.

http://codereview.chromium.org/159263/diff/1012/1023#newcode5209
Line 5209: UNIMPLEMENTED();
On 2009/07/27 07:43:41, Kasper Lund wrote:
> This probably needs an implementation too. It seems to be used by the
debugger.

Done. One needs to do amazing __proto__ gymnastics to get here though.

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

http://codereview.chromium.org/159263/diff/1012/1013#newcode7585
Line 7585: "};"
On 2009/07/27 07:43:41, Kasper Lund wrote:
> Why ;? (repeated)

Done.

http://codereview.chromium.org/159263/diff/1012/1013#newcode7633
Line 7633: result = CompileRun("pixels[7] = undefined;"
On 2009/07/27 07:43:41, Kasper Lund wrote:
> I might be nice to make sure this also works from an IC (add loop).

Done.

http://codereview.chromium.org/159263/diff/1012/1013#newcode7638
Line 7638: result = CompileRun("pixels[6] = '2.3';"
On 2009/07/27 07:43:41, Kasper Lund wrote:
> I might be nice to make sure this also works from an IC (add loop).

Done.

[Kasper Lund, 2009-07-28 06:07:27]

LGTM, with a few comments:

http://codereview.chromium.org/159263/diff/55/58
File include/v8.h (right):

http://codereview.chromium.org/159263/diff/55/58#newcode1172
Line 1172: void SetElementsToPixelData(uint8_t* data, int length);
I still think you should avoid using the Elements name here, because it's not
really a term we use in the API. Also a comment on what this does and that the
'embedder' still owns the data would be nice.

http://codereview.chromium.org/159263/diff/55/59
File src/api.cc (right):

http://codereview.chromium.org/159263/diff/55/59#newcode2201
Line 2201: i::Handle<i::PixelArray> pixels = i::Factory::NewPixelArray(length,
data);
Should we have an API check (that also checks in release mode) that self isn't a
JSArray? I think that would be nice given the constraints of the current
implementation. Maybe also check that length is a valid smi?

http://codereview.chromium.org/159263/diff/55/65
File src/api.h (right):

http://codereview.chromium.org/159263/diff/55/65#newcode102
Line 102: ASSERT(value()->GetElementsKind() ==
Keep as HasFastElements?

http://codereview.chromium.org/159263/diff/55/65#newcode109
Line 109: ASSERT(value_->GetElementsKind() ==
Keep as HasFastElements?

http://codereview.chromium.org/159263/diff/55/64
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/159263/diff/55/64#newcode287
Line 287: // Check whether the elements are a pixel array.
are -> is

http://codereview.chromium.org/159263/diff/55/62
File src/objects.cc (right):

http://codereview.chromium.org/159263/diff/55/62#newcode1676
Line 1676: switch (JSObject::cast(pt)->GetElementsKind()) {
It might be clearer with !JSObject::cast(pt)->HasDictionaryElements, but I'm
okay either way.

http://codereview.chromium.org/159263/diff/55/62#newcode5879
Line 5879: case PIXEL_ELEMENTS: {
Move above the DICTIONARY_ELEMENTS case to match the more common ordering.

http://codereview.chromium.org/159263/diff/55/67
File src/objects.h (right):

http://codereview.chromium.org/159263/diff/55/67#newcode115
Line 115: class Array;
Move this to globals.h?

http://codereview.chromium.org/159263/diff/55/67#newcode2472
Line 2472: // This accessor applies the correct conversion and clamping on the
value.
Maybe extend the comment to state that it only deals with numbers and undefined
values?

[iposva, 2009-07-28 08:22:26]

http://codereview.chromium.org/159263/diff/55/58
File include/v8.h (right):

http://codereview.chromium.org/159263/diff/55/58#newcode1172
Line 1172: void SetElementsToPixelData(uint8_t* data, int length);
On 2009/07/28 06:07:27, Kasper Lund wrote:
> I still think you should avoid using the Elements name here, because it's not
> really a term we use in the API. Also a comment on what this does and that the
> 'embedder' still owns the data would be nice.

Changed to SetIndexedPropertiesToPixelData to be inline with other naming within
this file.

http://codereview.chromium.org/159263/diff/55/59
File src/api.cc (right):

http://codereview.chromium.org/159263/diff/55/59#newcode2201
Line 2201: i::Handle<i::PixelArray> pixels = i::Factory::NewPixelArray(length,
data);
On 2009/07/28 06:07:27, Kasper Lund wrote:
> Should we have an API check (that also checks in release mode) that self isn't
a
> JSArray? I think that would be nice given the constraints of the current
> implementation. Maybe also check that length is a valid smi?

Done.

http://codereview.chromium.org/159263/diff/55/65
File src/api.h (right):

http://codereview.chromium.org/159263/diff/55/65#newcode102
Line 102: ASSERT(value()->GetElementsKind() ==
On 2009/07/28 06:07:27, Kasper Lund wrote:
> Keep as HasFastElements?

Done.

http://codereview.chromium.org/159263/diff/55/65#newcode109
Line 109: ASSERT(value_->GetElementsKind() ==
On 2009/07/28 06:07:27, Kasper Lund wrote:
> Keep as HasFastElements?

Done.

http://codereview.chromium.org/159263/diff/55/64
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/159263/diff/55/64#newcode287
Line 287: // Check whether the elements are a pixel array.
On 2009/07/28 06:07:27, Kasper Lund wrote:
> are -> is

Done.

http://codereview.chromium.org/159263/diff/55/62
File src/objects.cc (right):

http://codereview.chromium.org/159263/diff/55/62#newcode1676
Line 1676: switch (JSObject::cast(pt)->GetElementsKind()) {
On 2009/07/28 06:07:27, Kasper Lund wrote:
> It might be clearer with !JSObject::cast(pt)->HasDictionaryElements, but I'm
> okay either way.

Done.

http://codereview.chromium.org/159263/diff/55/62#newcode5879
Line 5879: case PIXEL_ELEMENTS: {
On 2009/07/28 06:07:27, Kasper Lund wrote:
> Move above the DICTIONARY_ELEMENTS case to match the more common ordering.

Done.

http://codereview.chromium.org/159263/diff/55/67
File src/objects.h (right):

http://codereview.chromium.org/159263/diff/55/67#newcode115
Line 115: class Array;
On 2009/07/28 06:07:27, Kasper Lund wrote:
> Move this to globals.h?

Done.

http://codereview.chromium.org/159263/diff/55/67#newcode2472
Line 2472: // This accessor applies the correct conversion and clamping on the
value.
On 2009/07/28 06:07:27, Kasper Lund wrote:
> Maybe extend the comment to state that it only deals with numbers and
undefined
> values?

Done.