The default bug view has changed. See this FAQ.

JSAutoStructuredCloneBuffer shouldn't require a 'cx'

RESOLVED FIXED in mozilla8

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: luke, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

unspecified
mozilla8
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment)

17.17 KB, patch
jorendorff
: review+
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
(Reporter)

Comment 1

6 years ago
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?
Attachment #544840 - Flags: review?(jorendorff)
Attachment #544840 - Flags: review?(bent.mozilla)
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?
Attachment #544840 - Flags: review?(bent.mozilla) → review+
(Reporter)

Comment 3

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

Yeah, what's wrong with me :P
Attachment #544840 - Flags: review?(jorendorff) → review+
(Reporter)

Comment 4

6 years ago
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.)
Summary: remove remaining IDB structured-clone deserialization to the main thread → JSAutoStructuredCloneBuffer shouldn't require a 'cx'
(Reporter)

Comment 5

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/2e0fea2cbd9b
Whiteboard: [inbound]
(Reporter)

Comment 6

6 years ago
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
http://hg.mozilla.org/mozilla-central/rev/eaf738a0e1b3
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.