Closed Bug 553128 Opened 15 years ago Closed 15 years ago

Add better support for typed arrays in structured clone

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

Attachments

(1 file)

It would be nice if we had faster copying (or even copy-on-write) for typed arrays. This would make moving canvas data to and from workers much faster.
Attached patch Patch, v1 (deleted) — Splinter Review
Here's a first stab. I had to make some changes to jstypedarray.h to get the reparenting right for workers, sadly.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #436827 - Flags: review?(vladimir)
Attachment #436827 - Flags: review?(jst)
Attachment #436827 - Flags: review?(jst) → review+
Comment on attachment 436827 [details] [diff] [review] Patch, v1 - In CloneSimpleValues(): + // Typed array objects. + if (js_IsTypedArray(obj)) { [...] + return SetPropertyOnValueOrObject(cx, OBJECT_TO_JSVAL(newTypedArray), rval, + robj, rid); + } + + // ArrayBuffer objects. + if (js_IsTypedArray(obj)) { You mean js_IsArrayBuffer(obj) here, right? If not, this will never test true... r=jst with that fixed.
Comment on attachment 436827 [details] [diff] [review] Patch, v1 >diff --git a/content/base/src/nsContentUtils.cpp b/content/base/src/nsContentUtils.cpp >--- a/content/base/src/nsContentUtils.cpp >+++ b/content/base/src/nsContentUtils.cpp >@@ -185,16 +185,17 @@ static NS_DEFINE_CID(kXTFServiceCID, NS_ > #include "nsIConsoleService.h" > > #include "mozAutoDocUpdate.h" > #include "imgICache.h" > #include "jsinterp.h" > #include "jsarray.h" > #include "jsdate.h" > #include "jsregexp.h" >+#include "jstypedarray.h" > > const char kLoadAsData[] = "loadAsData"; > > static const char kJSStackContractID[] = "@mozilla.org/js/xpc/ContextStack;1"; > static NS_DEFINE_CID(kParserServiceCID, NS_PARSERSERVICE_CID); > static NS_DEFINE_CID(kCParserCID, NS_PARSER_CID); > > nsIDOMScriptObjectFactory *nsContentUtils::sDOMScriptObjectFactory = nsnull; >@@ -5647,17 +5648,40 @@ CloneSimpleValues(JSContext* cx, > JSObject* newRegExp = js_CloneRegExpObject(cx, obj, proto); > if (!newRegExp) { > return NS_ERROR_FAILURE; > } > return SetPropertyOnValueOrObject(cx, OBJECT_TO_JSVAL(newRegExp), rval, > robj, rid); > } > >- // ImageData is just a normal JSObject with some properties in our impl. >+ // Typed array objects. >+ if (js_IsTypedArray(obj)) { >+ js::TypedArray* src = js::TypedArray::fromJSObject(obj); >+ JSObject* newTypedArray = js_CreateTypedArrayWithArray(cx, src->type, obj); >+ if (!newTypedArray) { >+ return NS_ERROR_FAILURE; >+ } >+ return SetPropertyOnValueOrObject(cx, OBJECT_TO_JSVAL(newTypedArray), rval, >+ robj, rid); >+ } >+ >+ // ArrayBuffer objects. >+ if (js_IsTypedArray(obj)) { ^ IsArrayBuffer >+ js::ArrayBuffer* src = js::ArrayBuffer::fromJSObject(obj); >+ JSObject* newBuffer = js_CreateArrayBuffer(cx, src->byteLength); >+ if (!newBuffer) { >+ return NS_ERROR_FAILURE; >+ } >+ memcpy(js::ArrayBuffer::fromJSObject(newBuffer)->data, src->data, >+ src->byteLength); >+ return SetPropertyOnValueOrObject(cx, OBJECT_TO_JSVAL(newBuffer), rval, >+ robj, rid); >+ } >+ > // Do we support File? > // Do we support Blob? > // Do we support FileList? > > *wasCloned = PR_FALSE; > return NS_OK; > } > >diff --git a/js/src/jstypedarray.cpp b/js/src/jstypedarray.cpp >--- a/js/src/jstypedarray.cpp >+++ b/js/src/jstypedarray.cpp >@@ -1439,8 +1439,39 @@ js_CreateTypedArrayWithBuffer(JSContext > js_NewNumberInRootedValue(cx, jsdouble(byteoffset), &vals[0]); > > if (!TypedArrayConstruct(cx, atype, argc, &vals[0], &vals[3])) > return NULL; > > return JSVAL_TO_OBJECT(vals[3]); > } > >+JS_FRIEND_API(JSBool) >+js_ReparentTypedArrayToScope(JSContext *cx, JSObject *obj, JSObject *scope) >+{ >+ scope = JS_GetGlobalForObject(cx, scope); >+ if (!scope) >+ return JS_FALSE; >+ >+ if (!js_IsTypedArray(obj)) >+ return JS_FALSE; >+ >+ TypedArray *typedArray = TypedArray::fromJSObject(obj); >+ >+ JSObject *buffer = typedArray->bufferJS; >+ JS_ASSERT(js_IsArrayBuffer(buffer)); >+ >+ JSObject *proto; >+ JSProtoKey key = >+ JSCLASS_CACHED_PROTO_KEY(&TypedArray::slowClasses[typedArray->type]); >+ if (!js_GetClassPrototype(cx, scope, key, &proto) || >+ !JS_SetPrototype(cx, obj, proto) || >+ !JS_SetParent(cx, obj, scope)) >+ return JS_FALSE; >+ >+ key = JSCLASS_CACHED_PROTO_KEY(&ArrayBuffer::jsclass); >+ if (!js_GetClassPrototype(cx, scope, key, &proto) || >+ !JS_SetPrototype(cx, buffer, proto) || >+ !JS_SetParent(cx, buffer, scope)) >+ return JS_FALSE; >+ >+ return JS_TRUE; >+} Hmm, I don't understand what this is doing, to be honest; what happens if there are multiple views of the same buffer and you reparent the scope of the buffer? Also, I'm not sure if the slowClasses/fastClasses usage is correct, since any "live" typed array object will have a fastClass, not a slowClass.
(In reply to comment #2) > You mean js_IsArrayBuffer(obj) here, right? Indeed! Apparently the slow fallback code works well ;) (In reply to comment #3) > what happens if there > are multiple views of the same buffer and you reparent the scope of the buffer? Well, that shouldn't be possible in the cloning case... We explicitly copy the old ArrayBuffer so there shouldn't be any way for another caller to get a view of it. Calling this reparent function outside the cloning process could be bad, though... I don't know what to do about that other than document when it's safe and when it's not safe to call. What do you think? > Also, I'm not sure if the slowClasses/fastClasses usage is correct, since any > "live" typed array object will have a fastClass, not a slowClass. Right, but that isn't the issue. The problem is that I need to get the new scope's prototype for Int32Array and others. The fast classes don't have such a thing, they use the slow classes' prototype objects. So this determines the correct slow class prototype to use given an object with a fast class. Make sense? I'll make sure jorendorff agrees with this though.
Comment on attachment 436827 [details] [diff] [review] Patch, v1 Ah ok, makes sense, but please check with jorendorff :-) r=me with the IsArrayBuffer fix.
Attachment #436827 - Flags: review?(vladimir) → review+
Comment on attachment 436827 [details] [diff] [review] Patch, v1 r=me on the JS stuff. This is not a great API to have and we'll probably rewrite this stuff someday. But it should work fine for now. Please change the JS_Set{Prototype,Parent} calls to use the set{Proto,Parent} methods of JSObject, which are faster and can't fail.
Attachment #436827 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: