Closed Bug 1409815 (binjs-bytecode) Opened 2 years ago Closed Last year

[BinAST] Compile BinAST to bytecode

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox58 --- wontfix
firefox59 --- ?
firefox61 --- fixed

People

(Reporter: Yoric, Assigned: efaust)

References

(Blocks 3 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

No description provided.
Priority: -- → P3
Summary: Compile BinAST to bytecode → [BinAST] Compile BinAST to bytecode
Assignee: nobody → efaustbmo
Depends on: 1451826
Attachment #8971745 - Flags: review?(jwalden+bmo)
Attachment #8971745 - Flags: review?(dteller)
Attachment #8971746 - Flags: review?(jwalden+bmo)
Attachment #8971746 - Flags: review?(dteller)
Status: NEW → ASSIGNED
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.
Attachment #8971745 - Flags: review?(dteller) → review+
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
Attachment #8971746 - Flags: review?(dteller) → feedback+
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.
Attachment #8971746 - Attachment is obsolete: true
Attachment #8971746 - Flags: review?(jwalden+bmo)
Attachment #8972096 - Flags: review?(jwalden+bmo)
Attachment #8971745 - Flags: review?(jorendorff)
Attachment #8972096 - Flags: review?(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.
Attachment #8971745 - Flags: review?(jorendorff) → review+
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.
Attachment #8972096 - Flags: review?(jorendorff) → review+
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.
Attachment #8971745 - Flags: review?(jwalden+bmo) → review+
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!
Attachment #8972096 - Flags: review?(jwalden+bmo) → review+
Don't factor out a new file. Instead, add functions to JSAPI that we'll need to browser integration anyways...
Attachment #8972181 - Flags: review?(jwalden+bmo)
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.
Attachment #8972181 - Flags: review?(jwalden+bmo) → review+
Pushed by efaustbmo@gmail.com:
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)
https://hg.mozilla.org/mozilla-central/rev/1c19a65b35f7
https://hg.mozilla.org/mozilla-central/rev/f7ad9a51e45b
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Blocks: 1454352
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?
Flags: needinfo?(efaustbmo)
As discussed on irc, this bug is all behind ifdefs, so it shouldn't have frightening vibes for release.
Flags: needinfo?(efaustbmo)
You need to log in before you can comment on or make changes to this bug.