Closed Bug 1444956 Opened 2 years ago Closed 2 years ago

[BinAST] Implement <script src> with BinAST files

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: Yoric, Assigned: jonco)

References

(Blocks 3 open bugs)

Details

Attachments

(2 files, 4 obsolete files)

Tentatively setting this to P3 to remove this from triage. I think it'd be good to send an "Intent to implement" for this. I also commented on the issue mentioned in OP with regards to HTML Standard integration.
Priority: -- → P3
Anne: Good point.
We'll also want to pref this off, for an initial rollout.
I realize that we haven't decided of a pref name.

Let's say

dom.script.enable.application_javascript_binast: bool
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #4)
Bikeshedding: Most prefs like this seem to have .enabled at the end, and dom.script_loader already exists.  How about dom.script_loader.binast_encoding.enabled?
(In reply to Jon Coppeard (:jonco) from comment #5)
> (In reply to David Teller [:Yoric] (please use "needinfo") from comment #4)
> Bikeshedding: Most prefs like this seem to have .enabled at the end, and
> dom.script_loader already exists.  How about
> dom.script_loader.binast_encoding.enabled?

Sounds good.

Also, annevk managed to convince me that this should be https only.
Attached patch bug1444956-binast-decoding (obsolete) — Splinter Review
Here's an initial patch that does the following:
 - add a pref: dom.script_loader.binast_encoding.enabled
 - send "Accept: application/javascript-binast, */*" header when fetching scripts over https
 - check for "Content-Type: application/javascript-binast" header when receiving scripts
 - store received data as binary without decoding
 - pass it to dummy JS APIs to decode it
Assignee: nobody → jcoppeard
This looks good! I can pretty easily fill in those APIs. Agreed that we want to pref this off.

Excited to see the plan coming together.
Attached patch bug1444956-binast-decoding v2 (obsolete) — Splinter Review
Updated patch to integrate with new JS APIs to decode BinAST data:
 - code is either conditional on JS_BUILD_BINAST macro, or does nothing when that's not defined
 - WPT test successfully runs very simple BinAST script from the browser
Attachment #8970895 - Attachment is obsolete: true
Comment on attachment 8972366 [details] [diff] [review]
bug1444956-binast-decoding v2

Requesting feedback on this patch to add BinAST decoding support to the browser.

The protocol settled upon in the github issue above is that the browser prepends  the mime type application/javascript-binast to the Accept header in the request and if the server supports it it can reply with the BinAST version of the resource, setting the Content-Type appropriately.

BinAST is conditionally supported in the JS engine based on the JS_BUILD_BINAST define.

So far I've made predicates like ScriptLoader::BinASTEncodingEnabled and ScriptLoadRequest::IsBinASTSource be inline functions that just return false when this is not defined so that most that depends on this feature will be compiled out without needing to add ifdefs to everything.  This hasn't been entirely successful.

If you prefer I can just ifdef everything, which is kinda ugly but might be clearer.
Attachment #8972366 - Flags: feedback?(amarchesini)
Attached patch bug1444956-binast-decoding v3 (obsolete) — Splinter Review
Updated patch with minor changes and a test to exercise off-thread parsing.
Comment on attachment 8972366 [details] [diff] [review]
bug1444956-binast-decoding v2

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

it looks great!

::: dom/script/ScriptLoader.cpp
@@ +1202,5 @@
> +  static bool sBinASTEnabled = false;
> +  static bool sCachedPref = false;
> +  if (!sCachedPref) {
> +    sCachedPref = true;
> +    Preferences::AddBoolVarCache(&sBinASTEnabled,

Maybe here you can use DOMPrefs. Eventually we will expose this to workers and we must have this method able to be called on a worker-thread.

::: js/src/jsapi.cpp
@@ +4249,5 @@
>  }
>  
> +JS_PUBLIC_API(bool)
> +JS::DecodeBinASTOffThread(JSContext* cx, const ReadOnlyCompileOptions& options,
> +                      const uint8_t* buf, size_t length,

indentation
Attachment #8972366 - Flags: feedback?(amarchesini) → feedback+
Thanks!

Updated to use DOMPrefs and minor refactoring.
Attachment #8972366 - Attachment is obsolete: true
Attachment #8972624 - Attachment is obsolete: true
Attachment #8974663 - Flags: review?(amarchesini)
Attachment #8974663 - Flags: review?(amarchesini) → review+
Attached patch bug1444956-test (obsolete) — Splinter Review
Test code.
(In reply to Jon Coppeard (:jonco) from comment #14)
Any idea how I can land this test?  This depends on code that is #ifdef JS_BUILD_BINAST, which is non-trivially defined here:

https://searchfox.org/mozilla-central/source/js/moz.configure#453
Flags: needinfo?(james)
So, in testing/web-platform/mozilla/meta/binast/small.https.html.ini you want something like

[small.https.html]
  expected:
    if release_or_beta or os == "android" or (os == "win" and processor == "x86"): ERROR

I'm assuming the test will error if it's not supported. (Random aside: upstreaming these tests doesn't seem particularly worse than many other things that are in upstream, particularly if you put .tentative. in the name, so I would be tempted to do that rather than using the mozilla directory).
Flags: needinfo?(james)
If this lands, it should land with CanDecodeBinASTOffThread returning false, until we've sorted out the off-thread error reporting in BinASTParser. I'll have a patch tomorrow, but I don't want us to race.
Depends on: 1459555
Depends on: 1460805
Updated tests with expectations that match where BinAST is present.
Attachment #8974691 - Attachment is obsolete: true
Attachment #8975002 - Flags: review?(amarchesini)
Attachment #8975002 - Flags: review?(amarchesini) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0433be07dc5
Support BinAST decoding in the script loader r=baku
https://hg.mozilla.org/mozilla-central/rev/f0433be07dc5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #21)
That's unexpected.  Is there any way I can get more information about what's taking up the extra space?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.