Bug 120112 – Typed Arrays have no public facing API

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Dominic Szablewski]

Relating to Bug 103454 - I see that Typed Arrays are now part of JSC itself, but it seems there's no public facing API to read/write/create them.

I previously shoved the old Typed Array implementation into a JSC fork and wrote a concise API for it. This worked out really well in practise:
https://github.com/phoboslab/JavaScriptCore-iOS/blob/typed-arrays/JavaScriptCore/API/JSTypedArray.h

[Dominic Szablewski]

I updated the Typed Array API methods to work with the 538.4 tag of JSC:

https://github.com/phoboslab/JavaScriptCore-iOS/blob/master/JavaScriptCore/API/JSTypedArray.h
https://github.com/phoboslab/JavaScriptCore-iOS/blob/master/JavaScriptCore/API/JSTypedArray.cpp

I'm sure there are things that could be done better, but I hope that's at least an inspiration to fix this "bug".

Currently, this issues and https://bugs.webkit.org/show_bug.cgi?id=106179 are the only things stopping us from using the version of JSC that ships with iOS7 in Ejecta ( github.com/phoboslab/ejecta ).

[Alexey Proskuryakov]

*** Bug 123058 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

There is quite a bit of discussion in bug 123058.

[Oliver Hunt]

Okay, here are thoughts about use cases that might be required:

* New API type that is based on a typed array
* Typed array referencing memory provided by an external source
* Extracting and working on typed array data - probably want type and pointer.
  - Could we provide a callback/block based api that could operate over the data without requiring slowdown and waste? e.g. a temporary pinning

I'm sure that there are other use cases
(incidentally maybe we want to update the C API with an index getter?)

[Oliver Hunt]

*** Bug 123927 has been marked as a duplicate of this bug. ***

[Isaac Burns]

Any chance for this to make it into iOS 8.0?

[Dominic Szablewski]

Any news on this?

A fast, native API to read/write typed arrays is absolutely critical for data exchange between JavaScript and native code for many apps. I hate that we have to bundle our own version of JSC with our apps, just because this one feature is missing.

The API that we wrote for our fork ( https://github.com/phoboslab/JavaScriptCore-iOS/blob/typed-arrays/JavaScriptCore/API/JSTypedArray.h ) has served us extremely well. It follows the style of the rest of the JSC API and could, imho, be a drop in solution for JSC proper.

[zhao shuan]

Is anyone working on this? We really need this API, otherwise we have to use our own version of JSC in our apps.

[Oliver Hunt]

Currently no one is working on this, but we'd be super happy if someone started designing the APIs we need to implement.  From there we can iterate the API in the bug until we get something we believe is satisfactory. Once we're there actually implementing it should be relatively easy.

[Sophie Alpert]

In bug 123058, the following API was suggested:

JSObjectGetTypedArrayBytesPtr(JSContextRef, JSObjectRef)
JSObjectGetTypedArrayByteLength(JSContextRef, JSObjectRef)

where they return null/0 for non-typed-array objects.

Would that be possible here?

[Dominic Szablewski]

Facebook's react-native isn't able to send or receive binary data over WebSockets or XHR, because there's still no native API:

https://github.com/facebook/react-native/issues/1424

I already wrote a Typed Array API for JSC that has served us extremely well for a few _years_ now [1]. Let me know if there's anything I can do to get this into JSC proper.

[1] https://github.com/phoboslab/JavaScriptCore-iOS/blob/typed-arrays/JavaScriptCore/API/JSTypedArray.h

[Radar WebKit Bug Importer]

<rdar://problem/22487505>

[Filip Pizlo]

(In reply to comment #11)
> Facebook's react-native isn't able to send or receive binary data over
> WebSockets or XHR, because there's still no native API:
> 
> https://github.com/facebook/react-native/issues/1424
> 
> I already wrote a Typed Array API for JSC that has served us extremely well
> for a few _years_ now [1]. Let me know if there's anything I can do to get
> this into JSC proper.

How about submitting a patch?

> 
> [1]
> https://github.com/phoboslab/JavaScriptCore-iOS/blob/typed-arrays/
> JavaScriptCore/API/JSTypedArray.h

This looks pretty good, but you need to clarify semantics.

JS_EXPORT JSObjectRef JSTypedArrayMake(JSContextRef ctx, JSTypedArrayType arrayType, size_t numElements);

What happens when arrayType is kJSTypedArrayTypeNone? or kJSTypedArrayTypeArrayBuffer? Probably any answer other than UB is a good answer.

JS_EXPORT void * JSTypedArrayGetDataPtr(JSContextRef ctx, JSValueRef value, size_t * byteLength);

Can you clarify what the caller must do to ensure that the returned pointer continues to point to valid memory? Is the caller expected to just keep the array alive? If so, then that's probably not what we want. It would mean that the following program would probably run fine in debug and maybe in release as well if you got lucky, but would actually be wrong because "array" is not live after the GC.

void foo()
{
    JSValueRef array = JSTypedArrayMake(ctx, kJSTypedArrayTypeInt8Array, 100);
    int8_t* data = JSTypedArrayGetDataPtr(ctx, array, 100);
    JSGarbageCollect(ctx);
    for (var i = 0; i < 100; ++i)
        data[i]++;
}

I think it's best if it was hard to write a program with such a mistake. One way to do this is to have GetDataPtr increment the ref count of the underlying buffer, and have an API like JSTypedArrayReleaseDataPtr(ctx, value, ptr) that decremented the ref count.

[Geoffrey Garen]

> JS_EXPORT JSTypedArrayType JSTypedArrayGetType(JSContextRef ctx, JSValueRef value);
> JS_EXPORT JSObjectRef JSTypedArrayMake(JSContextRef ctx, JSTypedArrayType arrayType, size_t numElements);
> JS_EXPORT void * JSTypedArrayGetDataPtr(JSContextRef ctx, JSValueRef value, size_t * byteLength);

The right phrasing here would be:

JSValueGetTypedArrayType(JSContextRef ctx, JSValueRef value);
OR
JSObjectGetTypedArrayType(JSContextRef ctx, JSObjectRef value);

JSObjectMakeTypedArray(JSContextRef ctx, JSTypedArrayType arrayType, size_t numElements);

JSValueGetTypedArrayDataPtr(JSContextRef ctx, JSValueRef value, size_t * byteLength);
OR
JSObjectGetTypedArrayDataPtr(JSContextRef ctx, JSObjectRef value, size_t * byteLength);

Why does getting the data pointer require a byte length? Why do we use byte length for access but element count for creation?

It seems like the uncontroversial idea in this API is an unforgeable way to create typed arrays using the C and ObjC APIs without having to save the JS constructor objects at the start of execution. Sort of like JSObjectMakeDate and JSObjectMakeRegExp. I'd like to see a patch that does that part for C and ObjC -- it's easy to get that right without solving the hard problem of how to get a data pointer.

[Filip Pizlo]

(In reply to comment #14)
> > JS_EXPORT JSTypedArrayType JSTypedArrayGetType(JSContextRef ctx, JSValueRef value);
> > JS_EXPORT JSObjectRef JSTypedArrayMake(JSContextRef ctx, JSTypedArrayType arrayType, size_t numElements);
> > JS_EXPORT void * JSTypedArrayGetDataPtr(JSContextRef ctx, JSValueRef value, size_t * byteLength);
> 
> The right phrasing here would be:
> 
> JSValueGetTypedArrayType(JSContextRef ctx, JSValueRef value);
> OR
> JSObjectGetTypedArrayType(JSContextRef ctx, JSObjectRef value);
> 
> JSObjectMakeTypedArray(JSContextRef ctx, JSTypedArrayType arrayType, size_t
> numElements);
> 
> JSValueGetTypedArrayDataPtr(JSContextRef ctx, JSValueRef value, size_t *
> byteLength);
> OR
> JSObjectGetTypedArrayDataPtr(JSContextRef ctx, JSObjectRef value, size_t *
> byteLength);
> 
> Why does getting the data pointer require a byte length? Why do we use byte
> length for access but element count for creation?
> 
> It seems like the uncontroversial idea in this API is an unforgeable way to
> create typed arrays using the C and ObjC APIs without having to save the JS
> constructor objects at the start of execution. Sort of like JSObjectMakeDate
> and JSObjectMakeRegExp. I'd like to see a patch that does that part for C
> and ObjC -- it's easy to get that right without solving the hard problem of
> how to get a data pointer.

I actually don't think that the data pointer is a hard problem, and I don't think it's controversial.  We should just do it, and we should just use reference counting - where GetTypedArrayDataPtr() is a retain and some other function is a release - is the answer for lifetime issues.  Another solution is to flip this around and have a JSObjectMakeTypedArrayWithData(context, basePointer, byteLength, type, keep) function.  I believe that the Java equivalent of typed arrays (java.nio.Buffer and friends) takes this approach in its native API.  I'd be happiest if we did both.

Note that none of the usual complaints about such an API work here.  You can't argue that the GC might move the memory, because our typed array implementation already supports pinning.  We can pin any typed array backing store at any time, and this is well-tested functionality since it already happens in WebCore.  You can't say that it's weird for native code to have a pointer to what is sort of a managed object, because our typed array implementation is deliberately designed around giving a pointer to the memory to native code and WebCore already uses this.  Also, array buffers already support ref counting in addition to GC marking, so that makes it even simpler to just expose a ref-countable raw buffer to API clients.  So, we should support some variant of this API, provided that we pick some sensible answer to questions like "how long does the memory live" when you call GetTypedArrayDataPtr().  Does anyone have an argument against having a ReleaseDataPtr() API that is the release counterpart of GetDataPtr()?

[Geoffrey Garen]

How do you create a typed array with a data pointer? Is the input a void*? Is the data copied or referenced through some delegate API?

[Filip Pizlo]

(In reply to comment #16)
> How do you create a typed array with a data pointer? Is the input a void*?

Yes.

> Is the data copied or referenced through some delegate API?

Referenced.  For example the "keep" argument could be interpreted as:

keep=true: we call the system free() on the original void* when we decide to free the buffer.

keep=false: we don't free the original void* buffer when we free the array buffer, and the only way that the client can be sure that we no longer want the backing data is when they destroy the VM.

Having a free callback would be even better.

The use cases for this are things like:

- Native code mmap's something and wants JS to be able to modify the mmap'd memory.
- Native code gets some buffer from some other native API and wants JS to be able to play with that buffer.
- Ad hoc wrapping of native buffers in C->JS calls (in that case keep=false works well enough).  Like, C code gets a piece of text and wants to zero-copy pass it to some JS handler, with the expectation that the handler won't squirrel away that buffer after it returns.

[Sophie Alpert]

> It seems like the uncontroversial idea in this API is an unforgeable way to create typed arrays using the C and ObjC APIs without having to save the JS constructor objects at the start of execution.

This does seem less controversial but it's also less useful because the same functionality can already be recreated by saving the constructors and using them, as you say.

Having to call JSTypedArrayReleaseDataPtr feels reasonable to me.

[Geoffrey Garen]

> Referenced.  For example the "keep" argument could be interpreted as:
> 
> keep=true: we call the system free() on the original void* when we decide to
> free the buffer.
> 
> keep=false: we don't free the original void* buffer when we free the array
> buffer, and the only way that the client can be sure that we no longer want
> the backing data is when they destroy the VM.

These options don't seem very good to me in a general purpose API.

We can't assume that our client used malloc. They might have used new, mmap, objc_createInstance, or a custom allocator.

Requiring a valid pointer for the lifetime of the VM is a difficult model to program with. For example, we couldn't use that model in the browser, or in the audio processing context being discussed on webkit-dev.

> Having a free callback would be even better.

This seems more reasonable to me.

It would be nice if we didn't have to reinvent a generic API for "referenced counted data pointer". Perhaps we can write this API in terms of CFMutableDataRef and NSMutableData.

[Filip Pizlo]

(In reply to comment #19)
> > Referenced.  For example the "keep" argument could be interpreted as:
> > 
> > keep=true: we call the system free() on the original void* when we decide to
> > free the buffer.
> > 
> > keep=false: we don't free the original void* buffer when we free the array
> > buffer, and the only way that the client can be sure that we no longer want
> > the backing data is when they destroy the VM.
> 
> These options don't seem very good to me in a general purpose API.
> 
> We can't assume that our client used malloc. They might have used new, mmap,
> objc_createInstance, or a custom allocator.
> 
> Requiring a valid pointer for the lifetime of the VM is a difficult model to
> program with. For example, we couldn't use that model in the browser, or in
> the audio processing context being discussed on webkit-dev.
> 
> > Having a free callback would be even better.
> 
> This seems more reasonable to me.
> 
> It would be nice if we didn't have to reinvent a generic API for "referenced
> counted data pointer". Perhaps we can write this API in terms of
> CFMutableDataRef and NSMutableData.

Hmmm, that's a good point.  NSData already has everything we need:

- We can return a NSData or CFDataRef when someone asks for a raw pointer to a typed array.  The NSData would have a deallocator that decrements our internal buffer ref count.

- We can create an array buffer when given a NSData. It would be easy to modify our internal ArrayBuffer code to make it capable of calling a custom destructor and/or just [object release].

[Sophie Alpert]

Unless I'm mistaken, NSData/CFDataRef require Foundation/CoreFoundation which some users of JavaScriptCore don't have available.

[Dominic Szablewski]

So, I have now changed the phrasing in my implementation as suggested by Geoffrey Garen:

JS_EXPORT JSTypedArrayType JSObjectGetTypedArrayType(JSContextRef ctx, JSObjectRef object);
JS_EXPORT JSObjectRef JSObjectMakeTypedArray(JSContextRef ctx, JSTypedArrayType arrayType, size_t numElements);
JS_EXPORT void * JSObjectGetTypedArrayDataPtr(JSContextRef ctx, JSObjectRef object, size_t* byteLength);

See: https://github.com/phoboslab/JavaScriptCore-iOS/commit/ac4a76470f5adfa592846466f2f8a84d868c053f

While this follows the JSC naming scheme, it also makes the API a bit more cumbersome to use: now, every time you want to get the data pointer or type of a typed array, you first have to check if the JSValueRef you got in your function call is a JSObjectRef (using JSValueIsObject()). Allowing for JSValues to be passed into these functions (i.e. "JSValueGetTypedArrayDataPtr") would be much more convenient, but also not reflect the truth that a Typed Array is a JSObject, not a JSValue.


To answer a few of the questions here:

> [JSObjectMakeTypedArray] What happens when arrayType is kJSTypedArrayTypeNone? or kJSTypedArrayTypeArrayBuffer? 
In case of kJSTypedArrayTypeNone the function returns NULL. In case of kJSTypedArrayTypeArrayBuffer the function returns an ArrayBuffer with the specified number of elements (i.e. bytes), similar to JS ArrayBuffer constructor (new ArrayBuffer(length)).


> Can you clarify what the caller must do to ensure that the returned pointer continues to point to valid memory? 
My assumption was that a call to JSValueProtect() on the Typed Array would also ensure the pointer remains valid, but I'm not really sure if that's actually the case. Maybe Filip Pizlo can chime in: when/how exactly is the underlying buffer "pinned"?

If JSValueProtect() indeed already ensures the pointer remains valid, I don't think we need another set of functions to retain/release the data pointer. Imho it's obvious that if the JSObject is GC'ed, the pointer will become invalid. If you want to hold on to the pointer, hold on to the JSObject.


> Why does getting the data pointer require a byte length?
I found that if you want to obtain the data pointer of a Typed Array, you almost always also need to get the byte length of that data. Note that the pointer to the size_t you pass in, is only used for output! Having two separate API calls for this seemed wasteful. If you don't need the byte length, simply pass NULL instead of a pointer to a size_t:
void * ptr = JSObjectGetTypedArrayDataPtr(ctx, object, NULL);


> Why do we use byte length for access but element count for creation?
JSObjectGetTypedArrayDataPtr() returns a pointer to raw data. If we were to return the element count instead of the byte length, you would always need to call JSObjectGetTypedArrayType() as well, to figure out how many bytes you could access. Using the byte length here greatly simplifies stuff like memcpy() to/from own buffers for API users.

Likewise, we use an element count for JSObjectMakeTypedArray() because 
1) you know the type of the Typed Array you want to create anyway
2) usually you know the number of elements you want to store when creating a Typed Array through the API (this is different from being passed a Typed Array from JS Land!)
3) using a byte length here would be error prone: what if I try to create a Uint32Array with a byte length of 5?


> How do you create a typed array with a data pointer? Is the input a void*?
I don't think we need an API for this. Just let JSC handle the data allocation and then obtain a pointer to it.

JSObjectRef array = JSObjectMakeTypedArray(ctx, kJSTypedArrayTypeUint8Array, 1024);
void *dataPtr = JSObjectGetTypedArrayDataPtr(ctx, array, NULL);
memcpy(dataPtr, myOwnData, 1024);

Imho it's not worth to complicate the API to avoid a short memcpy() for handing over data to JS Land.

[Oliver Hunt]

(In reply to comment #22)

> > Can you clarify what the caller must do to ensure that the returned pointer continues to point to valid memory? 
> My assumption was that a call to JSValueProtect() on the Typed Array would
> also ensure the pointer remains valid, but I'm not really sure if that's
> actually the case. Maybe Filip Pizlo can chime in: when/how exactly is the
> underlying buffer "pinned"?
> 
> If JSValueProtect() indeed already ensures the pointer remains valid, I
> don't think we need another set of functions to retain/release the data
> pointer. Imho it's obvious that if the JSObject is GC'ed, the pointer will
> become invalid. If you want to hold on to the pointer, hold on to the
> JSObject.
> 

I'm not sure about this - Fil, do we guarantee we won't move a typed array's backing buffer if the referencing object is pinned?

> 
> > Why does getting the data pointer require a byte length?
> I found that if you want to obtain the data pointer of a Typed Array, you
> almost always also need to get the byte length of that data. Note that the
> pointer to the size_t you pass in, is only used for output! Having two
> separate API calls for this seemed wasteful. If you don't need the byte
> length, simply pass NULL instead of a pointer to a size_t:
> void * ptr = JSObjectGetTypedArrayDataPtr(ctx, object, NULL);

I don't believe we have any other api that behaves in this way.
API consistency is important.

> > Why do we use byte length for access but element count for creation?
> JSObjectGetTypedArrayDataPtr() returns a pointer to raw data. If we were to
> return the element count instead of the byte length, you would always need
> to call JSObjectGetTypedArrayType() as well, to figure out how many bytes
> you could access. Using the byte length here greatly simplifies stuff like
> memcpy() to/from own buffers for API users.

r- just based on this API decision.

You're optimizing for a single case (memcpy), over the common case: casting the array of X into an X*.

Mixing and matching what unit the length you're using is a recipe for mistakes.

> Likewise, we use an element count for JSObjectMakeTypedArray() because 
> 1) you know the type of the Typed Array you want to create anyway
> 2) usually you know the number of elements you want to store when creating a
> Typed Array through the API (this is different from being passed a Typed
> Array from JS Land!)
> 3) using a byte length here would be error prone: what if I try to create a
> Uint32Array with a byte length of 5?

