1409815 - (binjs-bytecode) [BinAST] Compile BinAST to bytecode

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Comment 1

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 2

[Eric Faust [:efaust]]

Attachment: Part 1: Implement CompileGlobalBinASTScript

Comment 3

[Eric Faust [:efaust]]

Attachment: Part 2: As a convenience, allow running multipart binjs files from the shell cli

Comment 4

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

Comment on attachment 8971745 [details] [diff] [review]
Part 1: Implement CompileGlobalBinASTScript

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

r+ on everything except the BytecodeCompiler, which I don't know enough to review.

Comment 5

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

Comment on attachment 8971746 [details] [diff] [review]
Part 2: As a convenience, allow running multipart binjs files from the shell cli

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

::: js/src/shell/js.cpp
@@ +853,5 @@
> +           .setNoScriptRval(true);
> +
> +    Vector<uint8_t> fileContents(cx);
> +    for (;;) {
> +        int c = getc(file);

That looks unreasonably slow.

Any chance you could read the entire file, as we do in os.file.readFile? https://searchfox.org/mozilla-central/source/js/src/shell/OSObject.cpp#158

Comment 6

[Eric Faust [:efaust]]

Attachment: Part 2 (take 2): As a convenience, allow invoking BinAST from the shell

This is kinda doubling down against the previous review feedback (which was totally reasonable), but it's "no worse" than the other guy had it.

Use the identical mechanism to reading JS files from shell to read BinAST.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8971745 [details] [diff] [review]
Part 1: Implement CompileGlobalBinASTScript

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

It's disappointing that this complex setup must be repeated. I don't see a better way, but there's further annoyance ahead when you integrate this with off-thread parsing. :-\

::: js/src/frontend/BytecodeCompiler.cpp
@@ +600,5 @@
> +
> +    auto parsed = parser.parse(src);
> +
> +    if (parsed.isErr())
> +        return nullptr;

Better:

    ParseNode* pn;
    JS_TRY_VAR_OR_RETURN_NULL(cx, pn, parser.parse(src));

This takes care of the error check, early return, and unwrap, and eliminates a temporary.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 8972096 [details] [diff] [review]
Part 2 (take 2): As a convenience, allow invoking BinAST from the shell

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

r=me with misgivings.

::: js/src/jsapi.cpp
@@ -3886,5 @@
> -}
> -
> -namespace {
> -
> -class AutoFile

This code is so bad that I hate to see it in a header file. It could lead to other people thinking it's OK to use it.

Consider leaving AutoFile where it is (since ReadCompleteFile does not need it).

@@ +4042,4 @@
>      if (!ReadCompleteFile(cx, fp, buffer))
>          return false;
>  
> +    return ::Compile(cx, options, reinterpret_cast<char*>(buffer.begin()), buffer.length(), script);

Explicitly deferring to Waldo on whether this reinterpret_cast is kosher.

@@ +4764,4 @@
>  
>      CompileOptions options(cx, optionsArg);
>      options.setFileAndLine(filename, 1);
> +    return Evaluate(cx, options, reinterpret_cast<char*>(buffer.begin()), buffer.length(), rval);

And here.

::: js/src/shell/js.cpp
@@ +848,5 @@
> +static MOZ_MUST_USE bool
> +RunBinAST(JSContext* cx, const char* filename, FILE* file)
> +{
> +    CompileOptions options(cx);
> +    options.setFileAndLine(filename, 0)

Is this right? Usually files start at line 1, and saying 0 instead has been kind of a classic mistake for us...

@@ +1313,5 @@
>                  return false;
> +            break;
> +#if defined(JS_BUILD_BINAST)
> +          case FileBinAST:
> +            if (!RunBinAST(cx, filename, file))

Consider plumbing this one through jsapi.h the same way as the others, which would eliminate the need for moving ReadCompleteFile. I would love to keep that private to jsapi.cpp.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8971745 [details] [diff] [review]
Part 1: Implement CompileGlobalBinASTScript

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

::: js/src/frontend/BytecodeCompiler.cpp
@@ +586,5 @@
> +    if (!usedNames.init())
> +        return nullptr;
> +
> +    RootedObject sourceObj(cx, CreateScriptSourceObject(cx, options));
> +

Blank lines between decl/fallible thing and check-of-fallibility is not SpiderMonkey style, even tho it seems to be ubiquitous in this function.

@@ +590,5 @@
> +
> +    if (!sourceObj)
> +        return nullptr;
> +
> +    RootedScript script(cx, JSScript::Create(cx, options, sourceObj, 0, src.length(), 0, src.length()));

I am going to pretend those arguments that I'd have thought only made sense in the actual-code context are the right ones.

::: js/src/frontend/BytecodeCompiler.h
@@ +40,5 @@
> +
> +JSScript*
> +CompileGlobalBinASTScript(JSContext *cx, LifoAlloc& alloc,
> +                          const ReadOnlyCompileOptions& options,
> +                          Vector<uint8_t> &src);

*/& by type names.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8972096 [details] [diff] [review]
Part 2 (take 2): As a convenience, allow invoking BinAST from the shell

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

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

mozilla::Span<uint8_t> might be nice for this.

::: js/src/jsapi.cpp
@@ +4042,4 @@
>      if (!ReadCompleteFile(cx, fp, buffer))
>          return false;
>  
> +    return ::Compile(cx, options, reinterpret_cast<char*>(buffer.begin()), buffer.length(), script);

You can reinterpret any C++ data at all as char or unsigned char, so this is kosher.  Frowny perhaps, but kosher.

Is it necessary that it be non-const?  Tacking on a const here and below would be nice.

::: js/src/util/ReadFile.h
@@ +12,5 @@
> +#include "js/Vector.h"
> +
> +namespace js {
> +
> +typedef Vector<uint8_t, 8, TempAllocPolicy> FileContents;

using!

Comment 11

[Eric Faust [:efaust]]

Attachment: Part 2 (take 3): Take Jason's feedback to heart. Diff from results of last patch.

Don't factor out a new file. Instead, add functions to JSAPI that we'll need to browser integration anyways...

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 8972181 [details] [diff] [review]
Part 2 (take 3): Take Jason's feedback to heart. Diff from results of last patch.

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

::: js/src/jsapi.cpp
@@ +4236,5 @@
> +    CHECK_REQUEST(cx);
> +
> +    script.set(frontend::CompileGlobalBinASTScript(cx, cx->tempLifoAlloc(), options, buf, length));
> +
> +    return script != nullptr;

Seems like this would be better returning its JSScript* directly.

::: js/src/jsapi.h
@@ +4238,5 @@
> +             FILE* file, JS::MutableHandleScript script);
> +
> +extern JS_PUBLIC_API(bool)
> +DecodeBinAST(JSContext* cx, const ReadOnlyCompileOptions& options,
> +             const uint8_t* buf, size_t length, JS::MutableHandleScript script);

IMO it would be good for these functions to be in js/public/BinAST.h rather than accreting onto jsapi.h.

::: js/src/shell/js.cpp
@@ +857,4 @@
>  
> +        if (!JS::DecodeBinAST(cx, options, file, &script))
> +            return false;
> +        MOZ_ASSERT(script);

This seems like a reasonably elegant formulation of the way you do the thing.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c19a65b35f7
Part 1: Implement CompileGlobalBinASTScript. (r=Waldo, r=Yoric, r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ad9a51e45b
Part 2: Allow running BinAST files from shell CLI. (r=Waldo)

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1c19a65b35f7
https://hg.mozilla.org/mozilla-central/rev/f7ad9a51e45b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This seems like a huge change to land during a code freeze. Is there any reason this can't wait until after next week's version bump?

Comment 16

[Eric Faust [:efaust]]

As discussed on irc, this bug is all behind ifdefs, so it shouldn't have frightening vibes for release.