668915 - JSAutoStructuredCloneBuffer shouldn't require a 'cx'

Status: RESOLVED FIXED

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

Description

[Luke Wagner [:luke]]

There are a few remaining off-main-thread JSAPI uses.

The biggest remaining use is GetKeyPathValueFromStructuredData which does a JS_ReadStructuredClone.  khuey has described a plan to store the key separately in the db entry so that it can be retrieved without deserializing the containing object.

Another use is GetStructuredCloneDataFromStatement which seems to just need a 'cx' to call JSAutoStructuredCloneBuffer member functions (which itself just needs a 'cx' to call JS_malloc/JS_free).  If that's all this is, we can just make change JSAutoStructuredCloneBuffer to not need a 'cx' (I can write that patch, just let me know if that's all this is).

The last one is ClearStructuredCloneBuffer; its in the same boat as GetStructuredCloneDataFromStatement.

Comment 1

[Luke Wagner [:luke]]

Attachment: remove 'cx' needed for JSAutoStructuredCloneBuffer::clear/adopt

This removes (the easy) 2 of the 3 cases.

The patch just uses non-cx malloc which saves considerable GetSafeJSContext grossness in a few cases.  This isn't in the JSAPI, so I out-of-lined JSAutoStructuredCloneBuffer members which I wouldn't think would matter for perf given that we just did a structured clone or are about to.  Jason, could you check the js changes and Ben, could you check the dom changes?

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 544840 [details] [diff] [review]
remove 'cx' needed for JSAutoStructuredCloneBuffer::clear/adopt

Review of attachment 544840 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the dom changes.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +840,5 @@
>    NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
>  
> +  return aBuffer.copy(reinterpret_cast<const uint64 *>(data), dataLength)
> +         ? NS_OK
> +         : NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;

Nit: ? and : on previous lines.

::: js/src/jsapi.h
@@ +3364,5 @@
>                     void *closure);
>  
>  #ifdef __cplusplus
>  /* RAII sugar for JS_WriteStructuredClone. */
> +class JS_FRIEND_API(JSAutoStructuredCloneBuffer) {

Don't you want this to be JS_PUBLIC_API?

Comment 3

[Luke Wagner [:luke]]

(In reply to comment #2)
> Don't you want this to be JS_PUBLIC_API?

Yeah, what's wrong with me :P

Comment 4

[Luke Wagner [:luke]]

One patch per bug, so I'm going to re-title this and spawn a new bug for the remaining work item.

(Will land this patch after new web workers sticks to avoid rebase pain for bent.)

Comment 5

[Luke Wagner [:luke]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2e0fea2cbd9b

Comment 6

[Luke Wagner [:luke]]

Backed out (b/c of bug 662852)
http://hg.mozilla.org/integration/mozilla-inbound/rev/ac34cb0bb085
Relanded with gross JSInt* types
http://hg.mozilla.org/integration/mozilla-inbound/rev/eaf738a0e1b3

Comment 7

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/eaf738a0e1b3