Which is why we should always be using the item count. If absolutely necessary you could have a GetByteLength function just as the js api has.

> 
> 
> > How do you create a typed array with a data pointer? Is the input a void*?
> I don't think we need an API for this. Just let JSC handle the data
> allocation and then obtain a pointer to it.
> 
> JSObjectRef array = JSObjectMakeTypedArray(ctx, kJSTypedArrayTypeUint8Array,
> 1024);
> void *dataPtr = JSObjectGetTypedArrayDataPtr(ctx, array, NULL);
> memcpy(dataPtr, myOwnData, 1024);
> 
> Imho it's not worth to complicate the API to avoid a short memcpy() for
> handing over data to JS Land.
Again, optimizing for memcpy is not the right trade off here.

Also this API fails to allow sharing of data allocated elsewhere, that can lead to unnecessary memory copying. Most other data related APIs on OS X allow specifying the external buffer to use -- in fact most APIs take the buffer pointer, and only allocate if that pointer is null.

[Dominic Szablewski]

> [Passing pointer to size_t to receive byteLength] I don't believe we have any other api that behaves in this way.
All API functions that may throw an exception use this semantic (if I understood your concerns correctly).

JSValueRef exception = NULL;
JSValueToObject(ctx, value, &exception);

I would argue that doing the same thing with a size_t is not really different. However...

> Mixing and matching what unit the length you're using is a recipe for mistakes.

Point taken. 

So I propose the following set of functions:

JSTypedArrayType JSObjectGetTypedArrayType(JSContextRef ctx, JSObjectRef object);
JSObjectRef JSObjectMakeTypedArray(JSContextRef ctx, JSTypedArrayType arrayType, size_t numElements);
void * JSObjectGetTypedArrayDataPtr(JSContextRef ctx, JSObjectRef object);
size_t JSObjectGetTypedArrayByteLength(JSContextRef ctx, JSObjectRef object);
size_t JSObjectGetTypedArrayNumElements(JSContextRef ctx, JSObjectRef object);


> Also this API fails to allow sharing of data allocated elsewhere (...) Most other data related APIs on OS X allow specifying the external buffer

The JSC API *nowhere* adheres to this. Everything is copied all over the place. Even the somewhat lower level JSString functions copy everything.

Also, as far as I can tell, none of the existing Typed Array or ArrayBuffer constructors allow for using an external data pointer. The data is always allocated internally. The existing WebGL implementation in WebKit gets along just fine with this.

My proposed Typed Array API would still allow for most use cases to avoid copying. E.g. in Ejecta, the Canvas2D .getImageData() functions uses glReadPixels() directly with a Typed Array dataPtr I.e.:


void *dataPtr = JSObjectGetTypedArrayDataPtr(ctx, array);
size_t byteLength = JSObjectGetTypedArrayByteLength(ctx, array);
if( byteLength >= width * height * bytesPerPixel ) {
	glReadPixels(x, y, width, height, format, type, dataPtr);
}


An additional "JSObjectMakeTypedArrayWithDataPtrNoCopy" function (not sure what's the right phrasing here) may have its place, but I still don't believe it's critical and it may even be more confusing than helpful.

[Filip Pizlo]

(In reply to comment #23)
> (In reply to comment #22)
> 
> > > Can you clarify what the caller must do to ensure that the returned pointer continues to point to valid memory? 
> > My assumption was that a call to JSValueProtect() on the Typed Array would
> > also ensure the pointer remains valid, but I'm not really sure if that's
> > actually the case. Maybe Filip Pizlo can chime in: when/how exactly is the
> > underlying buffer "pinned"?
> > 
> > If JSValueProtect() indeed already ensures the pointer remains valid, I
> > don't think we need another set of functions to retain/release the data
> > pointer. Imho it's obvious that if the JSObject is GC'ed, the pointer will
> > become invalid. If you want to hold on to the pointer, hold on to the
> > JSObject.
> > 
> 
> I'm not sure about this - Fil, do we guarantee we won't move a typed array's
> backing buffer if the referencing object is pinned?

Let's not get confused about the issues here.  Nobody is pinning anything, since we don't have a pinning API.  There is no way to say "please don't move this object or things that this object points to".  Incidentally it just so happens that the JSCell backing a JSObjectRef will not move, but that's not because it's pinned; it's because we don't move JSCells.

> 
> > 
> > > Why does getting the data pointer require a byte length?
> > I found that if you want to obtain the data pointer of a Typed Array, you
> > almost always also need to get the byte length of that data. Note that the
> > pointer to the size_t you pass in, is only used for output! Having two
> > separate API calls for this seemed wasteful. If you don't need the byte
> > length, simply pass NULL instead of a pointer to a size_t:
> > void * ptr = JSObjectGetTypedArrayDataPtr(ctx, object, NULL);
> 
> I don't believe we have any other api that behaves in this way.
> API consistency is important.
> 
> > > Why do we use byte length for access but element count for creation?
> > JSObjectGetTypedArrayDataPtr() returns a pointer to raw data. If we were to
> > return the element count instead of the byte length, you would always need
> > to call JSObjectGetTypedArrayType() as well, to figure out how many bytes
> > you could access. Using the byte length here greatly simplifies stuff like
> > memcpy() to/from own buffers for API users.
> 
> r- just based on this API decision.

I think this is a little extreme.  I understand and appreciate the justification for using byte size, and in fact, the JS typed array API emphasizes byte sizes a lot.

> 
> You're optimizing for a single case (memcpy), over the common case: casting
> the array of X into an X*.
> 
> Mixing and matching what unit the length you're using is a recipe for
> mistakes.

Let's not use this kind of logic.  The typed array API is intended to give expert users who are mixing JS and C to exchange raw memory buffers between the two worlds.  This API must allow the user to shoot himself in the foot; if it doesn't then surely it is a useless API.

> 
> > Likewise, we use an element count for JSObjectMakeTypedArray() because 
> > 1) you know the type of the Typed Array you want to create anyway
> > 2) usually you know the number of elements you want to store when creating a
> > Typed Array through the API (this is different from being passed a Typed
> > Array from JS Land!)
> > 3) using a byte length here would be error prone: what if I try to create a
> > Uint32Array with a byte length of 5?
> 
> Which is why we should always be using the item count. If absolutely
> necessary you could have a GetByteLength function just as the js api has.

By that logic, the C API should be using a mix of byte sizes and element counts in its functions, just like the JS API already does.

> 
> > 
> > 
> > > How do you create a typed array with a data pointer? Is the input a void*?
> > I don't think we need an API for this. Just let JSC handle the data
> > allocation and then obtain a pointer to it.
> > 
> > JSObjectRef array = JSObjectMakeTypedArray(ctx, kJSTypedArrayTypeUint8Array,
> > 1024);
> > void *dataPtr = JSObjectGetTypedArrayDataPtr(ctx, array, NULL);
> > memcpy(dataPtr, myOwnData, 1024);
> > 
> > Imho it's not worth to complicate the API to avoid a short memcpy() for
> > handing over data to JS Land.
> Again, optimizing for memcpy is not the right trade off here.
> 
> Also this API fails to allow sharing of data allocated elsewhere, that can
> lead to unnecessary memory copying. Most other data related APIs on OS X
> allow specifying the external buffer to use -- in fact most APIs take the
> buffer pointer, and only allocate if that pointer is null.

[Filip Pizlo]

(In reply to comment #24)
> > [Passing pointer to size_t to receive byteLength] I don't believe we have any other api that behaves in this way.
> All API functions that may throw an exception use this semantic (if I
> understood your concerns correctly).
> 
> JSValueRef exception = NULL;
> JSValueToObject(ctx, value, &exception);
> 
> I would argue that doing the same thing with a size_t is not really
> different. However...
> 
> > Mixing and matching what unit the length you're using is a recipe for mistakes.
> 
> Point taken. 
> 
> So I propose the following set of functions:
> 
> JSTypedArrayType JSObjectGetTypedArrayType(JSContextRef ctx, JSObjectRef
> object);
> JSObjectRef JSObjectMakeTypedArray(JSContextRef ctx, JSTypedArrayType
> arrayType, size_t numElements);
> void * JSObjectGetTypedArrayDataPtr(JSContextRef ctx, JSObjectRef object);
> size_t JSObjectGetTypedArrayByteLength(JSContextRef ctx, JSObjectRef object);
> size_t JSObjectGetTypedArrayNumElements(JSContextRef ctx, JSObjectRef
> object);
> 
> 
> > Also this API fails to allow sharing of data allocated elsewhere (...) Most other data related APIs on OS X allow specifying the external buffer
> 
> The JSC API *nowhere* adheres to this. Everything is copied all over the
> place. Even the somewhat lower level JSString functions copy everything.
> 
> Also, as far as I can tell, none of the existing Typed Array or ArrayBuffer
> constructors allow for using an external data pointer. The data is always
> allocated internally. The existing WebGL implementation in WebKit gets along
> just fine with this.
> 
> My proposed Typed Array API would still allow for most use cases to avoid
> copying. E.g. in Ejecta, the Canvas2D .getImageData() functions uses
> glReadPixels() directly with a Typed Array dataPtr I.e.:
> 
> 
> void *dataPtr = JSObjectGetTypedArrayDataPtr(ctx, array);
> size_t byteLength = JSObjectGetTypedArrayByteLength(ctx, array);
> if( byteLength >= width * height * bytesPerPixel ) {
> 	glReadPixels(x, y, width, height, format, type, dataPtr);
> }
> 
> 
> An additional "JSObjectMakeTypedArrayWithDataPtrNoCopy" function (not sure
> what's the right phrasing here) may have its place, but I still don't
> believe it's critical and it may even be more confusing than helpful.

This isn't about creating an API just for you.  Data APIs on OSX allow you to create data object wrappers that borrow (or take ownership) of some data blob.  What you are proposing is changes to an OSX API.  Therefore, we like to be consistent with how OSX does things.

[Dominic Szablewski]

> This isn't about creating an API just for you.  Data APIs on OSX allow you
> to create data object wrappers that borrow (or take ownership) of some data
> blob.  What you are proposing is changes to an OSX API.  Therefore, we like
> to be consistent with how OSX does things.

I'm not sure I understand. JSC is used on a lot of platforms that are not OSX/iOS. I also never proposed a change to an OSX API. If you are talking about the new Obj-C API, as opposed to the C API I propose here, then yes, by all means it should follow OSX semantics. The existing C API however does not follow these.

I understand that a TypedArray API will be used for a whole number of different things. Obviously I can only speak for my own use cases here, but I'd argue that Ejecta already covers many of these, as it offers a full WebSocket, XMLHttpRequest, Canvas2D and WebGL implementation that makes heavy use TypedArrays.

[Filip Pizlo]

(In reply to comment #22)
> So, I have now changed the phrasing in my implementation as suggested by
> Geoffrey Garen:
> 
> JS_EXPORT JSTypedArrayType JSObjectGetTypedArrayType(JSContextRef ctx,
> JSObjectRef object);
> JS_EXPORT JSObjectRef JSObjectMakeTypedArray(JSContextRef ctx,
> JSTypedArrayType arrayType, size_t numElements);
> JS_EXPORT void * JSObjectGetTypedArrayDataPtr(JSContextRef ctx, JSObjectRef
> object, size_t* byteLength);
> 
> See:
> https://github.com/phoboslab/JavaScriptCore-iOS/commit/
> ac4a76470f5adfa592846466f2f8a84d868c053f
> 
> While this follows the JSC naming scheme, it also makes the API a bit more
> cumbersome to use: now, every time you want to get the data pointer or type
> of a typed array, you first have to check if the JSValueRef you got in your
> function call is a JSObjectRef (using JSValueIsObject()). Allowing for
> JSValues to be passed into these functions (i.e.
> "JSValueGetTypedArrayDataPtr") would be much more convenient, but also not
> reflect the truth that a Typed Array is a JSObject, not a JSValue.

Yeah.  Personally, I would not object to JSValueGetTypedArrayType in addition to JSObjectGetTypedArrayType, particularly since JSValueGetTypedArrayType has a very obvious spec and could even be implemented in terms of the other APIs.

> 
> 
> To answer a few of the questions here:
> 
> > [JSObjectMakeTypedArray] What happens when arrayType is kJSTypedArrayTypeNone? or kJSTypedArrayTypeArrayBuffer? 
> In case of kJSTypedArrayTypeNone the function returns NULL. In case of
> kJSTypedArrayTypeArrayBuffer the function returns an ArrayBuffer with the
> specified number of elements (i.e. bytes), similar to JS ArrayBuffer
> constructor (new ArrayBuffer(length)).
> 
> 
> > Can you clarify what the caller must do to ensure that the returned pointer continues to point to valid memory? 
> My assumption was that a call to JSValueProtect() on the Typed Array would
> also ensure the pointer remains valid, but I'm not really sure if that's
> actually the case. Maybe Filip Pizlo can chime in: when/how exactly is the
> underlying buffer "pinned"?

The underlying buffer can be moved by the GC.  This will happen even if you called JSValueProtect().

Currently internally in JSC the underlying buffer will be pinned (i.e. it won't move) so long as the typed array object is marked (JSValueProtect() ensures this) and so long as one of two other things happens:

1) The GC sees a pointer into that backing store on the stack.  This is transient, in the sense that if during some GC we pin the backing store, we may still move it in a future GC if in that future GC there are no longer any pointers into that backing store on the stack.
2) We pin the backing store by using the typed array's slowDownAndWasteMemory() API.  This is persistent, in the sense that if we ever do this then the typed array backing store will never move again.

It's not clear to me that we want to expose API that requires us to set either of these two things in stone.  Both of these are implementation details that we might want to change.  We may prohibit having backing store pointers on the stack at all as part of the concurrent GC work.  It's not super obvious that the fact that we have an inefficient method of making the backing immovable (slowDownAndWasteMemory()) is a good idea; we introduced it because it was sort of the most natural thing for the DOM at the time, but I'm not sure that we want an API that essentially mandates this.

That said, you could rely on either (1) or (2) and say that so long as you've JSValueProtect()'d the object and your pointer to the backing store is on the stack, then your backing store pointer is valid.  This is dangerous because this would force us to have a good definition of what it means for a pointer to an array to be on the stack.  Does an interior pointer count?  Does an offset pointer count?  What if the compiler sees that you wrote a loop over the backing store and as an optimization, it drops the pointer to the base of the array and instead has some crazy offset pointer that facilitates some exotic loop transformation - in that case its not at all clear that our GC would recognize the pointer that the compiler ended up using as being a pointer to that backing store.  These are tough questions and I don't want an API to have to answer.

Relying on either (1) or (2) is also weird because in all other JSC APIs, you don't have to JSValueProtect() an object if you're only referencing it from the stack.  So, if we introduced an API that mandating JSValueProtect()'ing a typed array object whenever you got its backing store, then this would be both error-prone and surprising.  Just imagine how you would explain this in the documentation for JSValueProtect().  That documentation currently implies that there is no need to JSValueProtect() if your stuff is on the stack.  I would anticipate that lots of people would not call JSValueProtect() in this case, and would only discover the error by way of heisencrashes.

So, for these reasons, it's worthwhile to have a slightly more verbose API that requires you to "release" the backing store.

> 
> If JSValueProtect() indeed already ensures the pointer remains valid, I
> don't think we need another set of functions to retain/release the data
> pointer. Imho it's obvious that if the JSObject is GC'ed, the pointer will
> become invalid. If you want to hold on to the pointer, hold on to the
> JSObject.

See above.  JSValueProtect() is not currently used for protecting values that are only referenced from the stack, and it's not clear that this is enough to guarantee that the GC won't move the backing store even if the typed array object is protected.

> 
> 
> > Why does getting the data pointer require a byte length?
> I found that if you want to obtain the data pointer of a Typed Array, you
> almost always also need to get the byte length of that data. Note that the
> pointer to the size_t you pass in, is only used for output! Having two
> separate API calls for this seemed wasteful. If you don't need the byte
> length, simply pass NULL instead of a pointer to a size_t:
> void * ptr = JSObjectGetTypedArrayDataPtr(ctx, object, NULL);
> 
> 
> > Why do we use byte length for access but element count for creation?
> JSObjectGetTypedArrayDataPtr() returns a pointer to raw data. If we were to
> return the element count instead of the byte length, you would always need
> to call JSObjectGetTypedArrayType() as well, to figure out how many bytes
> you could access. Using the byte length here greatly simplifies stuff like
> memcpy() to/from own buffers for API users.
> 
> Likewise, we use an element count for JSObjectMakeTypedArray() because 
> 1) you know the type of the Typed Array you want to create anyway
> 2) usually you know the number of elements you want to store when creating a
> Typed Array through the API (this is different from being passed a Typed
> Array from JS Land!)
> 3) using a byte length here would be error prone: what if I try to create a
> Uint32Array with a byte length of 5?
> 
> 
> > How do you create a typed array with a data pointer? Is the input a void*?
> I don't think we need an API for this. Just let JSC handle the data
> allocation and then obtain a pointer to it.
> 
> JSObjectRef array = JSObjectMakeTypedArray(ctx, kJSTypedArrayTypeUint8Array,
> 1024);
> void *dataPtr = JSObjectGetTypedArrayDataPtr(ctx, array, NULL);
> memcpy(dataPtr, myOwnData, 1024);
> 
> Imho it's not worth to complicate the API to avoid a short memcpy() for
> handing over data to JS Land.

[Filip Pizlo]

