1444956 - [BinAST] Implement <script src> with BinAST files

Status: RESOLVED FIXED

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

Description

[David Teller [:Yoric] - still alive but not very active]

Specifications are being discussed here: https://github.com/binast/ecmascript-binary-ast/issues/27 .

Comment 1

[Anne (:annevk)]

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.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Anne: Good point.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

We'll also want to pref this off, for an initial rollout.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

I realize that we haven't decided of a pref name.

Let's say

dom.script.enable.application_javascript_binast: bool

Comment 5

[Jon Coppeard (:jonco)]

(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?

Comment 6

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: bug1444956-binast-decoding

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

Comment 8

[Eric Faust [:efaust]]

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.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: bug1444956-binast-decoding v2

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

Comment 10

[Jon Coppeard (:jonco)]

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.

Comment 11

[Jon Coppeard (:jonco)]

Attachment: bug1444956-binast-decoding v3

Updated patch with minor changes and a test to exercise off-thread parsing.

Comment 12

[Andrea Marchesini [:baku]]

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

Comment 13

[Jon Coppeard (:jonco)]

Attachment: bug1444956-binast-decoding v4

Thanks!

Updated to use DOMPrefs and minor refactoring.

Comment 14

[Jon Coppeard (:jonco)]

Attachment: bug1444956-test

Test code.

Comment 15

[Jon Coppeard (:jonco)]

(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

Comment 16

[James Graham [:jgraham]]

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).

Comment 17

[Eric Faust [:efaust]]

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.

Comment 18

[Jon Coppeard (:jonco)]

Attachment: bug1444956-test v2

Updated tests with expectations that match where BinAST is present.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0433be07dc5
Support BinAST decoding in the script loader r=baku

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f0433be07dc5

Comment 21

[Eric Rahm [:erahm]]

It looks like this regressed libxul size by 142k:  https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-inbound,1299711,1,2&zoom=1526479790049.7925,1526493547192.946,129973880.48314336,130328358.09508367&selected=mozilla-inbound,1299711,337747,470208748,2

Comment 22

[Jon Coppeard (:jonco)]

(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?