Closed
Bug 1409815
(binjs-bytecode)
Opened 7 years ago
Closed 6 years ago
[BinAST] Compile BinAST to bytecode
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: Yoric, Assigned: efaust)
References
Details
Attachments
(3 files, 1 obsolete file)
6.90 KB,
patch
|
Waldo
:
review+
Yoric
:
review+
jorendorff
:
review+
|
Details | Diff | Splinter Review |
14.20 KB,
patch
|
Waldo
:
review+
jorendorff
:
review+
|
Details | Diff | Splinter Review |
11.03 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•7 years ago
|
status-firefox58:
--- → fix-optional
Priority: -- → P3
Comment 1•7 years ago
|
||
https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals
status-firefox59:
--- → ?
Reporter | ||
Updated•7 years ago
|
Summary: Compile BinAST to bytecode → [BinAST] Compile BinAST to bytecode
Reporter | ||
Updated•7 years ago
|
Blocks: binjs-implement
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → efaustbmo
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8971745 -
Flags: review?(jwalden+bmo)
Attachment #8971745 -
Flags: review?(dteller)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8971746 -
Flags: review?(jwalden+bmo)
Attachment #8971746 -
Flags: review?(dteller)
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•6 years ago
|
||
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+
Reporter | ||
Comment 5•6 years ago
|
||
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+
Assignee | ||
Comment 6•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Attachment #8971745 -
Flags: review?(jorendorff)
Assignee | ||
Updated•6 years ago
|
Attachment #8972096 -
Flags: review?(jorendorff)
Comment 7•6 years ago
|
||
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 8•6 years ago
|
||
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 9•6 years ago
|
||
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 10•6 years ago
|
||
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+
Assignee | ||
Comment 11•6 years ago
|
||
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 12•6 years ago
|
||
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+
Comment 13•6 years ago
|
||
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)
Comment 14•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1c19a65b35f7 https://hg.mozilla.org/mozilla-central/rev/f7ad9a51e45b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 15•6 years ago
|
||
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)
Assignee | ||
Comment 16•6 years ago
|
||
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.
Description
•