Last Comment Bug 668915 - JSAutoStructuredCloneBuffer shouldn't require a 'cx'
: JSAutoStructuredCloneBuffer shouldn't require a 'cx'
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86_64 Linux
-- normal (vote)
: mozilla8
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
: Andrew Overholt [:overholt]
Depends on:
Blocks: 650411
  Show dependency treegraph
Reported: 2011-07-01 15:48 PDT by Luke Wagner [:luke]
Modified: 2011-07-29 03:11 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

remove 'cx' needed for JSAutoStructuredCloneBuffer::clear/adopt (17.17 KB, patch)
2011-07-08 09:57 PDT, Luke Wagner [:luke]
jorendorff: review+
bent.mozilla: review+
Details | Diff | Splinter Review

Description User image Luke Wagner [:luke] 2011-07-01 15:48:30 PDT
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 User image Luke Wagner [:luke] 2011-07-08 09:57:58 PDT
Created attachment 544840 [details] [diff] [review]
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 User image Ben Turner (not reading bugmail, use the needinfo flag!) 2011-07-08 11:48:00 PDT
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 @@
> +  return aBuffer.copy(reinterpret_cast<const uint64 *>(data), dataLength)
> +         ? NS_OK

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 User image Luke Wagner [:luke] 2011-07-08 13:41:04 PDT
(In reply to comment #2)
> Don't you want this to be JS_PUBLIC_API?

Yeah, what's wrong with me :P
Comment 4 User image Luke Wagner [:luke] 2011-07-22 10:53:46 PDT
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 7 User image Marco Bonardo [::mak] 2011-07-29 03:11:34 PDT

Note You need to log in before you can comment on or make changes to this bug.