(In reply to comment #27)
> > This isn't about creating an API just for you.  Data APIs on OSX allow you
> > to create data object wrappers that borrow (or take ownership) of some data
> > blob.  What you are proposing is changes to an OSX API.  Therefore, we like
> > to be consistent with how OSX does things.
> 
> I'm not sure I understand. JSC is used on a lot of platforms that are not
> OSX/iOS. I also never proposed a change to an OSX API. If you are talking
> about the new Obj-C API, as opposed to the C API I propose here, then yes,
> by all means it should follow OSX semantics. The existing C API however does
> not follow these.

The JSC C API, which you are proposing changes to, is a public API on OSX.

> 
> I understand that a TypedArray API will be used for a whole number of
> different things. Obviously I can only speak for my own use cases here, but
> I'd argue that Ejecta already covers many of these, as it offers a full
> WebSocket, XMLHttpRequest, Canvas2D and WebGL implementation that makes
> heavy use TypedArrays.

[Dominic Szablewski]

Thanks for the in-depth explanation, Filip!

So it seems an additional JSObjectReleaseTypedArrayDataPtr(ctx, object, ptr) function would be absolutely necessary.

Unfortunately I understand way to little about the innards of JSC to implement the pining/retaining of the backing store myself. I'm not sure what steps exactly JSObjectGetTypedArrayDataPtr() would need to take.

On the other handJSObjectReleaseTypedArrayDataPtr() would only need to ensure that the pointer really belongs to object and then "unpin"/release the object's backing store, right?


Again, I don't see myself fit to actually implement this in JSC, but I believe the API we now arrived at is pretty solid. Namely:


Create a TypedArray:
JSObjectRef JSObjectMakeTypedArray(JSContextRef ctx, JSTypedArrayType arrayType, size_t numElements);

Query type:
JSTypedArrayType JSObjectGetTypedArrayType(JSContextRef ctx, JSObjectRef object);

Query byte length and element count:
size_t JSObjectGetTypedArrayByteLength(JSContextRef ctx, JSObjectRef object);
size_t JSObjectGetTypedArrayNumElements(JSContextRef ctx, JSObjectRef object);

Obtain a pointer to the array's data (retains the pointer):
void * JSObjectGetTypedArrayDataPtr(JSContextRef ctx, JSObjectRef object);
or
void * JSObjectGetTypedArrayDataPtr(JSContextRef ctx, JSObjectRef object, size_t &byteLength);

Release pointer to the array's data:
JSObjectReleaseTypedArrayDataPtr(ctx, JSObjectRef object, void * dataPtr);


Is there anything else the API needs to cover?

Let me also state again that I don't think a method to create a Typed Array with a pointer (that was allocated elsewhere and would not be copied) is necessary. Current users of Typed Arrays within Webkit (XMLHttpRequest, WebSockets, WebGL) seem to get along without it. I.e. always letting JSC take care of the allocation doesn't seem to be a problem.

[Filip Pizlo]

(In reply to comment #30)
> Thanks for the in-depth explanation, Filip!
> 
> So it seems an additional JSObjectReleaseTypedArrayDataPtr(ctx, object, ptr)
> function would be absolutely necessary.
> 
> Unfortunately I understand way to little about the innards of JSC to
> implement the pining/retaining of the backing store myself. I'm not sure
> what steps exactly JSObjectGetTypedArrayDataPtr() would need to take.

JSArrayBufferView* view = jsDynamicCast<JSArrayBufferView*>(object);
if (!view)
    .... wrong object type
ArrayBuffer* buffer = view->buffer(); // this pins the buffer, and we know that it will remain pinned so long as you hold a +1 on the ArrayBuffer.
buffer->ref(); // do the +1 thing

The JSObjectReleaseTypedArrayDataPtr method could then just do something like:

JSArrayBufferView* view = jsDynamicCast<JSArrayBufferView*>(object);
if (!view)
    .... wrong object type
ArrayBuffer* buffer = view->buffer();
buffer->deref(); // tell the GC that we don't need pinning anymore

I believe that if you do it this way, it guarantees safety while ensuring that this API stays out of the way of our future VM hacks.

> 
> On the other handJSObjectReleaseTypedArrayDataPtr() would only need to
> ensure that the pointer really belongs to object and then "unpin"/release
> the object's backing store, right?

Yeah.

> 
> 
> Again, I don't see myself fit to actually implement this in JSC, but I
> believe the API we now arrived at is pretty solid. Namely:
> 
> 
> Create a TypedArray:
> JSObjectRef JSObjectMakeTypedArray(JSContextRef ctx, JSTypedArrayType
> arrayType, size_t numElements);
> 
> Query type:
> JSTypedArrayType JSObjectGetTypedArrayType(JSContextRef ctx, JSObjectRef
> object);
> 
> Query byte length and element count:
> size_t JSObjectGetTypedArrayByteLength(JSContextRef ctx, JSObjectRef object);
> size_t JSObjectGetTypedArrayNumElements(JSContextRef ctx, JSObjectRef
> object);
> 
> Obtain a pointer to the array's data (retains the pointer):
> void * JSObjectGetTypedArrayDataPtr(JSContextRef ctx, JSObjectRef object);
> or
> void * JSObjectGetTypedArrayDataPtr(JSContextRef ctx, JSObjectRef object,
> size_t &byteLength);
> 
> Release pointer to the array's data:
> JSObjectReleaseTypedArrayDataPtr(ctx, JSObjectRef object, void * dataPtr);
> 
> 
> Is there anything else the API needs to cover?
> 
> Let me also state again that I don't think a method to create a Typed Array
> with a pointer (that was allocated elsewhere and would not be copied) is
> necessary. Current users of Typed Arrays within Webkit (XMLHttpRequest,
> WebSockets, WebGL) seem to get along without it. I.e. always letting JSC
> take care of the allocation doesn't seem to be a problem.

OK.

Please also consider our earlier suggestion to have the API revolve around a C object that is something like:

struct Data {
    void* base;
    size_t size;
    void (*destructor)(void*);
};

Where JSObjectGetTypedArrayDataPtr would actually return a Data*.  You could imagine being able to then remove JSObjectReleaseTypedArrayDataPtr and instead mandate that the user calls data->destructor(data->base) when done.  This makes the API seem less ad hoc, IMO.  On OSX, Data* could be replaced with just CFDataRef.  In the future, having the notion of "Data" would make it easy to wrap an ArrayBuffer around a Data.  I know that you don't want it, but others might.  FWIW, I want this.

I know that you don't care about OSX APIs, and I totally get that.  But I have to care. :-)  It would be great to come up with something that we can admit into our API on OSX and that also serves your purposes.

[Dominic Szablewski]

(In reply to comment #31)
> JSArrayBufferView* view = jsDynamicCast<JSArrayBufferView*>(object);
> if (!view)
>     .... wrong object type
> ArrayBuffer* buffer = view->buffer(); // this pins the buffer, and we know
> that it will remain pinned so long as you hold a +1 on the ArrayBuffer.
> buffer->ref(); // do the +1 thing
> 
> The JSObjectReleaseTypedArrayDataPtr method could then just do something
> like:
> 
> JSArrayBufferView* view = jsDynamicCast<JSArrayBufferView*>(object);
> if (!view)
>     .... wrong object type
> ArrayBuffer* buffer = view->buffer();
> buffer->deref(); // tell the GC that we don't need pinning anymore
> 
> I believe that if you do it this way, it guarantees safety while ensuring
> that this API stays out of the way of our future VM hacks.

Thanks! I assumed that there would be more to it :) I noticed that ArrayBuffer also has pin() and unpin() methods. This seems to be used when transfer() is called, but the underlying data is not free()ed until the ArrayBuffer is released - it remains safe to use even when it's not pinned. So I'm unsure if pin() & unpin() need to be called as well.


> Please also consider our earlier suggestion to have the API revolve around a
> C object that is something like:
> 
> struct Data {
>     void* base;
>     size_t size;
>     void (*destructor)(void*);
> };
> 
> Where JSObjectGetTypedArrayDataPtr would actually return a Data*.  You could
> imagine being able to then remove JSObjectReleaseTypedArrayDataPtr and
> instead mandate that the user calls data->destructor(data->base) when done. 
> This makes the API seem less ad hoc, IMO.  On OSX, Data* could be replaced
> with just CFDataRef.  In the future, having the notion of "Data" would make
> it easy to wrap an ArrayBuffer around a Data.  I know that you don't want
> it, but others might.  FWIW, I want this.

Yeah, I think returning a struct would make it a cleaner API. However, to stay consistent with the rest of JSC, it should probably work more like JSStringRef. I.e.:

typedef struct OpaqueJSData* JSDataRef;

JS_EXPORT JSDataRetain(JSDataRef data);
JS_EXPORT JSDataRelease(JSDataRef data);
JS_EXPORT void * JSDataGetBytesPtr(JSDataRef data);
JS_EXPORT size_t JSDataGetLength(JSDataRef data);

JS_EXPORT JSDataRef JSObjectGetTypedArrayData(JSContextRef ctx, JSObjectRef object);

This would also make it easy and very obvious to have a JSObjectGetTypedArrayCFData function as well, just like JSStringRefCF.h does it.

However, the name JSObjectGetTypedArrayData doesn't make it clear that you have to release the JSDataRef again. In fact, according to the CF ownership policy it implies that you don't have to release() it. Naming it analogous to the String functions (JSObjectGetTypedArrayDataCopy) follows the create rule, but is extremely confusing in that it makes it sound like the whole data bytes are copied. I have no idea what the right phrasing would be here.


I'm sorry for abusing the bugtracker for discussion about the implementation. Is there a better place to discuss this?

My hope two years ago was that some of the core contributors would be interested in implementing this, as it's a huge endeavor for me to get up to speed on the style and contribution guidelines, implement and test the changes, build a patch, commit it and get it reviewed - all as a private hobby. Sorry if I'm getting on your nerves here with my questions.

[Geoffrey Garen]

> > While this follows the JSC naming scheme, it also makes the API a bit more
> > cumbersome to use: now, every time you want to get the data pointer or type
> > of a typed array, you first have to check if the JSValueRef you got in your
> > function call is a JSObjectRef (using JSValueIsObject()). Allowing for
> > JSValues to be passed into these functions (i.e.
> > "JSValueGetTypedArrayDataPtr") would be much more convenient, but also not
> > reflect the truth that a Typed Array is a JSObject, not a JSValue.
> 
> Yeah.  Personally, I would not object to JSValueGetTypedArrayType in
> addition to JSObjectGetTypedArrayType, particularly since
> JSValueGetTypedArrayType has a very obvious spec and could even be
> implemented in terms of the other APIs.

The fact that JSObject is separate from JSValue, requiring a type check and cast before use, is a mistake, which is fixed in the modern Objective-C API. But I think it's good to adhere to the mistake in the C API so that the API remains internally consistent. 

It's easy for clients to fix the mistake by writing their own 2 line JSValue wrappers for JSObject functions, and indeed that's what we do in WebKit.

[Geoffrey Garen]

> Create a TypedArray:
> JSObjectRef JSObjectMakeTypedArray(JSContextRef ctx, JSTypedArrayType
> arrayType, size_t numElements);

I would say "length" instead of numElements. We don't like to abbreviate in API names, and length is the name in JavaScript.

> Query byte length and element count:
> size_t JSObjectGetTypedArrayByteLength(JSContextRef ctx, JSObjectRef object);
> size_t JSObjectGetTypedArrayNumElements(JSContextRef ctx, JSObjectRef
> object);

Once again, length.

> Obtain a pointer to the array's data (retains the pointer):
> void * JSObjectGetTypedArrayDataPtr(JSContextRef ctx, JSObjectRef object);
> or
> void * JSObjectGetTypedArrayDataPtr(JSContextRef ctx, JSObjectRef object,
> size_t &byteLength);

We say "Create" or "Copy" when we make something that needs a release. Here I would use "Copy" I guess.

[Dominic Szablewski]

> We say "Create" or "Copy" when we make something that needs a release. Here
> I would use "Copy" I guess.

Right, but "JSObjectCopyTypedArrayData" would be misleading, in that it implies you get a copy of the data and modifying the data wouldn't do anything on the Typed Array from where it was copied. Maybe it should be named JSDataWrapper instead: i.e. JSObjectCopyTypedArrayDataWrapper()!? I'm not really fan of the verbosity, though.

Two more ideas:

a) JSObjectGetTypedArrayData() returns a JSDataRef unretained. If you want to hold on to it, you have to retain it yourself. I'm not sure if this is feasible - the JSDataRef would have to last as least until the scope of the function that called JSObjectGetTypedArrayData exits.

b) We name the function in a way that doesn't follow the create/copy nomenclature, but clearly states that you own the JSDataRef. I'd propose JSObjectGetRetainedTypedArrayData() or JSObjectGetTypedArrayDataRetain().

If it's possible in a sane matter, I think a) is preferable. Can anybody confirm if this would be feasible? Does JSC implement something like objc's autorelease?

[Geoffrey Garen]

(In reply to comment #35)
> > We say "Create" or "Copy" when we make something that needs a release. Here
> > I would use "Copy" I guess.
> 
> Right, but "JSObjectCopyTypedArrayData" would be misleading, in that it
> implies you get a copy of the data and modifying the data wouldn't do
> anything on the Typed Array from where it was copied.

Since the type name is "TypedArrayDataPtr", you would say "JSObjectCopyTypedArrayDataPtr". So, the function would claim to copy the pointer, not the underlying data. Since the pointer is a reference-counted object, it's reasonable to copy it.

> Maybe it should be
> named JSDataWrapper instead: i.e. JSObjectCopyTypedArrayDataWrapper()!? I'm
> not really fan of the verbosity, though.

I wouldn't use "Wrapper" because the other API names don't use it, and it's less specific than "data pointer".

> a) JSObjectGetTypedArrayData() returns a JSDataRef unretained. If you want
> to hold on to it, you have to retain it yourself. I'm not sure if this is
> feasible - the JSDataRef would have to last as least until the scope of the
> function that called JSObjectGetTypedArrayData exits.

It's impossible to return a reference counted object unretained. By definition, that's a pointer to freed memory (or leaked memory, depending on how you design things).

> b) We name the function in a way that doesn't follow the create/copy
> nomenclature, but clearly states that you own the JSDataRef. I'd propose
> JSObjectGetRetainedTypedArrayData() or JSObjectGetTypedArrayDataRetain().

Maybe JSObjectRetainTypedArrayDataPtr(). (Retain functions return their targets.)

I think the copy name is better because it is more idiomatic.

> If it's possible in a sane matter, I think a) is preferable. Can anybody
> confirm if this would be feasible? Does JSC implement something like objc's
> autorelease?

No autorelease. Only GC and reference counting.

You could make the data pointer a GC object. I don't think that changes things much, though.

[Dominic Szablewski]

(In reply to comment #36)
> (In reply to comment #35)
> > > We say "Create" or "Copy" when we make something that needs a release. Here
> > > I would use "Copy" I guess.
> > 
> > Right, but "JSObjectCopyTypedArrayData" would be misleading, in that it
> > implies you get a copy of the data and modifying the data wouldn't do
> > anything on the Typed Array from where it was copied.
> 
> Since the type name is "TypedArrayDataPtr", you would say
> "JSObjectCopyTypedArrayDataPtr". So, the function would claim to copy the
> pointer, not the underlying data. Since the pointer is a reference-counted
> object, it's reasonable to copy it.

In my proposal, I named the type "JSDataRef", not "TypedArrayDataPtr". Naming it "TypedArrayDataPtr" would be confusing when the returned type is not a pointer to raw data, but a ref to a "wrapper" and we would end up with function names like "TypedArrayDataPtrGetBytesPtr".


> I think the copy name is better because it is more idiomatic.

Honestly, I think these function names all come short for one reason or another:

- "JSObjectCopyTypedArrayData" implies that data is copied. 
- "JSObjectCopyTypedArrayDataRef" is not consistent with the rest of the JSC API. None of the other JSC API functions that work on refs actually have the "Ref" in the name.
- "JSObjectCopyTypedArrayDataPtr" implies you get a raw pointer to bytes.
- "JSObjectRetainTypedArrayData" is maybe the most sensible, but I'd really prefer to have a "get" in the name, because the primary function of this function is to *get* the data, not to retain it. It would also be confusing to have a separate "JSDataRetain".




Aaaaaanyway, current API proposal:

https://github.com/phoboslab/JavaScriptCore-iOS/blob/new-typed-array-api/JavaScriptCore/API/JSTypedArray.h

In short:

JSTypedArrayType JSObjectGetTypedArrayType(JSContextRef ctx, JSObjectRef object);
JSObjectRef JSObjectMakeTypedArray(JSContextRef ctx, JSTypedArrayType arrayType, size_t numElements);
JSDataRef JSObjectGetRetainedTypedArrayData(JSContextRef ctx, JSObjectRef object);

JSDataRef JSDataRetain(JSDataRef data);
void JSDataRelease(JSDataRef data);
void * JSDataGetBytesPtr(JSDataRef data);
size_t JSDataGetLength(JSDataRef data);


This API behaves very much like the JSStringRef stuff. From my perspective the only unresolved issue is the "JSObjectGetRetainedTypedArrayData" function name.


Thoughts?

[Scott Kyle]

(In reply to comment #37)
> Aaaaaanyway, current API proposal:
> 
> https://github.com/phoboslab/JavaScriptCore-iOS/blob/new-typed-array-api/
> JavaScriptCore/API/JSTypedArray.h
> 
> In short:
> 
> JSTypedArrayType JSObjectGetTypedArrayType(JSContextRef ctx, JSObjectRef
> object);
> JSObjectRef JSObjectMakeTypedArray(JSContextRef ctx, JSTypedArrayType
> arrayType, size_t numElements);

I know it doesn't really matter, but I think "numElements" should be changed to "length" to better match the nomenclature used in the JS API, and also because "numElements" make less sense when used with kJSTypedArrayTypeArrayBuffer.

To go along with this, I think JSDataGetLength would be be nicer represented by JSDataGetByteLength (matches byteLength API in JS) or JSDataGetByteCount (reads better to me and matches JSPropertyNameArrayGetCount).

> This API behaves very much like the JSStringRef stuff. From my perspective
> the only unresolved issue is the "JSObjectGetRetainedTypedArrayData"
> function name.

I know this is tricky one to name, but I think your proposed name is probably the best for being the most clear about its return value.

I really wish this proposed API included a way to expose data into JS without copying it into a new buffer. Memory is still a precious commodity in some circumstances, so here's my proposal accomplishing this with the following additions:

typedef void (*JSDataFinalizeCallback)(void*);

JSDataRef JSDataCreateWithBytes(void*, size_t, _Nullable JSDataFinalizeCallback);

JSObjectRef JSObjectMakeTypedArrayWithData(JSContextRef, JSTypedArrayType, JSDataRef, JSValueRef* exception);

A JSDataRefCF.h could also have some convenience functions for dealing with CFDataRef types to make Cocoa integration even easier (just like with CFStringRefCF.h).

[Dominic Szablewski]

(In reply to comment #38)
> I really wish this proposed API included a way to expose data into JS
> without copying it into a new buffer. Memory is still a precious commodity
> in some circumstances, so here's my proposal accomplishing this with the
> following additions:
> 
> typedef void (*JSDataFinalizeCallback)(void*);
> 
> JSDataRef JSDataCreateWithBytes(void*, size_t, _Nullable
> JSDataFinalizeCallback);
> 
> JSObjectRef JSObjectMakeTypedArrayWithData(JSContextRef, JSTypedArrayType,
> JSDataRef, JSValueRef* exception);
> 
> A JSDataRefCF.h could also have some convenience functions for dealing with
> CFDataRef types to make Cocoa integration even easier (just like with
> CFStringRefCF.h).

I like this proposal, but I think it would be ok to implement it subsequently.

Imho it's important to get the basic API implemented sooner rather than later. It's been 3 years now since I started to fork JSC because of the missing Typed Array API. Getting each new version of JSC to compile on iOS/tvOS is a terrifying, herculean task, as there are no targets for these platforms in the project files. As a result, the JSC fork lags behind significantly of what's available on current iOS versions - missing security fixes and performance improvements. I'm not even sure if JSC is still AppStore compatible with Apple's new Bitcode requirements (I doubt LLINT properly compiles to Bitcode?!). We NEED the Typed Array API in JSC to continue our project _at all_. </rant>



I created a patch of my implementation. The "svn-create-patch upload" script silently failed for me (and I'm probably missing a bunch of other steps!?), so I just uploaded the patch here in the hopes that anybody would take a look:

http://phoboslab.org/files/jsc-typed-array-api.patch


The patch includes the API implementation and header files as can be seen here:
https://github.com/phoboslab/JavaScriptCore-iOS/blob/new-typed-array-api/JavaScriptCore/API/JSTypedArray.cpp
https://github.com/phoboslab/JavaScriptCore-iOS/blob/new-typed-array-api/JavaScriptCore/API/JSTypedArray.h

It works for me, but I'm probably miles away from being able to contribute this API to JSC on my own. I'm struggling with the patch tools already and in general have no idea what I'm doing...

If anybody of the JSC contributors could take a look and possibly use my work to build upon, I'd be very grateful and owe you more than one beer!

[Sebastian Markbåge]

To be clear, we at Facebook strongly want this API as well. It would be hugely beneficial for our React Native apps and we would also like to see this prioritized.

However, the underlying issue here is that the normal APIs are unnecessarily over protective. The exclusive locks means that everything we do is slow with the API is slow and forces to put more and more stuff in JavaScript code because doing any work in native code adds too much overhead.

[Michael Dwan]

(In reply to comment #39)
> Imho it's important to get the basic API implemented sooner rather than
> later. It's been 3 years now since I started to fork JSC because of the
> missing Typed Array API. Getting each new version of JSC to compile on
> iOS/tvOS is a terrifying, herculean task, as there are no targets for these
> platforms in the project files. As a result, the JSC fork lags behind
> significantly of what's available on current iOS versions - missing security
> fixes and performance improvements. I'm not even sure if JSC is still
> AppStore compatible with Apple's new Bitcode requirements (I doubt LLINT
> properly compiles to Bitcode?!). We NEED the Typed Array API in JSC to
> continue our project _at all_. </rant>

I feel like this *really* needs to be emphasized. ANY TypedArray API at all is better than none. I understand the importance of doing it right in a project as big as this, but, frankly, having nothing makes the inclusion of these types radically less valuable. Our work also doesn't afford us the "convenience" of compiling a custom build of the JSC for iOS, and, as Dominic explained, even tackling that monster chore is no longer an option with the introduction of Apple's Bitcode requirements. 

Typed Arrays exist to facilitate communication with low-level processes, having no interface for that side of the exchange makes them more novelty than tool.

[Keith Miller]

Since it seems like this feature is heavily desired. I figured I would give it a shot. Picking up where Dominic left off I will attach a version of the C-API I would like to propose. I am also working on an Obj-C API but that is not quite done yet. I would appreciate any comments or suggestions.

[Keith Miller]

Created attachment 267304 [details]
[WIP] JSTypedArray.h

[Geoffrey Garen]

We're going to get a lot of pushback from confused iOS and Mac engineers who will want to know:

(a) Why didn't you provide an API for working with CFDataRef, the canonical way to do byte pointers on the platform?

(b) Why did you reinvent a slightly worse version of CFDataRef?

You can solve (a) by providing a built-in CFDataRef API, which builds on top of this API.

I'm not sure what the solution to (b) is. 

CoreFoundation is open source software, so I think it might be reasonable to solve (b) by just specifying this API in terms of CFDataRef. That would mean that clients could either use this API and build with CF, use our internal WTF::String API and version-lock themselves to a JavaScriptCore they build themselves, or not use this API but still use the rest of JavaScriptCore.

Analogously, we created our own string type in this API (JSStringRef) rather than using CFStringRef, and it has caused nothing but trouble.

>      @enum JSType

JSTypedArrayType

>     typedef struct OpaqueJSTypedArrayData* JSTypedArrayDataRef;

OpaqueJSTypedArrayBuffer and JSTypedArrayBufferRef

>     typedef void (*JSTypedArrayDataDestructor)(void*, unsigned);

Need to document whether the number is size in bytes or length.

This API is awkward to use if you're supplying data from a CFDataRef or any other structured object. Just getting back the void* won't tell you which CoreFoundation object to call CFRelease() on.

I guess we need buffer creation and destruction to offer a void* context pointer.

>    JS_EXPORT JSObjectRef JSObjectMakeTypedArrayFromData(JSContextRef ctx, JSTypedArrayType arrayType, JSTypedArrayDataRef data, size_t byteOffset, size_t length);

JSObjectMakeTypedArrayWithBuffer

"With" is canonical on the platform.

>    JS_EXPORT JSTypedArrayDataRef JSObjectGetRetainedTypedArrayData(JSContextRef ctx, JSObjectRef object);

JSObjectCopyTypedArrayBuffer

>    JS_EXPORT JSTypedArrayDataRef JSObjectMakeTypedArrayDataWithBytesNoCopy(JSContextRef ctx, void* bytes, size_t length, JSTypedArrayDataDestructor destructor);

JSTypedArrayBufferCreateWithBytesNoCopy

You need a way to create a buffer that does copy.

>    JS_EXPORT JSTypedArrayDataRef JSTypedArrayDataRetain(JSTypedArrayDataRef data);

JSTypedArrayBufferRetain

Should take a context argument.

>    JS_EXPORT void JSTypedArrayDataRelease(JSTypedArrayDataRef data);

JSTypedArrayBufferRelease

>    JS_EXPORT void* JSTypedArrayDataGetBytesPtr(JSTypedArrayDataRef data);

I think we want this API to be on the typed array and not on the typed array buffer.

There's clearly a use case for accessing a typed array's raw pointer and length. We do this in web audio, for example.

But it's less clear to me that you would benefit from tearing off a typed array's inner data and retaining it. This could cause you to make a mistake and write into a pointer that the typed array has long since abandoned.

I think it clarifies the API if the typed array buffer is a write-only API designed for providing a data source, and all reads happen through the typed array.

But perhaps there's a use case I haven't considered.
    
>    JS_EXPORT size_t JSTypedArrayDataGetLength(JSTypedArrayDataRef data);

Ditto.

Need to be clearer about byte length vs unit length.

[Scott Kyle]

(In reply to comment #43)

This mostly looks really great Keith. I only have a few comments...

> typedef struct OpaqueJSTypedArrayData* JSTypedArrayDataRef;

Why not not give this a more generic name like JSDataRef since it really should only encapsulate bytes, length, and an optional destructor? Right now it's only used for typed arrays, but who knows what the future holds. :)

> JS_EXPORT JSTypedArrayDataRef JSObjectMakeTypedArrayDataWithBytesNoCopy(JSContextRef ctx, void* bytes, size_t length, JSTypedArrayDataDestructor destructor);

Starting with "JSObject..." feels funny since it returns data. Also, I'm not sure I understand why this function takes a context. It seems like data should just encapsulate bytes (and destructor) in a similar way to JSStringRef, so in theory it shouldn't need a context for its creation. The rest of the functions that operate on data don't need a context, which makes a lot of sense to me.

[Scott Kyle]

(In reply to comment #44)
> (a) Why didn't you provide an API for working with CFDataRef, the canonical
> way to do byte pointers on the platform?

I figured that would follow in another header file (like JSStringRef) along with the Objective-C API being updated as well.

> CoreFoundation is open source software, so I think it might be reasonable to
> solve (b) by just specifying this API in terms of CFDataRef. That would mean
> that clients could either use this API and build with CF, use our internal
> WTF::String API and version-lock themselves to a JavaScriptCore they build
> themselves, or not use this API but still use the rest of JavaScriptCore.

That would be pretty unfortunate for a project like React Native to have to also include CoreFoundation. It doesn't seem worth it to add that whole dependency just for a simple data container that contains bytes, length, retain count, and an optional destructor.

> This API is awkward to use if you're supplying data from a CFDataRef or any
> other structured object. Just getting back the void* won't tell you which
> CoreFoundation object to call CFRelease() on.
> 
> I guess we need buffer creation and destruction to offer a void* context
> pointer.

I was originally thinking the caller could use a map to track additional information if needed, but a context pointer would be simpler.

> >    JS_EXPORT JSTypedArrayDataRef JSObjectGetRetainedTypedArrayData(JSContextRef ctx, JSObjectRef object);
> 
> JSObjectCopyTypedArrayBuffer

By "Copy", are you suggesting that the whole data buffer would in fact be copied? That's a big part of what we are trying to avoid, right?

> >    JS_EXPORT JSTypedArrayDataRef JSTypedArrayDataRetain(JSTypedArrayDataRef data);
> 
> JSTypedArrayBufferRetain
> 
> Should take a context argument.

I really honestly would prefer a simpler type (JSDataRef) like JSStringRef that isn't tied to a particular context. I'm curious what the rationale is for tying this to a context?

> >    JS_EXPORT void* JSTypedArrayDataGetBytesPtr(JSTypedArrayDataRef data);
> 
> I think we want this API to be on the typed array and not on the typed array
> buffer.

I don't agree since there should be a method on the typed array to access its data (without copying).

[Filip Pizlo]

(In reply to comment #44)
> We're going to get a lot of pushback from confused iOS and Mac engineers who
> will want to know:
> 
> (a) Why didn't you provide an API for working with CFDataRef, the canonical
> way to do byte pointers on the platform?
> 
> (b) Why did you reinvent a slightly worse version of CFDataRef?
> 
> You can solve (a) by providing a built-in CFDataRef API, which builds on top
> of this API.
> 
> I'm not sure what the solution to (b) is. 
> 
> CoreFoundation is open source software, so I think it might be reasonable to
> solve (b) by just specifying this API in terms of CFDataRef. That would mean
> that clients could either use this API and build with CF, use our internal
> WTF::String API and version-lock themselves to a JavaScriptCore they build
> themselves, or not use this API but still use the rest of JavaScriptCore.

Currently, JSC for GTK or EFL don't need CoreFoundation.  They probably won't add CoreFoundation as a dependency.

> 
> Analogously, we created our own string type in this API (JSStringRef) rather
> than using CFStringRef, and it has caused nothing but trouble.

I talked about this with Keith in person before he started the WIP.  My suggestion was to do this in a platform-dependent way:

On Darwin: use CFDataRef
Not on Darwin: provide a custom CFDataRef-like thing

Maybe Keith can elaborate, but I think that maybe his WIP only contains the second part so far and he just hasn't gotten around to the first part.

> 
> >      @enum JSType
> 
> JSTypedArrayType
> 
> >     typedef struct OpaqueJSTypedArrayData* JSTypedArrayDataRef;
> 
> OpaqueJSTypedArrayBuffer and JSTypedArrayBufferRef
> 
> >     typedef void (*JSTypedArrayDataDestructor)(void*, unsigned);
> 
> Need to document whether the number is size in bytes or length.
> 
> This API is awkward to use if you're supplying data from a CFDataRef or any
> other structured object. Just getting back the void* won't tell you which
> CoreFoundation object to call CFRelease() on.
> 
> I guess we need buffer creation and destruction to offer a void* context
> pointer.
> 
> >    JS_EXPORT JSObjectRef JSObjectMakeTypedArrayFromData(JSContextRef ctx, JSTypedArrayType arrayType, JSTypedArrayDataRef data, size_t byteOffset, size_t length);
> 
> JSObjectMakeTypedArrayWithBuffer
> 
> "With" is canonical on the platform.
> 
> >    JS_EXPORT JSTypedArrayDataRef JSObjectGetRetainedTypedArrayData(JSContextRef ctx, JSObjectRef object);
> 
> JSObjectCopyTypedArrayBuffer
> 
> >    JS_EXPORT JSTypedArrayDataRef JSObjectMakeTypedArrayDataWithBytesNoCopy(JSContextRef ctx, void* bytes, size_t length, JSTypedArrayDataDestructor destructor);
> 
> JSTypedArrayBufferCreateWithBytesNoCopy
> 
> You need a way to create a buffer that does copy.
> 
> >    JS_EXPORT JSTypedArrayDataRef JSTypedArrayDataRetain(JSTypedArrayDataRef data);
> 
> JSTypedArrayBufferRetain
> 
> Should take a context argument.
> 
> >    JS_EXPORT void JSTypedArrayDataRelease(JSTypedArrayDataRef data);
> 
> JSTypedArrayBufferRelease
> 
> >    JS_EXPORT void* JSTypedArrayDataGetBytesPtr(JSTypedArrayDataRef data);
> 
> I think we want this API to be on the typed array and not on the typed array
> buffer.
> 
> There's clearly a use case for accessing a typed array's raw pointer and
> length. We do this in web audio, for example.
> 
> But it's less clear to me that you would benefit from tearing off a typed
> array's inner data and retaining it. This could cause you to make a mistake
> and write into a pointer that the typed array has long since abandoned.
> 
> I think it clarifies the API if the typed array buffer is a write-only API
> designed for providing a data source, and all reads happen through the typed
> array.
> 
> But perhaps there's a use case I haven't considered.
>     
> >    JS_EXPORT size_t JSTypedArrayDataGetLength(JSTypedArrayDataRef data);
> 
> Ditto.
> 
> Need to be clearer about byte length vs unit length.

[Geoffrey Garen]

> >    JS_EXPORT JSTypedArrayDataRef JSTypedArrayDataRetain(JSTypedArrayDataRef data);
> 
> JSTypedArrayBufferRetain
> 
> Should take a context argument.

Didn't mean to say this. I take it back.

[Geoffrey Garen]

> > Analogously, we created our own string type in this API (JSStringRef) rather
> > than using CFStringRef, and it has caused nothing but trouble.
> 
> I talked about this with Keith in person before he started the WIP.  My
> suggestion was to do this in a platform-dependent way:
> 
> On Darwin: use CFDataRef
> Not on Darwin: provide a custom CFDataRef-like thing

This could work.

I think the right platform test is USE(CF) (as opposed to OS(DARWIN)).

I would suggest making the argument types CFMutableDataRef.

On non-CF platforms, we could either

(a) typedef CFMutableDataRef to be JSDataRef

or

(b) just continue calling it CFMutableDataRef, and provide an implementation of the functions we need.

I think (b) is preferable, since it's the clearest explanation of what we've done: We've made an API that works with CFMutableDataRef, and supplied a shim implementation for folks who choose not to build CoreFoundation.

So, I guess my proposal is something like this:

// JSObjectRef.h
#if !USE(CF)
// Provide CFMutableDataRef for platforms that don't have it.
#include <JavaScriptCore/CFMutableDataRefShim.h>
#endif

[Keith Miller]

(In reply to comment #47)
> (In reply to comment #44)
> > We're going to get a lot of pushback from confused iOS and Mac engineers who
> > will want to know:
> > 
> > (a) Why didn't you provide an API for working with CFDataRef, the canonical
> > way to do byte pointers on the platform?
> > 
> > (b) Why did you reinvent a slightly worse version of CFDataRef?
> > 
> > You can solve (a) by providing a built-in CFDataRef API, which builds on top
> > of this API.
> > 
> > I'm not sure what the solution to (b) is. 
> > 
> > CoreFoundation is open source software, so I think it might be reasonable to
> > solve (b) by just specifying this API in terms of CFDataRef. That would mean
> > that clients could either use this API and build with CF, use our internal
> > WTF::String API and version-lock themselves to a JavaScriptCore they build
> > themselves, or not use this API but still use the rest of JavaScriptCore.
> 
> Currently, JSC for GTK or EFL don't need CoreFoundation.  They probably
> won't add CoreFoundation as a dependency.
> 
> > 
> > Analogously, we created our own string type in this API (JSStringRef) rather
> > than using CFStringRef, and it has caused nothing but trouble.
> 
> I talked about this with Keith in person before he started the WIP.  My
> suggestion was to do this in a platform-dependent way:
> 
> On Darwin: use CFDataRef
> Not on Darwin: provide a custom CFDataRef-like thing
> 
> Maybe Keith can elaborate, but I think that maybe his WIP only contains the
> second part so far and he just hasn't gotten around to the first part.

This was my original intent.

> 
> > 
> > >      @enum JSType
> > 
> > JSTypedArrayType
> > 
> > >     typedef struct OpaqueJSTypedArrayData* JSTypedArrayDataRef;
> > 
> > OpaqueJSTypedArrayBuffer and JSTypedArrayBufferRef
> > 
> > >     typedef void (*JSTypedArrayDataDestructor)(void*, unsigned);
> > 
> > Need to document whether the number is size in bytes or length.
> > 
> > This API is awkward to use if you're supplying data from a CFDataRef or any
> > other structured object. Just getting back the void* won't tell you which
> > CoreFoundation object to call CFRelease() on.
> > 
> > I guess we need buffer creation and destruction to offer a void* context
> > pointer.

This shouldn't be an issue; there is a separate CFData API happening later.

> > 
> > >    JS_EXPORT JSObjectRef JSObjectMakeTypedArrayFromData(JSContextRef ctx, JSTypedArrayType arrayType, JSTypedArrayDataRef data, size_t byteOffset, size_t length);
> > 
> > JSObjectMakeTypedArrayWithBuffer
> > 
> > "With" is canonical on the platform.

Fixed.

> > 
> > >    JS_EXPORT JSTypedArrayDataRef JSObjectGetRetainedTypedArrayData(JSContextRef ctx, JSObjectRef object);
> > 
> > JSObjectCopyTypedArrayBuffer
> > 
> > >    JS_EXPORT JSTypedArrayDataRef JSObjectMakeTypedArrayDataWithBytesNoCopy(JSContextRef ctx, void* bytes, size_t length, JSTypedArrayDataDestructor destructor);
> > 
> > JSTypedArrayBufferCreateWithBytesNoCopy
> > 
> > You need a way to create a buffer that does copy.

That was an oversight. I agree I'm adding a JSTypedArrayDataCreateWithBytes method as well. Also, after looking at my C++ code the context was unnecessary will be removed.

> > 
> > >    JS_EXPORT JSTypedArrayDataRef JSTypedArrayDataRetain(JSTypedArrayDataRef data);
> > 
> > JSTypedArrayBufferRetain
> > 
> > Should take a context argument.
> > 
> > >    JS_EXPORT void JSTypedArrayDataRelease(JSTypedArrayDataRef data);
> > 
> > JSTypedArrayBufferRelease
> > 
> > >    JS_EXPORT void* JSTypedArrayDataGetBytesPtr(JSTypedArrayDataRef data);
> > 
> > I think we want this API to be on the typed array and not on the typed array
> > buffer.
> > 
> > There's clearly a use case for accessing a typed array's raw pointer and
> > length. We do this in web audio, for example.
> > 
> > But it's less clear to me that you would benefit from tearing off a typed
> > array's inner data and retaining it. This could cause you to make a mistake
> > and write into a pointer that the typed array has long since abandoned.
> > 
> > I think it clarifies the API if the typed array buffer is a write-only API
> > designed for providing a data source, and all reads happen through the typed
> > array.
> > 
> > But perhaps there's a use case I haven't considered.

I always thought about Typed Arrays as a specific type cast on a underlying c-style pointer. i.e. an instance of an Int32Array is just an interface that allows the JS programmer a clean way, in JavaScript, to reason about what type (e.g. int, float) they are viewing the underlying C-style buffer as. In C however, it seems overly cumbersome to be bound by the same object based interface rather than cast. I suppose we could add ways to view the buffer both directly and through the CFData/JSTypedArrayData but it seems like that complicates the API.

> >     
> > >    JS_EXPORT size_t JSTypedArrayDataGetLength(JSTypedArrayDataRef data);
> > 
> > Ditto.
> > 
> > Need to be clearer about byte length vs unit length.

This is a fair point this function should probably be JSTypedArrayDataGetByteLength.

[Zhao Wang]

Will the fix be back porting to iOS 8, or it will only be existing with new iOS version?

Thanks a lot for all the efforts to make this happen. Just want add that these APIs are much needed for our app too due to performance issue. We probably will not have resource for rolling out customized JSC. We are relying on this api to make things works with our app. 

Thank you.

[chase gray]

I just wanted to add my vote to getting this fixed. I had a need to send video data bytes to a webview and there isn't currently a good solution for doing that. Being able to write the bytes directly to a typed array in JavaScript would have solved my problem. 

Thanks!

[Patrick Fabrizius]

I too would like to raise a vote to prioritize this issue since it plays a significant role for graphics performance. There are plenty of hybrid frameworks that would/could benefit and a great deal of IOS applications.

[Filip Pizlo]

(In reply to comment #51)
> Will the fix be back porting to iOS 8, or it will only be existing with new
> iOS version?

Unfortunately we can't comment on that.  Generally, we don't comment on Apple releases in the WebKit open source forums. 

> 
> Thanks a lot for all the efforts to make this happen. Just want add that
> these APIs are much needed for our app too due to performance issue. We
> probably will not have resource for rolling out customized JSC. We are
> relying on this api to make things works with our app. 
> 
> Thank you.

I'm glad to hear that this will make a difference!

[Keith Miller]

Created attachment 268933 [details]
Patch

[Keith Miller]

It looks like the CFData integration is not going to work out. Here is a proposal for what I would recommend as the API. Let me know what you think.

[Keith Miller]

Created attachment 268935 [details]
Patch

[WebKit Commit Bot]

Attachment 268935 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSTypedArray.h:81:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:96:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:96:  The parameter name "destructor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:107:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:107:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:120:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:120:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:128:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:138:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:146:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:155:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:176:  The parameter name "destructor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:184:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:191:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:199:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:207:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 17 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 268985 [details]
Patch

[WebKit Commit Bot]

Attachment 268985 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSTypedArray.h:81:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:96:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:96:  The parameter name "destructor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:107:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:107:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:120:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:120:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:128:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:138:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:146:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:155:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:176:  The parameter name "destructor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:184:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:191:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:199:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:207:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 17 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 268985 [details]
Patch

Attachment 268985 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/691427

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html

[Build Bot]

Created attachment 268993 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268985 [details]
Patch

Attachment 268985 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/691432

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html

[Build Bot]

Created attachment 268994 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Keith Miller]

Created attachment 269520 [details]
Patch

[WebKit Commit Bot]

Attachment 269520 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSTypedArray.h:77:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:92:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:92:  The parameter name "destructor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:103:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:103:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:116:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:116:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:124:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:134:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:142:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:151:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:160:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:172:  The parameter name "destructor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:180:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:187:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:195:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:203:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 17 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 269612 [details]
Patch

[Keith Miller]

This should, hopefully, fix the build issues. I guess we build C files as C++ files on Windows...

[WebKit Commit Bot]

Attachment 269612 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSTypedArray.h:77:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:92:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:92:  The parameter name "destructor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:103:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:103:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:116:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:116:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:124:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:134:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:142:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:151:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:160:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:172:  The parameter name "destructor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:180:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:187:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:195:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:203:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 17 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 269612 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269612&action=review

> Source/JavaScriptCore/API/tests/testapi.c:1926
> +    // Check TypedArray.
> +    {
> +        // Test making a typedArray from scratch length.
> +        JSObjectRef typedArray = JSObjectMakeTypedArray(context, kJSTypedArrayTypeUint32Array, 10, NULL);
> +        JSTypedArrayDataRef data = JSObjectGetRetainedTypedArrayData(context, typedArray);
> +        unsigned *buffer = (unsigned*)JSTypedArrayDataGetBytesPtr(data);
> +
> +        ASSERT(JSObjectGetTypedArrayLength(typedArray) == 10);
> +
> +        // Test buffer is connected to typedArray.
> +        buffer[1] = 1;
> +        v = JSObjectGetPropertyAtIndex(context, typedArray, 1, NULL);
> +        assertEqualsAsNumber(v, 1);
> +
> +        // Test passing a buffer from a new array to an old array
> +        JSTypedArrayDataRetain(data);
> +        typedArray = JSObjectMakeTypedArrayFromBytesNoCopy(context, kJSTypedArrayTypeUint32Array, buffer, 40, releaseContext, data, NULL);
> +        buffer = (unsigned*)JSObjectGetTypedArrayBytesPtr(context, typedArray);
> +        ASSERT(buffer[1] == 1);
> +        buffer[1] = 20;
> +        ASSERT(((unsigned*)JSTypedArrayDataGetBytesPtr(data))[1] == 20);
> +
> +        // Check releaseContext is called when the object is GCed.
> +        typedArray = NULL;
> +        // Make a new Typed Array object and don't assign it to a local variable so it won't be on the stack when we garbage collect.
> +        JSObjectMakeTypedArrayFromBytesNoCopy(context, kJSTypedArrayTypeUint32Array, buffer, 40, releaseContext, data, NULL);
> +        JSSynchronousGarbageCollectForDebugging(context);
> +        ASSERT(didCallDestructor);
> +
> +        // Test constructing with data and the data returned are the same even with an offset.
> +        typedArray = JSObjectMakeTypedArrayFromDataWithOffset(context, kJSTypedArrayTypeUint32Array, data, 4, 9, NULL);
> +        assertEqualsAsNumber(JSObjectGetPropertyAtIndex(context, typedArray, 0, NULL), 20);
> +        ASSERT(data == JSObjectGetRetainedTypedArrayData(context, typedArray));
> +        ASSERT(JSTypedArrayDataGetByteLength(data) == JSObjectGetTypedArrayByteLength(typedArray));
> +        JSTypedArrayDataRelease(data);
> +
> +        // Test attempting to allocate an array too big for memory.
> +        JSValueRef exception = NULL;

There would be value to this test having full coverage over the API, including doing all typed array types.

[Keith Miller]

> There would be value to this test having full coverage over the API,
> including doing all typed array types.

That's fair. I will add tests.

[Geoffrey Garen]

Comment on attachment 269612 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269612&action=review

> Source/JavaScriptCore/API/JSTypedArray.cpp:44
> +inline JSTypedArrayType toKJSTypedArrayType(TypedArrayType type)

Let's call this toJSTypedArrayType. We don't usually include the "k" when talking about constant type names.

> Source/JavaScriptCore/API/JSTypedArray.cpp:99
> +static JSObject* createTypedArrayFromArrayBuffer(ExecState* exec, JSTypedArrayType type, RefPtr<ArrayBuffer>&& buffer, size_t offset, size_t length)

You can call this createTypedArray. The "from array buffer" detail is described by argument types.

> Source/JavaScriptCore/API/JSTypedArray.h:29
> +#ifndef JSTypedArray_h
> +#define JSTypedArray_h

You need to #include JSTypedArray.h in JavaScriptCore.h. It's a design point of Apple framework APIs that clients should only need to #include FrameworkName.h, and not lots of distinct headers. This is an easier way to program, and it also makes code more amenable to precompiled headers.

> Source/JavaScriptCore/API/JSTypedArray.h:38
> +    /*!

All this stuff should not be indented.

> Source/JavaScriptCore/API/JSTypedArray.h:39
> +     @enum JSType

Wrong type name here. Should be JSTypedArrayType.

> Source/JavaScriptCore/API/JSTypedArray.h:40
> +     @abstract     A constant identifying the Typed Array type of a JSObject.

JSObjectRef

> Source/JavaScriptCore/API/JSTypedArray.h:65
> +    typedef struct OpaqueJSTypedArrayData* JSTypedArrayDataRef;

Let's put this in JSBase.h so all our data types are together. Also, needs an @typedef headerdoc.

Is there a principled reason that you called this "data" instead of "buffer"? I thought we liked the name buffer, since that's what this data type is called in JavaScript APIs. I think you should rename this to JSTypedArrayBufferRef and OpaqueJSTypedArrayBuffer, unless there's a good reason not to.

> Source/JavaScriptCore/API/JSTypedArray.h:66
> +    typedef void (*JSTypedArrayDataDestructor)(void* bytes, void* destructorContext);

Let's call this JSTypedArrayBytesDeallocator. We want to match CF, and it's misleading to call this a typed array data destructor since typed array data is a data type in our API, and we do not call this function to destroy it.

> Source/JavaScriptCore/API/JSTypedArray.h:75
> +     @result             A JSObjectRef that is a Typed Array or NULL if there was an error. The backing store of the array will be zero filled.

I would say "a Typed Array with all elements set to zero", and remove the sentence about zero filling. Zero filling requires some kind of description of behavior over time, which is more complicated than what we want to say.

> Source/JavaScriptCore/API/JSTypedArray.h:87
> +     @param destructorContext  A pointer to some data that is desired when destructor is called.

A pointer to pass back to the destructor.

> Source/JavaScriptCore/API/JSTypedArray.h:92
> +    JS_EXPORT JSObjectRef JSObjectMakeTypedArrayFromBytesNoCopy(JSContextRef ctx, JSTypedArrayType arrayType, void* bytes, size_t byteLength, JSTypedArrayDataDestructor destructor, void* destructorContext, JSValueRef* exception);

Let's call this JSObjectMakeTypedArrayWithBytesNoCopy, to match CF.

> Source/JavaScriptCore/API/JSTypedArray.h:103
> +    JS_EXPORT JSObjectRef JSObjectMakeTypedArrayFromData(JSContextRef ctx, JSTypedArrayType arrayType, JSTypedArrayDataRef data, JSValueRef* exception);

From => With

> Source/JavaScriptCore/API/JSTypedArray.h:112
> +     @param length       The number of elements to include in the Typed Array. length should be less than or equal to (dataByteLength - byteOffset) / elementSize where dataByteLength is the number of bytes data's backing store points to and elementSize is the element size of arrayType.

You should explain what happens if this "should" is not met. If what happens is a JavaScript exception, the exception will speak for itself and you can remove this warning.

> Source/JavaScriptCore/API/JSTypedArray.h:116
> +    JS_EXPORT JSObjectRef JSObjectMakeTypedArrayFromDataWithOffset(JSContextRef ctx, JSTypedArrayType arrayType, JSTypedArrayDataRef data, size_t byteOffset, size_t length, JSValueRef* exception);

From...With => With...And

> Source/JavaScriptCore/API/JSTypedArray.h:128
> +     @abstract           Returns a temporary pointer to the backing store pointer used by a Typed Array object.

Returns a temporary pointer to the backing store of a Typed Array object.

> Source/JavaScriptCore/API/JSTypedArray.h:131
> +     @result             A pointer to the raw data buffer that serves as object's backing store.

You should mention what happens if the passed in object is not a typed array.

> Source/JavaScriptCore/API/JSTypedArray.h:132
> +     @discussion         Calling JSObjectGetTypedArrayBytesPtr is eqiuvalent to calling JSObjectGetRetainedTypedArrayData, JSTypedArrayDataGetBytesPtr, then JSTypedArrayDataRelease. Except, the backing store of object will not be neutered as long as object is alive.

This explanation is super confusing to non-VM-experts. I think you should just say that the pointer is temporary and it is not safe to use it across other calls to the JavaScriptCore API.

> Source/JavaScriptCore/API/JSTypedArray.h:155
> +     @abstract           Returns a retained JSTypedArrayDataRef that encapsulates a pointer to a Typed Array's data in memory

Need a period.

> Source/JavaScriptCore/API/JSTypedArray.h:158
> +     @result             A JSTypedArrayDataRef or NULL if the JSObjectRef is not a Typed Array. The return value is automatically retained and has to be released again with JSTypedArrayDataRelease.

You should just say "A JSTypedArrayDataRef with a +1 reference count or NULL if the JSObjectRef is not a Typed Array". Use JSTypedArrayDataRelease to release this reference."

> Source/JavaScriptCore/API/JSTypedArray.h:160
> +    JS_EXPORT JSTypedArrayDataRef JSObjectGetRetainedTypedArrayData(JSContextRef ctx, JSObjectRef object);

I would call this JSObjectRetainTypedArrayData. "GetRetained" sounds weird -- whereas retain usually returns the thing retained.

> Source/JavaScriptCore/API/JSTypedArray.h:172
> +    JS_EXPORT JSTypedArrayDataRef JSTypedArrayDataMakeFromBytesNoCopy(JSContextRef ctx, void* bytes, size_t byteLength, JSTypedArrayDataDestructor destructor, void* destructorContext);

From=>With

> Source/JavaScriptCore/API/JSTypedArray.h:193
> +     @result           A pointer to the raw data buffer that serves as data's backing store. The backing store will not be deallocated until the deallocator method is called, if provided. Otherwise the backing store will be deallocated when the data is deallocated.

I don't understand this paragraph.

[Keith Miller]

Comment on attachment 269612 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269612&action=review

I realized while adding tests that I didn't implement a byteOffset method for TypedArray objects so I added that as well. Patch will be up soon.

>> Source/JavaScriptCore/API/JSTypedArray.cpp:44
>> +inline JSTypedArrayType toKJSTypedArrayType(TypedArrayType type)
> 
> Let's call this toJSTypedArrayType. We don't usually include the "k" when talking about constant type names.

Fixed.

>> Source/JavaScriptCore/API/JSTypedArray.cpp:99
>> +static JSObject* createTypedArrayFromArrayBuffer(ExecState* exec, JSTypedArrayType type, RefPtr<ArrayBuffer>&& buffer, size_t offset, size_t length)
> 
> You can call this createTypedArray. The "from array buffer" detail is described by argument types.

Fixed.

>> Source/JavaScriptCore/API/JSTypedArray.h:29
>> +#define JSTypedArray_h
> 
> You need to #include JSTypedArray.h in JavaScriptCore.h. It's a design point of Apple framework APIs that clients should only need to #include FrameworkName.h, and not lots of distinct headers. This is an easier way to program, and it also makes code more amenable to precompiled headers.

Didn't know that. Fixed.

>> Source/JavaScriptCore/API/JSTypedArray.h:38
>> +    /*!
> 
> All this stuff should not be indented.

Fixed.

>> Source/JavaScriptCore/API/JSTypedArray.h:39
>> +     @enum JSType
> 
> Wrong type name here. Should be JSTypedArrayType.

Fixed.

>> Source/JavaScriptCore/API/JSTypedArray.h:40
>> +     @abstract     A constant identifying the Typed Array type of a JSObject.
> 
> JSObjectRef

Fixed.

>> Source/JavaScriptCore/API/JSTypedArray.h:65
>> +    typedef struct OpaqueJSTypedArrayData* JSTypedArrayDataRef;
> 
> Let's put this in JSBase.h so all our data types are together. Also, needs an @typedef headerdoc.
> 
> Is there a principled reason that you called this "data" instead of "buffer"? I thought we liked the name buffer, since that's what this data type is called in JavaScript APIs. I think you should rename this to JSTypedArrayBufferRef and OpaqueJSTypedArrayBuffer, unless there's a good reason not to.

I moved the typedefs to JSBase.h

I went with data because I felt that it would be confusing to use buffer. I expect if the backing store was called buffer then users would expect to be able to treat it like a JS ArrayBuffer object, which we do not provide them an API to do. Perhaps this is not an important enough reason or we should add such an API.

>> Source/JavaScriptCore/API/JSTypedArray.h:66
>> +    typedef void (*JSTypedArrayDataDestructor)(void* bytes, void* destructorContext);
> 
> Let's call this JSTypedArrayBytesDeallocator. We want to match CF, and it's misleading to call this a typed array data destructor since typed array data is a data type in our API, and we do not call this function to destroy it.

Fair enough, Fixed.

>> Source/JavaScriptCore/API/JSTypedArray.h:75
>> +     @result             A JSObjectRef that is a Typed Array or NULL if there was an error. The backing store of the array will be zero filled.
> 
> I would say "a Typed Array with all elements set to zero", and remove the sentence about zero filling. Zero filling requires some kind of description of behavior over time, which is more complicated than what we want to say.

I changed it to "@result             A JSObjectRef that is a Typed Array with all elements set to zero or NULL if there was an error."

>> Source/JavaScriptCore/API/JSTypedArray.h:87
>> +     @param destructorContext  A pointer to some data that is desired when destructor is called.
> 
> A pointer to pass back to the destructor.

Fixed but destructor => deallocator since we changed the name.

>> Source/JavaScriptCore/API/JSTypedArray.h:92
>> +    JS_EXPORT JSObjectRef JSObjectMakeTypedArrayFromBytesNoCopy(JSContextRef ctx, JSTypedArrayType arrayType, void* bytes, size_t byteLength, JSTypedArrayDataDestructor destructor, void* destructorContext, JSValueRef* exception);
> 
> Let's call this JSObjectMakeTypedArrayWithBytesNoCopy, to match CF.

Fixed.

>> Source/JavaScriptCore/API/JSTypedArray.h:103
>> +    JS_EXPORT JSObjectRef JSObjectMakeTypedArrayFromData(JSContextRef ctx, JSTypedArrayType arrayType, JSTypedArrayDataRef data, JSValueRef* exception);
> 
> From => With

Fixed.

>> Source/JavaScriptCore/API/JSTypedArray.h:112
>> +     @param length       The number of elements to include in the Typed Array. length should be less than or equal to (dataByteLength - byteOffset) / elementSize where dataByteLength is the number of bytes data's backing store points to and elementSize is the element size of arrayType.
> 
> You should explain what happens if this "should" is not met. If what happens is a JavaScript exception, the exception will speak for itself and you can remove this warning.

Fixed. It throws an exception so the warning has been removed.

>> Source/JavaScriptCore/API/JSTypedArray.h:116
>> +    JS_EXPORT JSObjectRef JSObjectMakeTypedArrayFromDataWithOffset(JSContextRef ctx, JSTypedArrayType arrayType, JSTypedArrayDataRef data, size_t byteOffset, size_t length, JSValueRef* exception);
> 
> From...With => With...And

Fixed.

>> Source/JavaScriptCore/API/JSTypedArray.h:128
>> +     @abstract           Returns a temporary pointer to the backing store pointer used by a Typed Array object.
> 
> Returns a temporary pointer to the backing store of a Typed Array object.

Fixed.

>> Source/JavaScriptCore/API/JSTypedArray.h:131
>> +     @result             A pointer to the raw data buffer that serves as object's backing store.
> 
> You should mention what happens if the passed in object is not a typed array.

Fixed. Changed to: "A pointer to the raw data buffer that serves as object's backing store or NULL if object is not a Typed Array object."

>> Source/JavaScriptCore/API/JSTypedArray.h:132
>> +     @discussion         Calling JSObjectGetTypedArrayBytesPtr is eqiuvalent to calling JSObjectGetRetainedTypedArrayData, JSTypedArrayDataGetBytesPtr, then JSTypedArrayDataRelease. Except, the backing store of object will not be neutered as long as object is alive.
> 
> This explanation is super confusing to non-VM-experts. I think you should just say that the pointer is temporary and it is not safe to use it across other calls to the JavaScriptCore API.

Fixed. Changed to: "The pointer returned by this function is temporary and is not guaranteed to remain valid across JavaScriptCore API calls."

>> Source/JavaScriptCore/API/JSTypedArray.h:155
>> +     @abstract           Returns a retained JSTypedArrayDataRef that encapsulates a pointer to a Typed Array's data in memory
> 
> Need a period.

Fixed.

>> Source/JavaScriptCore/API/JSTypedArray.h:158
>> +     @result             A JSTypedArrayDataRef or NULL if the JSObjectRef is not a Typed Array. The return value is automatically retained and has to be released again with JSTypedArrayDataRelease.
> 
> You should just say "A JSTypedArrayDataRef with a +1 reference count or NULL if the JSObjectRef is not a Typed Array". Use JSTypedArrayDataRelease to release this reference."

That comment seems a bit confusing since its not explicitly clear that the reference count is incremented and not the value one. I changed your comment to "A JSTypedArrayDataRef with an incremented reference count or NULL if the JSObjectRef is not a Typed Array". Use JSTypedArrayDataRelease to release this reference."

>> Source/JavaScriptCore/API/JSTypedArray.h:160
>> +    JS_EXPORT JSTypedArrayDataRef JSObjectGetRetainedTypedArrayData(JSContextRef ctx, JSObjectRef object);
> 
> I would call this JSObjectRetainTypedArrayData. "GetRetained" sounds weird -- whereas retain usually returns the thing retained.

Fixed.

>> Source/JavaScriptCore/API/JSTypedArray.h:172
>> +    JS_EXPORT JSTypedArrayDataRef JSTypedArrayDataMakeFromBytesNoCopy(JSContextRef ctx, void* bytes, size_t byteLength, JSTypedArrayDataDestructor destructor, void* destructorContext);
> 
> From=>With

Fixed

>> Source/JavaScriptCore/API/JSTypedArray.h:193
>> +     @result           A pointer to the raw data buffer that serves as data's backing store. The backing store will not be deallocated until the deallocator method is called, if provided. Otherwise the backing store will be deallocated when the data is deallocated.
> 
> I don't understand this paragraph.

I intended to say that the pointer returned by this function is valid provided: IsAlive(data) || (HasCustomDeallocator(data) && !HasCustomDeallocatorBeenCalled(data)). Perhaps it is clearer (although its a weaker statement) to say: "the pointer will remain valid while data is retained".

>> Source/JavaScriptCore/API/tests/testapi.c:1926
>> +        JSValueRef exception = NULL;
> 
> There would be value to this test having full coverage over the API, including doing all typed array types.

I have moved the tests to TypedArrayCTest.cpp. Also, I added tests for all the various API methods.

[Keith Miller]

Created attachment 271415 [details]
Patch

[Oliver Hunt]

Comment on attachment 267304 [details]
[WIP] JSTypedArray.h

Ugh, as a patch doesn't work :-/

[Oliver Hunt]

Comment on attachment 267304 [details]
[WIP] JSTypedArray.h


>    /*!
>     @function
>     @abstract           Creates a JavaScript Typed Array with the given number of elements.
>     @param ctx          The execution context to use.
>     @param arrayType    A value of type JSTypedArrayType identifying the type of array you want to create. If arrayType is kJSTypedArrayTypeNone then NULL will be returned.
>     @param length       The number of elements for the array.
>     @param byteOffset   The byte offset for the created Typed Array. This will be ignore if arrayType is kJSTypedArrayTypeArrayBuffer.
>     @param length       The number of elements to include in the Typed Array. This will be ignored if arrayType is kJSTypedArrayTypeArrayBuffer.
>     @result             A JSObjectRef that is a Typed Array or NULL if there was an error. The backing store of the Typed Array will be the same as the one in data.
>     */
>    JS_EXPORT JSObjectRef JSObjectMakeTypedArrayFromData(JSContextRef ctx, JSTypedArrayType arrayType, JSTypedArrayDataRef data, size_t byteOffset, size_t length);
>

Ok, this is unclear to me, are we copying the content of <data> -- I think this is because you're not documenting @param data.

>    /*!
>     @function
>     @abstract           Creates a JavaScript Typed Array with the given number of elements.
>     @param ctx          The execution context to use.
>     @param bytes        A pointer to the byte buffer to be used as the backing store of the Typed Array object.
>     @param length       The number of bytes in the buffer bytes.
>     @result             A JSTypedArrayDataRef whose backing store is the same as the one pointed to by bytes.
>     */
>    JS_EXPORT JSTypedArrayDataRef JSObjectMakeTypedArrayDataWithBytesNoCopy(JSContextRef ctx, void* bytes, size_t length, JSTypedArrayDataDestructor destructor);
>

Perhaps have a copying variant? or has that idea already being killed in another comment?

>    /*!
>     @function
>     @abstract         Returns a pointer to the data buffer that serves as the backing store for a JavaScript data object.
>     @param data       The JSTypedArrayData whose backing store you want to access.
>     @result           A pointer to the raw data buffer that serves as data's backing store. The backing store will not be deallocated until the deallocator method is called, if provided. Otherwise the backing store will be deallocated when the data is deallocated.
>     */
>    JS_EXPORT void* JSTypedArrayDataGetBytesPtr(JSTypedArrayDataRef data);
What was the pinning API for this? It seems odd to talk about the backing buffer lifetime as the standard API model for GetPtr APIs is that lifetime is tied to owning object.

[Oliver Hunt]

Ummm, the wonders of bugzilla mean i think that the last attachment upload got nuked. Because bugzilla is terrible.

[Keith Miller]

(In reply to comment #77)
> Ummm, the wonders of bugzilla mean i think that the last attachment upload
> got nuked. Because bugzilla is terrible.

Yeah, https://bugs.webkit.org/attachment.cgi?id=271415&action=review should roughly be the latest copy. I was dealing with a git bug and accidentally obsoleted it before I had finished rebasing.

[Keith Miller]

Created attachment 271433 [details]
Patch

[WebKit Commit Bot]

Attachment 271433 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSTypedArray.h:74:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:89:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:89:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:100:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:100:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:113:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:113:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:121:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:131:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:139:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:147:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:155:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:176:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:184:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:191:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:199:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:207:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 18 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

> >> Source/JavaScriptCore/API/JSTypedArray.h:158
> >> +     @result             A JSTypedArrayDataRef or NULL if the JSObjectRef is not a Typed Array. The return value is automatically retained and has to be released again with JSTypedArrayDataRelease.
> > 
> > You should just say "A JSTypedArrayDataRef with a +1 reference count or NULL if the JSObjectRef is not a Typed Array". Use JSTypedArrayDataRelease to release this reference."
> 
> That comment seems a bit confusing since its not explicitly clear that the
> reference count is incremented and not the value one. I changed your comment
> to "A JSTypedArrayDataRef with an incremented reference count or NULL if the
> JSObjectRef is not a Typed Array". Use JSTypedArrayDataRelease to release
> this reference."

How about this, for maximum clarity on both points:

@result a A JSTypedArrayDataRef or NULL if the JSObjectRef is not a Typed Array. The JSTypedArrayDataRef is returned with a +1 reference count. Use JSTypedArrayDataRelease to release this reference.

I like the phrase "+1 reference count" because the compiler uses phrasing like that when issuing reference counting warnings and errors.

> >> Source/JavaScriptCore/API/JSTypedArray.h:193
> >> +     @result           A pointer to the raw data buffer that serves as data's backing store. The backing store will not be deallocated until the deallocator method is called, if provided. Otherwise the backing store will be deallocated when the data is deallocated.
> > 
> > I don't understand this paragraph.
> 
> I intended to say that the pointer returned by this function is valid
> provided: IsAlive(data) || (HasCustomDeallocator(data) &&
> !HasCustomDeallocatorBeenCalled(data)). Perhaps it is clearer (although its
> a weaker statement) to say: "the pointer will remain valid while data is
> retained".

I think we should make the same comment we made in JSObjectGetTypedArrayBytesPtr: The pointer returned by this function is temporary and is not guaranteed to remain valid across JavaScriptCore API calls.

Knowing how to treat the pointer is probably more instructive than knowing the low-level rules for what will happen to it.

Also, it's confusing to document these functions differently when one is just a convenience for the other -- if I understand correctly. Alternatively, if there's some other feature you're trying to describe here, we should document it in both places and with more clarity. Let's try to describe what you *should* do with these APIs.

> > Is there a principled reason that you called this "data" instead of "buffer"? I thought we liked the name buffer, since that's what this data type is called in JavaScript APIs. I think you should rename this to JSTypedArrayBufferRef and OpaqueJSTypedArrayBuffer, unless there's a good reason not to.
> 
> I went with data because I felt that it would be confusing to use buffer. I
> expect if the backing store was called buffer then users would expect to be
> able to treat it like a JS ArrayBuffer object, which we do not provide them
> an API to do. Perhaps this is not an important enough reason or we should
> add such an API.

I think you should rename “raw data buffer” and “backing store” to “the bytes of a typed array buffer”. This makes us consistent with CFDataRef, and also frees up those other names in case we want to use them.

With regard to this object not being a true JS ArrayBuffer: Imagine a world where we offer JSObjectMakeArrayBuffer and/or JSObjectIsArrayBuffer. So, now you have a true JS ArrayBuffer in the API and this thing.

I think I would still call this thing JSArrayBuffer. The point of this API is that it is a C representation of an ArrayBuffer. The fact that it is a C representation, not usable as a JS object, is a quirk of the API, but the quirk is not made easier to deal with by calling it something else.

If we don't like this quirk, perhaps the API should be JSObjectMakeArrayBuffer instead of JSArrayBufferMake*, and it should be a JavaScript object all along. Are we sure we need the feature of reference counting this object separate from its JavaScript counterpart?

Side note: You need to change JSTypedArrayDataMake* to JSTypedArrayDataCreate*. Make is a term of art for GC objects. Create is a term of art for reference counted objects.

[Geoffrey Garen]

> I think I would still call this thing JSArrayBuffer.

Maybe JSArrayBufferData instead, to call out that it's a lower-level type than an array buffer.

But TypedArrayData seems weird, since typed arrays don't have data -- array buffers do.

[Keith Miller]

Created attachment 273304 [details]
Patch

[Keith Miller]

Created attachment 273326 [details]
Patch

[WebKit Commit Bot]

Attachment 273326 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSValueRef.h:188:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:49:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:64:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:64:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:75:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:88:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:99:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:109:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:119:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:129:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:139:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:155:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:165:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:175:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 14 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 273342 [details]
Patch

[Keith Miller]

Hopefully, that fixes the EFL build.

[WebKit Commit Bot]

Attachment 273342 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSValueRef.h:188:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:49:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:64:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:64:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:75:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:88:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:99:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:109:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:119:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:129:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:139:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:155:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:165:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:175:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 14 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 273342 [details]
Patch

r=me

Needs to go through API review before landing.

[Keith Miller]

Created attachment 273645 [details]
Patch

[WebKit Commit Bot]

Attachment 273645 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSValueRef.h:188:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:48:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:74:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:87:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:98:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:108:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:118:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:128:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:138:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:154:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:174:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 14 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 273648 [details]
Patch

[WebKit Commit Bot]

Attachment 273648 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSValueRef.h:188:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:48:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:74:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:87:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:98:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:108:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:118:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:128:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:138:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:154:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:174:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 14 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 273661 [details]
Patch

[WebKit Commit Bot]

Attachment 273661 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSValueRef.h:188:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:48:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:74:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:87:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:98:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:108:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:118:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:128:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:138:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:154:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:174:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 14 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Committed r197983: <http://trac.webkit.org/changeset/197983>

[Alexey Proskuryakov]

This broke the build: 

/Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/API/JSValueRef.h:83:20: error: expected '=' after '__NSi_10_12'
/Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/API/JSValueRef.h:188:112: error: expected '=' after '__NSi_10_12'

Aren't we supposed to use TBA?

[Keith Miller]

(In reply to comment #97)
> This broke the build: 
> 
> /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/API/
> JSValueRef.h:83:20: error: expected '=' after '__NSi_10_12'
> /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/API/
> JSValueRef.h:188:112: error: expected '=' after '__NSi_10_12'
> 
> Aren't we supposed to use TBA?

Fixing. I just tried to copy what previous JSC API patches used.

[Keith Miller]

(In reply to comment #98)
> (In reply to comment #97)
> > This broke the build: 
> > 
> > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/API/
> > JSValueRef.h:83:20: error: expected '=' after '__NSi_10_12'
> > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/API/
> > JSValueRef.h:188:112: error: expected '=' after '__NSi_10_12'
> > 
> > Aren't we supposed to use TBA?
> 
> Fixing. I just tried to copy what previous JSC API patches used.

in their initial commits.*

[David Kilzer (:ddkilzer)]

(In reply to comment #99)
> (In reply to comment #98)
> > (In reply to comment #97)
> > > This broke the build: 
> > > 
> > > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/API/
> > > JSValueRef.h:83:20: error: expected '=' after '__NSi_10_12'
> > > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/API/
> > > JSValueRef.h:188:112: error: expected '=' after '__NSi_10_12'
> > > 
> > > Aren't we supposed to use TBA?
> > 
> > Fixing. I just tried to copy what previous JSC API patches used.
> 
> in their initial commits.*

Committed r197985: <http://trac.webkit.org/changeset/197985>

Attempt to fix another build failure on internal bots here (apparently header includes changed in such a way as to cause this build failure):

Committed r197995: <http://trac.webkit.org/changeset/197995>