[BinAST] Expose the APIs necessary for <script> loading

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: Yoric, Assigned: efaust)

Tracking

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

List provided by jonco:

JS::Compile

JS::CompileForNonSyntacticScope

JS::CompileOffThread   

JS::FinishOffThreadScript
efaust, it looks to me like these functions fall in your lap, right?
Flags: needinfo?(efaustbmo)
Assignee

Comment 2

Last year
Yeah, that sounds reasonable. I have enough for JS::Compile already as part of my musings. Off-thread stuff should be pretty easy, though I haven't looked into it yet. I'm imagining (read: assuming.) that most of the stuff we want to use from frontend:: uses the appropriate main thread checks already.
Assignee: nobody → efaustbmo
Flags: needinfo?(efaustbmo)
Priority: -- → P2
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #0)
> JS::CompileForNonSyntacticScope

I don't think we need this one - although it's called by the generic nsJSUtils class it's not used by the script loader so we can just assert that this case doesn't arise.
Assignee

Comment 4

Last year
Posted patch offThreadParse.patch (obsolete) — Splinter Review
This looks right to me, and would provide the other two, but it seems like it must be supposed to be more complicated. Does this look plausible, Jon?
Attachment #8972472 - Flags: feedback?(jcoppeard)
Assignee

Updated

Last year
Status: NEW → ASSIGNED
Comment on attachment 8972472 [details] [diff] [review]
offThreadParse.patch

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

Yes this looks fine.  I'm still working hooking up a test to use this path.  I'll let you know if things don't work out.

Can you add JS::CanDecodeBinASTOffThread() (along the same lines as the existing versions) and add a case for BinAST in CanDoOffThread()?
Attachment #8972472 - Flags: feedback?(jcoppeard) → feedback+
(In reply to Jon Coppeard (:jonco) from comment #5)
Testing was successful.
I combined the patches and rebased.
Attachment #8972472 - Attachment is obsolete: true
Attachment #8973061 - Attachment is obsolete: true
Comment on attachment 8974653 [details] [diff] [review]
off-thread-decode-binast

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

LGTM.

::: js/src/frontend/BytecodeCompiler.h
@@ +41,5 @@
>  JSScript*
>  CompileGlobalBinASTScript(JSContext *cx, LifoAlloc& alloc,
>                            const ReadOnlyCompileOptions& options,
> +                          const uint8_t* src, size_t len,
> +                          ScriptSourceObject** sourceObjectOut = nullptr);

The last parameter should probably be a |RootedScriptSourceObject*|.  If it wasn't optional you could use a MutableHandle, but I don't think we have a way of making that optional.
Attachment #8974653 - Flags: review+
(In reply to Jon Coppeard (:jonco) from comment #9)
> The last parameter should probably be a |RootedScriptSourceObject*|.  If it
> wasn't optional you could use a MutableHandle, but I don't think we have a
> way of making that optional.

Oh we already do this in a bunch of places.  Never mind, I'll file a separate bug.

Comment 11

Last year
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91ced8101f99
Add APIs for parallel decoding of BinAST data r=jonco

Comment 12

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/91ced8101f99
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.