Closed
Bug 1331092
Opened 7 years ago
Closed 7 years ago
Implement Async Iteration for web content on Nightly builds
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: shu, Assigned: arai)
References
()
Details
(Keywords: dev-doc-needed)
Attachments
(14 files, 16 obsolete files)
1.02 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
1.15 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
82.79 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
39.61 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
15.14 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
12.80 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
11.93 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
1.13 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
8.37 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
1.70 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
3.34 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
2.31 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
9.98 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
781 bytes,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
https://github.com/tc39/proposal-async-iteration is stage 3, let's get some implementation experience.
Updated•7 years ago
|
Component: JavaScript → JavaScript Engine
Product: Developer Documentation → Core
Updated•7 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 1•7 years ago
|
||
so far, here's the list of things to be defined: * the difference between async function and async generator, in JSScript/LazyScript flags (currently unwrapped async function is also star generator) * the difference between await and yield inside async generator (currently await is converted to yield) about the first one, I guess we should refactor GeneratorKind and FunctionAsyncKind. there will be 5 kind of functions: * normal function (sync function) * legacy generator * star generator * async function * async generator current GeneratorKind(2 bits) + FunctionAsyncKind(1 bit) have no space to express the difference between async function and async generator, as long as we keep using GeneratorKind==StarGenerator for async function. on the other hand, we have 3 bits to express those 5 kinds, so if we stop using current enum and moved to different enum, it can be expressed without consuming extra space. what I'm thinking is, using 2 bits for standard things: * normal function (sync function) * star generator * async function * async generator (or 1 bit for generator, and 1 bit for async, separately) and remaining 1 bit for legacy generator, so that we can free that 1 bit when we stop supporting legacy generator (not sure when it happens tho :P I haven't yet figured out what's the best way to express the difference between await and yield...
Assignee | ||
Comment 2•7 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #1) > I haven't yet figured out what's the best way to express the difference > between await and yield... we could add JSOP_AWAIT for await expression, and check current opcode to determine if it was yield or await.
Assignee | ||
Comment 3•7 years ago
|
||
in WIP patch, I'm using unmodified GeneratorKind and FunctionAsyncKind, but NotGenerator for async function and StarGenerator for async generator. also using JSOP_YIELD/JSOP_AWAIT difference for checking whether the generator returns from yield or await.
Assignee | ||
Comment 4•7 years ago
|
||
current WIP patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bb6bc86e9f612826523c05b0b23dc23d7c8cf0d TODO: * AsyncIteratorClose with JSTRY_ITERCLOSE note in for-await-of (we should do "await" in IteratorCloseForException...?) * add tests * so many bug fix :P
Assignee | ||
Comment 5•7 years ago
|
||
I'm about to use try-finally instead of try-note for AsyncIteratorClose, to embed JSOP_AWAIT into bytecode. bug 1322069 would be useful for it.
See Also: → 1322069
Assignee | ||
Comment 6•7 years ago
|
||
updated https://treeherder.mozilla.org/#/jobs?repo=try&revision=104c6bac68ee7c0c818087d886899d567cb1c000 now AsyncIteratorClose uses try-catch, instead of try-finally, since we should only handle throwing case there, and all other non-local-jump can be handled in NonLocalExitControl::prepareForNonLocalJump. also it's based on bug 1322069 to add multiple try-catch.
Assignee | ||
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=01163cd43c58adfe7adc5e0e002cdc9262ef2367 supported newly added opcodes in JIT, and also some fix. I'm about to look the best way for testing (want to avoid creating duplicated tests between jstests and test262...
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•7 years ago
|
||
test262 for AsyncIteration is tracked in https://github.com/tc39/proposal-async-iteration/issues/69 I'll post tests there.
Summary: Implement async iterators → Implement Async Iteration
Assignee | ||
Comment 9•7 years ago
|
||
I'll post preliminary patches as a first step. the whole patch series are in https://hg.mozilla.org/try/pushloghtml?changeset=c54e213af6d30a0d902f1be64c9fcd050e0bedd3 [Part 1 / Part 2] I'll remove isStarGenerator from Async Function and use isStarGenerator+isAsync for Async Generator, so that isStarGenerator means "*" in syntax, and isAsync means "async" in syntax. before: | isStarGenerator | isAsync | isLegacyGenerator -----------------+-----------------+---------+------------------- Function | false | false | false Star Generator | true | false | false Async Function | true | true | false Legacy Generator | false | false | true after: | isStarGenerator | isAsync | isLegacyGenerator -----------------+-----------------+---------+------------------- Function | false | false | false Star Generator | true | false | false Async Function | false | true | false Async Generator | true | true | false Legacy Generator | false | false | true then, several places needs to handle both {isStarGenerator,isAsync} cases, or all {isStarGenerator,isLegacyGenerator,isAsync} cases. so I think isGenerator is misleading for such purpose, and just enumerating them, or adding more specific/descriptive accessors (needsFinalYield, needsDotGeneratorName, etc) should be better. Part 1 removes isGenerator and replaces its consumer with equivalent conditions, and Part 2 removes isStarGenerator flag from Async Function, and handle isAsync==true case in some places.
Attachment #8832998 -
Flags: review?(shu)
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8832999 -
Flags: review?(shu)
Assignee | ||
Comment 11•7 years ago
|
||
[Part 3 / Part 4] In Async Generator, I need to check whether the result object {value,done} is returned from yield, await, or return. I'm about to use the opcode pointed by `yieldAndAwaitOffsets[yieldAndAwaitIndex()]` (renamed from `yieldOffsets[yieldIndex()]`), to see which operation was the last thing done in the function. (actually, there could be several more ways other than checking opcode itself, like, using dedicated object (with special clasp) for result object used by await, or store some flag in GeneratorObject and flip it in JSOP_YIELD/JSOP_AWAIT, etc..., but at least this patch seems to work)
Attachment #8833001 -
Flags: review?(shu)
Assignee | ||
Comment 12•7 years ago
|
||
Attachment #8833003 -
Flags: review?(shu)
Assignee | ||
Comment 13•7 years ago
|
||
[Part 5] Before adding Async Generator related things to Promise, renamed Async Function related things to say "AsyncFunction" explicitly.
Attachment #8833004 -
Flags: review?(shu)
Assignee | ||
Comment 14•7 years ago
|
||
[Part 6] Added native wrapper for GetInternalError/GetTypeError self-hosted functions, to simplify callsites. I'll use GetTypeError in later patches.
Attachment #8833005 -
Flags: review?(shu)
Assignee | ||
Comment 15•7 years ago
|
||
[Part 7] Just added Symbol.asyncIterator.
Attachment #8833006 -
Flags: review?(shu)
Assignee | ||
Comment 16•7 years ago
|
||
I guess I should've summarize the whole structure of async generator implementation before getting those parts reviewed. * it uses similar approach as async function, that is, 1 unwrapped function + 1 wrapped function. unwrapped function corresponds to scripted function that is compiled from the JS source code (as a star generator), and wrapped function is a native function that implements the interface, and calls unwrapped function when it gets called * async generator returns dedicated native object (AsyncGeneratorObject) that implements the interface, and operates on GeneratorObject returned by unwrapped function. * unwrapped function behaves almost same as star generator. yield and await behaves almost identically on that part. * after returned from StarGeneratorNext/StarGeneratorThrow/StarGeneratorReturn, it checks whether the last operation was yield/await/return, and does corresponding things (resolved already returned promise for yield/return, wait for the passed promise for await) let me know if I'm missing something that needs explanation.
Reporter | ||
Comment 17•7 years ago
|
||
Comment on attachment 8832998 [details] [diff] [review] Part 1: Remove {JSFunction,JSScript,LazyScript}.isGenerator() method. Review of attachment 8832998 [details] [diff] [review]: ----------------------------------------------------------------- Your comments are amazingly helpful, as always.
Attachment #8832998 -
Flags: review?(shu) → review+
Reporter | ||
Comment 18•7 years ago
|
||
Comment on attachment 8832999 [details] [diff] [review] Part 2: Stop using StarGegerator for async function. Review of attachment 8832999 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +8119,5 @@ > { > if (!updateSourceCoordNotes(pn->pn_pos.begin)) > return false; > > + bool needsResultObject = sc->isFunctionBox() && sc->asFunctionBox()->needsResultObject(); I'd prefer needsIteratorResult over needsResultObject. ::: js/src/frontend/Parser.cpp @@ +2543,2 @@ > ? JSFunction::INTERPRETED_LAMBDA > : JSFunction::INTERPRETED_LAMBDA_GENERATOR); We should change INTERPRETED_LAMBDA_GENERATOR be changed to INTERPRETED_LAMBDA_GENERATOR_OR_ASYNC to be more accurate.
Attachment #8832999 -
Flags: review?(shu) → review+
Reporter | ||
Comment 19•7 years ago
|
||
Comment on attachment 8833001 [details] [diff] [review] Part 3: Add JSOP_AWAIT and rename {yieldIndex,yieldOffset} to {yieldAndAwaitIndex,yieldAndAwaitOffset}. Review of attachment 8833001 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.h @@ +227,5 @@ > CGTryNoteList tryNoteList; /* list of emitted try notes */ > CGScopeNoteList scopeNoteList; /* list of emitted block scope notes */ > > /* > + * For each yield op, map the yield and await index (stored as bytecode Nit: for each yield or await op ::: js/src/jit/BaselineCompiler.cpp @@ +274,5 @@ > // The last entry in the last index found, and is used to avoid binary > // searches for the sought entry when queries are in linear order. > bytecodeMap[script->nTypeSets()] = 0; > > + baselineScript->copyYieldEntries(script, yieldAndAwaitOffsets_); Should rename copyYieldEntries as well. ::: js/src/vm/GeneratorObject.cpp @@ +75,3 @@ > > + if ((*pc == JSOP_YIELD || *pc == JSOP_AWAIT) && > + genObj->isClosing() && genObj->is<LegacyGeneratorObject>()) Does this condition need changing? Legacy generators shouldn't be able to emit JSOP_AWAIT. ::: js/src/vm/GeneratorObject.h @@ +138,3 @@ > } > bool isClosing() const { > + return getFixedSlot(YIELD_AND_AWAIT_INDEX_SLOT).toInt32() == YIELD_INDEX_CLOSING; For consistency, YIELD_AND_AWAIT_INDEX_RUNNING and YIELD_AND_AWAIT_INDEX_CLOSING. ::: js/src/vm/Opcodes.h @@ +2132,5 @@ > */ \ > macro(JSOP_DEBUGAFTERYIELD, 208, "debugafteryield", NULL, 1, 0, 0, JOF_BYTE) \ > + /* > + * Pops the generator and the return value 'rval1', stops interpretation and > + * returns 'rval1'. Pushes resolved value onto the stack. Stale comment. 'rval1' -> 'promise'
Attachment #8833001 -
Flags: review?(shu) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #8833003 -
Flags: review?(shu) → review+
Reporter | ||
Comment 20•7 years ago
|
||
Comment on attachment 8833004 [details] [diff] [review] Part 5: Rename AsyncFunction-related names in Promise.cpp to explicitly say Async Function. Review of attachment 8833004 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Promise.cpp @@ +820,5 @@ > RootedValue generatorVal(cx, resultPromise->getFixedSlot(PromiseSlot_AwaitGenerator)); > > int32_t handlerNum = int32_t(handlerVal.toNumber()); > + MOZ_ASSERT(handlerNum == PromiseHandlerAsyncFunctionAwaitFulfilled > + || handlerNum == PromiseHandlerAsyncFunctionAwaitRejected); Style nit: that's a weird style, put || on previous line while you're here please.
Attachment #8833004 -
Flags: review?(shu) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #8833005 -
Flags: review?(shu) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #8833006 -
Flags: review?(shu) → review+
Assignee | ||
Comment 21•7 years ago
|
||
here's remaining patches. all of them has no testcase now, but at least this passes test262 (trunk and PRs), I'm about to add testcases to test262. [[Part 8]] through this patch, many places do similar thing as async function, with minor difference. we could merge them into single function with parameter or template tho, I'd like to use different implementation as a first step. some notes: [parser] Removed the code to reject async generator syntax. [bytecode] emit JSOP_TOASYNCGEN instead of JSOP_TOASYNC, for async generator. that wraps unwapped function and pushes wrapped async generator. we could use same opcode and branch inside it, but adding dedicated opcode is easier to handle it in JIT. [objects] Added the following objects: * %AsyncIteratorPrototype% https://tc39.github.io/proposal-async-iteration/#sec-asynciteratorprototype * %AsyncGeneratorPrototype% https://tc39.github.io/proposal-async-iteration/#sec-properties-of-asyncgenerator-prototype * %AsyncGenerator% https://tc39.github.io/proposal-async-iteration/#sec-properties-of-asyncgeneratorfunction-prototype * %AsyncGeneratorFunction% https://tc39.github.io/proposal-async-iteration/#sec-properties-of-asyncgeneratorfunction [promise] new PromiseHandler for: * Async Iterator Value Unwrap Functions (PromiseHandlerAsyncIteratorValueUnwrapDone, PromiseHandlerAsyncIteratorValueUnwrapNotDone) https://tc39.github.io/proposal-async-iteration/#sec-async-iterator-value-unwrap-functions * Await Fulfilled Functions (PromiseHandlerAsyncGeneratorAwaitFulfilled) https://tc39.github.io/proposal-async-iteration/#await-fulfilled * Await Rejected Functions (PromiseHandlerAsyncGeneratorAwaitRejected) https://tc39.github.io/proposal-async-iteration/#await-rejected PromiseReactionRecord for async generator has ReactionRecordSlot_Generator slot, that contains AsyncGeneratorObject, to pass it to actual handler functions (AsyncGeneratorAwaitedFulfilled, AsyncGeneratorAwaitedRejected) [AsyncGeneratorResume] AsyncGeneratorResume is called to resume async generator, for following cases: * when await resolves * when await rejects * AsyncGeneratorResumeNext https://tc39.github.io/proposal-async-iteration/#sec-asyncgeneratorresumenext it resumes unwrapped function, and it returns with following 4 reasons: * yield * await * return * throw "throw" case is detected by abrupt completion (CallSelfHostedFunction() == false), other cases are detected by opcode pointed by PC (actually, previous opcode of PC that would point JSOP_DEBUGAFTERYIELD, for yield/await) [own prototype property of async generator] async generator instance has own prototype property, just like star generator. the property should be defined on wrapped function, so ResolveInterpretedFunctionPrototype now accepts native function, only if it's wrapped async generator, and defined object which prototype is %AsyncGeneratorPrototype% [queue of AsyncGeneratorObject] queue uses single slot, for single request or an array for multiple requests. > + // Queue is implemented in 2 ways. If only one request is queued ever, > + // request is stored directly to the slot. Once 2 requests are queued, an > + // array is created and requests are pushed into it, and the array is > + // stored to the slot. current implementation is not optimized for queue with multiple requests, since it just calls array functions (push, shift). we could optimize it with 2 arrays or some other way when it's necessary, but so far I'm not sure if it's common to enqueue multiple requests.
Attachment #8836282 -
Flags: review?(shu)
Assignee | ||
Comment 22•7 years ago
|
||
Assignee | ||
Comment 23•7 years ago
|
||
[bytecode] Added IteratorKind (Sync or Async) parameter to emitIteratorClose, to emit AsyncIteratorClose, that does await for values returned by iterator. https://tc39.github.io/proposal-async-iteration/#sec-asynciteratorclose also added "ParseNode* generator" parameter to use it while emitting JSOP_AWAIT. emitAsyncIterator is added separately from emitIterator, since it does so much different things than emitIterator. https://tc39.github.io/proposal-async-iteration/#sec-getiterator it emits JSOP_TOASYNCITER that does CreateAsyncFromSyncIterator. emitYieldStar is also changed to do await for values returned by iterator. https://tc39.github.io/proposal-async-iteration/#prod-YieldExpression [objects] Added the following object: * %AsyncFromSyncIteratorPrototype% https://tc39.github.io/proposal-async-iteration/#sec-%asyncfromsynciteratorprototype%-object
Attachment #8836284 -
Flags: review?(shu)
Assignee | ||
Comment 24•7 years ago
|
||
Assignee | ||
Comment 25•7 years ago
|
||
Added ForAwaitOfLoopControl, that handles AsyncIteratorClose, like ForOfLoopControl, but ForAwaitOfLoopControl uses try-catch, instead of try-note. since we need to await inside it, and it's hard to handle it in try-note. Added IteratorKind and `ParseNode* generator` parameter to emitIteratorNext, just like emitIteratorClose, to do await. Also, added CompletionKind to emitIteratorClose, to emit dedicated bytecode for AsyncIteratorClose with abrupt completion. kid2 of PNK_FOROF was unused, so this patch uses kid2 to store dotGenerator name node, to pass it to JSOP_AWAIT.
Attachment #8836286 -
Flags: review?(shu)
Reporter | ||
Comment 26•7 years ago
|
||
Comment on attachment 8836282 [details] [diff] [review] Part 8: Implement Async Generator except yield*. Review of attachment 8836282 [details] [diff] [review]: ----------------------------------------------------------------- Okay, I think I understand all of this now. It's a very faithful translation of the spec, and should be fun to optimize later. ::: js/src/builtin/Promise.cpp @@ +40,5 @@ > + PromiseHandlerAsyncGeneratorAwaitFulfilled, > + PromiseHandlerAsyncGeneratorAwaitRejected, > + > + // Async Iteration proposal 6.1.1.2.1. > + // Both for [[Done]] internal slot is true and false cases. This comment confused me. How about: Async iterator handlers take the resolved value and create new iterator objects. To do so it needs to forward whether the iterator is done. In spec, this is achieved via the [[Done]] internal slot. We enumerate both true and false cases here. @@ +860,5 @@ > } > > +static MOZ_MUST_USE bool > +AsyncGeneratorAwaitPromiseReactionJob(JSContext* cx, Handle<PromiseReactionRecord*> reaction, > + MutableHandleValue rval) Style nit: line up argument @@ +888,5 @@ > +} > + > +// ES 2017 draft 7.4.7. > +static JSObject* > +CreateIterResultObject(JSContext* cx, HandleValue value, bool done) There is already the strangely named and unused js::CreateItrResultObject in jsiter.{h,cpp}. Please rename that js::CreateIterResultObject, and update and use that instead. @@ +2282,5 @@ > > +// Async Iteration proposal 5.1 steps 2-9. > +MOZ_MUST_USE bool > +js::AsyncGeneratorAwait(JSContext* cx, Handle<AsyncGeneratorObject*> asyncGenObj, > + HandleValue value) I'm fine with keeping the await implementations separate between functions and generators for now if that makes your patches easier to maintain. Could you file a followup bug to refactor? ::: js/src/frontend/Parser.cpp @@ -8891,5 @@ > } > } > > - if (isAsync && isGenerator) { > - error(JSMSG_ASYNC_GENERATOR); JSMSG_ASYNC_GENERATOR shouldn't be used after this patch series, so please remove it. ::: js/src/jsfun.cpp @@ +306,5 @@ > > JSFunction* callerFun = &callerObj->as<JSFunction>(); > if (IsWrappedAsyncFunction(callerFun)) > callerFun = GetUnwrappedAsyncFunction(callerFun); > + if (IsWrappedAsyncGenerator(callerFun)) else if @@ +1637,5 @@ > + introductionType = "AsyncGenerator"; > + else > + introductionType = "AsyncFunction"; > + } > + else if (generatorKind != NotGenerator) { Style nit: } else if (...) { @@ +1829,5 @@ > +js::AsyncGeneratorConstructor(JSContext* cx, unsigned argc, Value* vp) > +{ > + CallArgs args = CallArgsFromVp(argc, vp); > + > + // Save the callee before its reset in FunctionConstructor(). Typo: it's ::: js/src/vm/AsyncIteration.cpp @@ +137,5 @@ > + > +// Async Iteration proposal 5.1.2 Await Rejected Functions. > +MOZ_MUST_USE bool > +js::AsyncGeneratorAwaitedRejected(JSContext* cx, Handle<AsyncGeneratorObject*> asyncGenObj, > + HandleValue reason) Style nit: alignment ::: js/src/vm/AsyncIteration.h @@ +125,5 @@ > + } > + void setSingleQueueRequest(AsyncGeneratorRequest* request) { > + setFixedSlot(Slot_QueueOrRequest, ObjectValue(*request)); > + } > + void resetSingleQueueRequest() { How about "clear" instead of "reset"?
Attachment #8836282 -
Flags: review?(shu) → review+
Reporter | ||
Comment 27•7 years ago
|
||
Comment on attachment 8836284 [details] [diff] [review] Part 10: Implement Async Generator yield*. Review of attachment 8836284 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Promise.cpp @@ +2281,5 @@ > } > > +// ES 2017 draft 7.4.2. > +static bool > +IteratorNext(JSContext* cx, HandleObject iter, HandleValue value, MutableHandleObject resultObj) Hm it's kind of weird that this function is here. I'm surprised we didn't have a C++ implementation of this already. @@ +2387,5 @@ > + > + // Steps 5-6. > + RootedObject resultObj(cx); > + if (completionKind == CompletionKind::Normal) { > + if (!IteratorNext(cx, iter, args.get(0), &resultObj)) IteratorNext isn't used anywhere except here, right? Could you inline most of it and factor out the ThrowCheckIsObject part? @@ +2441,5 @@ > + return AbruptRejectPromise(cx, args, resultPromise, nullptr); > + > + // Steps 10. > + if (!resultVal.isObject()) { > + MOZ_ALWAYS_FALSE(ThrowCheckIsObject(cx, CheckIsObjectKind::IteratorNext)); CheckIsObjectKind should differ depending on completionKind here. @@ +2464,5 @@ > + return AbruptRejectPromise(cx, args, resultPromise, nullptr); > + bool done = ToBoolean(doneVal); > + > + // Step 11. > + Rooted<PromiseObject*> promise(cx, CreatePromiseObjectWithoutResolutionFunctions(cx)); I don't quite understand in the spec why this promise is made with the iterator value unwrap functions, then resolved immediately. Is it just for ease of chaining into the result promise? Isn't this part all sync? ::: js/src/frontend/BytecodeEmitter.cpp @@ +5240,5 @@ > + if (!emitTree(generator)) // ... FTYPE FVALUE RESULT GEN > + return false; > + if (!emitYieldOp(JSOP_AWAIT)) // ... FTYPE FVALUE RESULT > + return false; > + } This await and the one below are for the yield* cases, right? I don't see an await for non-yield* AsyncIteratorClose. For AsyncIteratorClose, the spec says Await(innerResult.[[Value]]). First, is the spec wrong and is that supposed to be Await(innerResult), like for the .next case? Second, here's what I understand this snippet to be doing in pseudocode: innerResult = iter.return(value); newInnerResult = { value: innerResult, done: false }; innerResult = await newInnerResult; That doesn't seem right, or am I reading this wrong? @@ +6754,5 @@ > + return false; > + if (!emitCall(JSOP_CALLITER, 0)) // ITER > + return false; > + checkTypeSet(JSOP_CALLITER); > + if (!emitCheckIsObj(CheckIsObjectKind::GetIterator)) // ITER Should add GetAsyncIterator @@ +8428,5 @@ > + if (!emitTree(gen)) // ITER OLDRESULT RESULT GEN > + return false; > + if (!emitYieldOp(JSOP_AWAIT)) // ITER OLDRESULT RESULT > + return false; > + } Same question as above, this seems to be repackaging an iterator result object into a new iterator result and awaiting that instead of just awaiting the iterator result. @@ +8482,5 @@ > + if (!emitTree(gen)) // ITER RESULT RESULT GEN > + return false; > + if (!emitYieldOp(JSOP_AWAIT)) // ITER RESULT > + return false; > + } Ditto.
Attachment #8836284 -
Flags: review?(shu)
Reporter | ||
Comment 28•7 years ago
|
||
Comment on attachment 8836284 [details] [diff] [review] Part 10: Implement Async Generator yield*. Review of attachment 8836284 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +5240,5 @@ > + if (!emitTree(generator)) // ... FTYPE FVALUE RESULT GEN > + return false; > + if (!emitYieldOp(JSOP_AWAIT)) // ... FTYPE FVALUE RESULT > + return false; > + } Ah I just understood what's going on. Async iterators return Promises, and since we compile awaits as yields that expect the value to implement the IteratorResult interface, we need to wrap the returned Promise into a not-done IteratorResult. Could you please refactor this bit out to a method on BCE with a comment explaining what it's doing?
Attachment #8836284 -
Flags: review+
Reporter | ||
Comment 29•7 years ago
|
||
Comment on attachment 8836286 [details] [diff] [review] Part 12: Implement for-await-of. Review of attachment 8836286 [details] [diff] [review]: ----------------------------------------------------------------- I'm not happy with the extra try/catch stuff. Let me think on the approach for a bit. ::: js/src/frontend/BytecodeEmitter.cpp @@ +1912,5 @@ > + // // try-block. > + // try { > + // ... > + // if (...) { > + // // 1. process StatementKind::Finally Where is finally used? I don't see it. @@ +1938,5 @@ > + generator_(generator) > + { > + } > + > + bool emitBefore(BytecodeEmitter* bce) { The before/after naming isn't very clear. I'm not sure what would be better right now. @@ +1941,5 @@ > + > + bool emitBefore(BytecodeEmitter* bce) { > + tryCatch_.emplace(bce, TryEmitter::TryCatch, TryEmitter::DontUseRetVal); > + > + if (!tryCatch_.ref().emitTry()) if (!tryCatch_->emitTry()) Please change the other places where you use ref() as well. @@ +1956,5 @@ > + unsigned slotFromTop = bce->stackDepth - iterDepth_; > + if (!bce->emitDupAt(slotFromTop)) // ITER ... EXCEPTION ITER > + return false; > + IteratorKind kind = IteratorKind::Async; > + ParseNode* gen = generator(); Is this always just the name '.generator'? You could remove this and all other instances where you pass along a ParseNode and manually do something like the following where you need it: NameLocation loc = *locationOfNameBoundInFunctionScope(cx->names().dotGenerator); if (!emitGetNameAtLocation(cx->names().dotGenerator, loc)) return false; @@ +5284,5 @@ > return false; > + > + if (kind == IteratorKind::Async) { > + if (!emitToIteratorResult(false)) // ... RESULT > + return false; I'm confused by this for the same reason as the previous patch. How does wrapping it in a new { value, done } let you await? @@ +7130,5 @@ > + MOZ_ASSERT_IF(iterKind == IteratorKind::Async, sc->asFunctionBox()->isAsync()); > + > + ParseNode* generator = forOfHead->pn_kid2; > + MOZ_ASSERT_IF(iterKind == IteratorKind::Async, generator); > + MOZ_ASSERT_IF(iterKind == IteratorKind::Sync, !generator); Like above, it would be cleaner to manually emit .generator names instead of carrying around an extra ParseNode in for-of nodes.
Assignee | ||
Comment 30•7 years ago
|
||
Thank you for reviewing :) (In reply to Shu-yu Guo [:shu] from comment #27) > @@ +2464,5 @@ > > + return AbruptRejectPromise(cx, args, resultPromise, nullptr); > > + bool done = ToBoolean(doneVal); > > + > > + // Step 11. > > + Rooted<PromiseObject*> promise(cx, CreatePromiseObjectWithoutResolutionFunctions(cx)); > > I don't quite understand in the spec why this promise is made with the > iterator value unwrap functions, then resolved immediately. Is it just for > ease of chaining into the result promise? Isn't this part all sync? uh, I don't understand the reasoning or underlying requirement. I'll ask in #jslang. > ::: js/src/frontend/BytecodeEmitter.cpp > @@ +5240,5 @@ > > + if (!emitTree(generator)) // ... FTYPE FVALUE RESULT GEN > > + return false; > > + if (!emitYieldOp(JSOP_AWAIT)) // ... FTYPE FVALUE RESULT > > + return false; > > + } > > This await and the one below are for the yield* cases, right? I don't see an > await for non-yield* AsyncIteratorClose. non-yield* part is changed in Part 12. > For AsyncIteratorClose, the spec says Await(innerResult.[[Value]]). First, > is the spec wrong and is that supposed to be Await(innerResult), like for > the .next case? Second, here's what I understand this snippet to be doing in > pseudocode: > > innerResult = iter.return(value); > newInnerResult = { value: innerResult, done: false }; > innerResult = await newInnerResult; > > That doesn't seem right, or am I reading this wrong? (I think you've already understood. just for future reference) before this patch series, unwrapped function of Async Function was purely a star generator. "await" was implemented with JSOP_YIELD, and both "await" and "return creates result object. The result object's "done" property is used to determine if the generator returned from "await" or "return". Then, Part 3 added JSOP_AWAIT, and now we can detect how the generator returns, without using result object's "done" property: * from yield - GeneratorObject::isAfterYield() * from await - GeneratorObject::isAfterAwait() * from throw - abrupt completion * from return - other case so, basically we no more need result object for "await" and "return". I'm about to optimize them out in bug 1316098.
Assignee | ||
Comment 31•7 years ago
|
||
(In reply to Shu-yu Guo [:shu] from comment #29) > ::: js/src/frontend/BytecodeEmitter.cpp > @@ +1912,5 @@ > > + // // try-block. > > + // try { > > + // ... > > + // if (...) { > > + // // 1. process StatementKind::Finally > > Where is finally used? I don't see it. sorry, it was stale comment. it should say something like "leave try-catch block by popping stack value". > @@ +1938,5 @@ > > + generator_(generator) > > + { > > + } > > + > > + bool emitBefore(BytecodeEmitter* bce) { > > The before/after naming isn't very clear. I'm not sure what would be better > right now. how about emitBeginIteratorCloseTarget/emitEndIteratorCloseTarget ? > @@ +1956,5 @@ > > + unsigned slotFromTop = bce->stackDepth - iterDepth_; > > + if (!bce->emitDupAt(slotFromTop)) // ITER ... EXCEPTION ITER > > + return false; > > + IteratorKind kind = IteratorKind::Async; > > + ParseNode* gen = generator(); > > Is this always just the name '.generator'? You could remove this and all > other instances where you pass along a ParseNode and manually do something > like the following where you need it: > > NameLocation loc = > *locationOfNameBoundInFunctionScope(cx->names().dotGenerator); > if (!emitGetNameAtLocation(cx->names().dotGenerator, loc)) > return false; Yes, it's to emit '.generator' reference, just like TOK_YIELD and TOK_AWAIT. Does it apply for PNK_YIELD (that has dotGenerator name in pn_right) too?
Assignee | ||
Comment 32•7 years ago
|
||
apparently enclosing `x = next().value` part of `for (let x of y) { ... }` with try-catch cannot be done simply, if we need to support Ion. it breaks the following assertion: https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/js/src/jit/MIR.cpp#2601 with try-catch way, it generates something like following: let x = uninitialized; tmp = IteratorNext(); try { x = tmp; } catch (e) { IteratorClose(); throw e; } // for-of body, use v here looks like Ion assumes that `x` is uninitialized after the try-catch :/ since generator itself is not compatible with Ion, the issue doesn't happen with for-await-of tho, if we use try-catch for for-of, it hits the issue. I'll try investigating it.
Assignee | ||
Comment 33•7 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #30) > Thank you for reviewing :) > > (In reply to Shu-yu Guo [:shu] from comment #27) > > @@ +2464,5 @@ > > > + return AbruptRejectPromise(cx, args, resultPromise, nullptr); > > > + bool done = ToBoolean(doneVal); > > > + > > > + // Step 11. > > > + Rooted<PromiseObject*> promise(cx, CreatePromiseObjectWithoutResolutionFunctions(cx)); > > > > I don't quite understand in the spec why this promise is made with the > > iterator value unwrap functions, then resolved immediately. Is it just for > > ease of chaining into the result promise? Isn't this part all sync? > > uh, I don't understand the reasoning or underlying requirement. > I'll ask in #jslang. caitp told me that it's to accept thennable for yielded value: function* g() { yield a_promise_that_will_be_resolved_to(10); } ... for await (let x of g()) { // x becomes 10, instead of the promise above. }
Reporter | ||
Comment 34•7 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #31) > > > @@ +1938,5 @@ > > > + generator_(generator) > > > + { > > > + } > > > + > > > + bool emitBefore(BytecodeEmitter* bce) { > > > > The before/after naming isn't very clear. I'm not sure what would be better > > right now. > > how about emitBeginIteratorCloseTarget/emitEndIteratorCloseTarget ? > That's better, but I'm still not clear on what "target" means in this context.
Assignee | ||
Comment 35•7 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #32) > apparently enclosing `x = next().value` part of `for (let x of y) { ... }` > with try-catch cannot be done simply, if we need to support Ion. > > it breaks the following assertion: > > https://dxr.mozilla.org/mozilla-central/rev/ > 0eef1d5a39366059677c6d7944cfe8a97265a011/js/src/jit/MIR.cpp#2601 > > > with try-catch way, it generates something like following: > > let x = uninitialized; > tmp = IteratorNext(); > try { > x = tmp; > } catch (e) { > IteratorClose(); > throw e; > } > // for-of body, use v here > > > looks like Ion assumes that `x` is uninitialized after the try-catch :/ > > since generator itself is not compatible with Ion, the issue doesn't happen > with for-await-of tho, if we use try-catch for for-of, it hits the issue. > > > I'll try investigating it. this was just because I was using 2 try-catch, each for head and body (to maintain the different stack depth) we could tweak the stack to unify them. that will solve the duplicated catch block issue too.
Comment hidden (obsolete) |
Assignee | ||
Comment 37•7 years ago
|
||
changed ForOfLoopControl to use try-catch, and separated the patch try is running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a581cd8f2c7e3d1615fbb1287c4f0b622881fdc4
Assignee | ||
Comment 38•7 years ago
|
||
added some more test cases. the result varies for throwing case. 17.3% slower on worst case and 24.2% faster on best case, depending on where the code throws. I'm not sure if hot code throws so much in wild tho... code: // simple loop function A() { var A = []; for (var i = 0; i < 30000000; i++) A.push(i); var x = 0; for (var a of A) x += a; return x; } // loop with destructuring in head function B() { var A = []; for (var i = 0; i < 3000000; i++) A.push([i, i, i]); var x = 0; for (var [a, b, c] of A) x += a; return x; } // break function C() { var A = [1, 2, 3]; var x = 0; for (var i = 0; i < 10000000; i++) { for (var a of A) { x += a; if (a == 2) break; } } return x; } // throw after 1 loop function D() { var A = [1, 2, 3]; var x = 0; for (var i = 0; i < 1000000; i++) { try { for (var a of A) { x += a; if (a == 2) throw 10; } } catch (e) { } } return x; } // throw after many loop function E() { var A = []; for (var i = 0; i < 3000; i++) A.push([i, i, i]); var x = 0; for (i = 0; i < 200; i++) { try { for (var a of A) { x += a; if (a == 2900) throw 10; } } catch (e) { } } return x; } // throw in next function F() { var A = { [Symbol.iterator]() { return this; }, next() { throw 10; } }; var x = 0; for (var i = 0; i < 1000000; i++) { try { for (var a of A) { x += a; } } catch (e) { } } return x; } // throw in return function G() { var A = { [Symbol.iterator]() { return this; }, next() { return { value: 10, done: false }; }, return() { throw 10; }, }; var x = 0; for (var i = 0; i < 1000000; i++) { try { for (var a of A) { break; } } catch (e) { } } return x; } // throw in return getter function H() { var A = { [Symbol.iterator]() { return this; }, next() { return { value: 10, done: false }; }, get return() { throw 10; }, }; var x = 0; for (var i = 0; i < 1000000; i++) { try { for (var a of A) { break; } } catch (e) { } } return x; } result: | try-note [us] | try-catch [us] ---+---------------+---------------------- A | 734627 | 766428 (+ 4.3 %) B | 510020 | 512687 (+ 0.5 %) C | 683960 | 700039 (+ 2.4 %) D | 619942 | 727185 (+ 17.3 %) E | 950225 | 966191 (+ 1.7 %) F | 650356 | 513020 (- 21.1 %) G | 738028 | 726812 (- 1.5 %) H | 735558 | 557719 (- 24.2 %)
Assignee | ||
Comment 39•7 years ago
|
||
Changed ForOfLoopControl implementation to ForAwaitOfLoopControl's one that uses try-catch. here's difference from previous ForAwaitOfLoopControl: 1. for-head and for-body are now in single try-catch block 2. stack layout is changed to balance stack around loop and try-catch. now "result.value" is also pushed to the stack before the loop, and the depth of for-of loop is 3 instead of 2 3. ITER value on the stack is cleared to UNDEFINED before non-local jump, and catch block checks the ITER value, and skip IteratorClose if ITER is UNDEFINED 4. code in NonLocalExitControl::prepareForNonLocalJump is moved to ForOfLoopControl::emitPrepareForNonLocalJump, to reduce the duplication Also, some fix: 5. in BytecodeEmitter::emitIteratorClose with CompletionKind::Throw, check if the iter.return is callable, outside of try-catch for JSOP_CALL https://tc39.github.io/ecma262/#sec-iteratorclose 7.4.6 IteratorClose ( iterator, completion ) ... 3. Let return be ? GetMethod(iterator, "return"). 4. If return is undefined, return Completion(completion). 5. Let innerResult be Call(return, iterator, « »). 6. If completion.[[Type]] is throw, return Completion(completion). 7. If innerResult.[[Type]] is throw, return Completion(innerResult). in CompletionKind::Normal case, this was done in JSOP_CALL for Step 5, but we should check if callable in Step 3, and the failure there should throw immediately, ignoring completion.[[Type]]. 6. to implement above, added JSOP_CHECKISCALLABLE, that throws if the top of the stack value is not callable, internal implementation is just like JSOP_CHECKISOBJ and ThrowCheckIsObject. JIT support is in next patch 7. Also applied the same stack balance fix for BytecodeEmitter::emitComprehensionForOf to match to the depth of ForOfLoop 8. removed JSTRY_ITERCLOSE 9. moved CompletionKind to jsapi.h
Attachment #8838913 -
Flags: review?(shu)
Assignee | ||
Comment 40•7 years ago
|
||
Assignee | ||
Comment 41•7 years ago
|
||
Comment on attachment 8836286 [details] [diff] [review] Part 12: Implement for-await-of. clearing for now. this will become smaller since most part is now in Part 0.1
Attachment #8836286 -
Flags: review?(shu)
Assignee | ||
Updated•7 years ago
|
Attachment #8838913 -
Attachment description: 01-Bug_1331092___Part_0_1__Use_try_catch_fo.patch → Part 0.1: Use try-catch for IteratorClose in for-of.
Assignee | ||
Comment 42•7 years ago
|
||
The following ParseNode were BINARY and pn_right was Generator NAME or assignment (.generator = PNK_GENERATOR). * PNK_YIELD * PNK_YIELD_STAR * PNK_AWAIT now Generator NAME can be emitted without having the ParseNode, so changed them (except assignment case) to UNARY with the operand. also added PNK_INITIALYIELD, that is UNARY and holds assignment, separated from PNK_YIELD with JSOP_INITIALYIELD.
Attachment #8838916 -
Flags: review?(shu)
Assignee | ||
Comment 43•7 years ago
|
||
also skip the test that checks async generator syntax throws SyntaxError.
Attachment #8838917 -
Flags: review?(shu)
Assignee | ||
Comment 44•7 years ago
|
||
addressed review comments. (I guess I should've posted in original order... :P
Attachment #8836282 -
Attachment is obsolete: true
Attachment #8838918 -
Flags: review+
Assignee | ||
Comment 45•7 years ago
|
||
Attachment #8836284 -
Attachment is obsolete: true
Attachment #8838919 -
Flags: review+
Assignee | ||
Comment 46•7 years ago
|
||
Attachment #8836286 -
Attachment is obsolete: true
Assignee | ||
Comment 47•7 years ago
|
||
here's all updated patches, including some rebase. https://hg.mozilla.org/try/pushloghtml?changeset=ae156487ba2624053764babb6f7d7c48131470bb
Reporter | ||
Comment 48•7 years ago
|
||
Comment on attachment 8838913 [details] [diff] [review] Part 0.1: Use try-catch for IteratorClose in for-of. Review of attachment 8838913 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! One question: the current way this is emitted doesn't yet try to cut down on the # of catch blocks generated, like what we talked about on IRC, with more trynote tricks or with catch blocks jumping to a common catch block that has the IteratorClose code. Will that be done later? In the for-of bytecode there happens to be just one catch block anyways, so we're fine here. But this deduplication will be necessary for replacing JSTRY_DESTRUCTURING_ITERCLOSE with a similar approach, else the destructuring bytecode will get even more bloated. ::: js/src/frontend/BytecodeEmitter.cpp @@ +2053,5 @@ > + MOZ_ASSERT(slotFromTop == unsigned(bce->stackDepth - iterDepth_)); > + if (!bce->emitDupAt(slotFromTop)) // ITER ... EXCEPTION ITER > + return false; > + CompletionKind completionKind = CompletionKind::Throw; > + if (!emitIteratorClose(bce, completionKind)) // ITER ... EXCEPTION Just inline CompletionKind::Throw, since the stack comment already doesn't line up. @@ +2074,5 @@ > + CompletionKind completionKind = CompletionKind::Normal) { > + return bce->emitIteratorClose(allowSelfHosted_, completionKind); > + } > + > + bool emitPrepareForNonLocalJump(BytecodeEmitter* bce, bool isTarget = false) { No need to make isTarget have a default argument, passing in true/false explicitly with a /* isTarget = */ comment is clearer. @@ +2720,2 @@ > > // IteratorClose is handled specially in the exception unwinder. For Stale comment, we can delete this sentence now. @@ +5239,5 @@ > return false; > > + if (completionKind == CompletionKind::Throw) { > + // For throw case, we should check if the RET is callable outside of > + // the try-catch below. Great catch. Could you copy-paste your explanation from the bug comment into this comment here? Namely, that with a normal CompletionKind, where the error is thrown for the return method being non-callable is non-observable.
Attachment #8838913 -
Flags: review?(shu) → review+
Reporter | ||
Comment 49•7 years ago
|
||
Comment on attachment 8838916 [details] [diff] [review] Part 7.5: Add BytecodeEmitter::emitDotGenerator and make yield/await nodes unary. Review of attachment 8838916 [details] [diff] [review]: ----------------------------------------------------------------- Very nice. ::: js/src/frontend/BytecodeEmitter.cpp @@ +8274,5 @@ > return true; > } > > bool > +BytecodeEmitter::emitDotGenerator() Nit: rename this to emitGetDotGenerator.
Attachment #8838916 -
Flags: review?(shu) → review+
Reporter | ||
Comment 50•7 years ago
|
||
Comment on attachment 8838917 [details] [diff] [review] Part 8.1: Disable test262 test to check the non-existence of async generators. Review of attachment 8838917 [details] [diff] [review]: ----------------------------------------------------------------- Just a heads up, this will conflict with bug 1340583, where :anba changes how negative tests are handled (from using the -n suffix in the filename to error: in the reftest line, similar to jit-tests).
Attachment #8838917 -
Flags: review?(shu) → review+
Assignee | ||
Comment 51•7 years ago
|
||
Rebased. Now for-await-of uses ForOfLoopControl.
Attachment #8838920 -
Attachment is obsolete: true
Attachment #8839840 -
Flags: review?(shu)
Reporter | ||
Comment 52•7 years ago
|
||
Comment on attachment 8839840 [details] [diff] [review] Part 12: Implement for-await-of. Review of attachment 8839840 [details] [diff] [review]: ----------------------------------------------------------------- So much cleaner with a unified approach to IteratorClose. :) ::: js/src/frontend/BytecodeEmitter.cpp @@ +2020,3 @@ > public: > + ForOfLoopControl(BytecodeEmitter* bce, int32_t iterDepth, bool allowSelfHosted, > + IteratorKind iterKind) Nit: put iterKind before allowSelfHosted in the constructor and in the fields above. @@ +2077,5 @@ > } > > bool emitIteratorClose(BytecodeEmitter* bce, > CompletionKind completionKind = CompletionKind::Normal) { > + return bce->emitIteratorClose(allowSelfHosted_, iterKind_, completionKind); Nit: should've noticed this earlier; please put allowSelfHosted as the last argument to any function that needs it. @@ +6978,5 @@ > + unsigned iflags = forOfLoop->pn_iflags; > + IteratorKind iterKind = (iflags & JSITER_FORAWAITOF) > + ? IteratorKind::Async > + : IteratorKind::Sync; > + MOZ_ASSERT_IF(iterKind == IteratorKind::Async, sc->asFunctionBox()); Assuming it's a typo for sc->isFunctionBox(), no need for assert, since asFunctionBox() below will assert it.
Attachment #8839840 -
Flags: review?(shu) → review+
Assignee | ||
Comment 53•7 years ago
|
||
JSOP_CHECKISCALLABLE is added by Part 0.1. that does the following at once (the error message is customizable), just like JSOP_CHECKISOBJ: if (!IsCallable(obj)) ThrowTypeError(JSMSG_RETURN_NOT_CALLABLE) this opcode is used only in IteratorClose with abrupt completion (like when for-of loop body throws), so it won't be so much hot. those opcodes just calls CheckIsCallable VMFunction, nothing is inlined. we could optimize this more by reusing MIsCallable, but I'd like to go with simpler way as a first step.
Attachment #8838914 -
Attachment is obsolete: true
Attachment #8840143 -
Flags: review?(jdemooij)
Assignee | ||
Comment 54•7 years ago
|
||
JSOP_TOASYNCGEN does almost same thing as JSOP_TOASYNC, but calls js::WrapAsyncGenerator instead of js::WrapAsyncFunction.
Attachment #8836283 -
Attachment is obsolete: true
Attachment #8840146 -
Flags: review?(jdemooij)
Assignee | ||
Comment 55•7 years ago
|
||
JSOP_TOASYNCITER is added in Part 10, that does CreateAsyncFromSyncIterator(syncIterator) in step 3.b.iii there https://tc39.github.io/proposal-async-iteration/#sec-getiterator > iii. Return ? CreateAsyncFromSyncIterator(syncIterator). JSOP_TOASYNCITER just calls js::CreateAsyncFromSyncIterator, like other opcodes above.
Attachment #8836285 -
Attachment is obsolete: true
Attachment #8840152 -
Flags: review?(jdemooij)
Reporter | ||
Comment 56•7 years ago
|
||
arai, could you land just the ITERCLOSE to try-catch patches?
Reporter | ||
Comment 58•7 years ago
|
||
Comment on attachment 8840143 [details] [diff] [review] Part 0.2: Support JSOP_CHECKISCALLABLE in JIT. Review of attachment 8840143 [details] [diff] [review]: ----------------------------------------------------------------- Stealing since I'd like these parts landed before the rest of the async iter patches. We already have a boolean returning (instead of throwing) version called {M,L}IsCallable. Might not make sense to refactor the MIR/LIR because of the extra checkKind. We need to at least refactor the codegen, as LIsCallable has inlined the fast paths of the checks. ::: js/src/jit/VMFunctions.cpp @@ +1474,5 @@ > > +bool > +CheckIsCallable(JSContext* cx, HandleValue v, CheckIsCallableKind kind) > +{ > + if (!v.isObject() || !v.toObject().isCallable()) Please use !IsCallable(v)
Attachment #8840143 -
Flags: review?(jdemooij)
Assignee | ||
Comment 59•7 years ago
|
||
Comment on attachment 8838913 [details] [diff] [review] Part 0.1: Use try-catch for IteratorClose in for-of. moved to bug 1342553
Attachment #8838913 -
Attachment is obsolete: true
Assignee | ||
Comment 60•7 years ago
|
||
Comment on attachment 8840143 [details] [diff] [review] Part 0.2: Support JSOP_CHECKISCALLABLE in JIT. moved to bug 1342553
Attachment #8840143 -
Attachment is obsolete: true
Comment 61•7 years ago
|
||
Comment on attachment 8840146 [details] [diff] [review] Part 9: Support JSOP_TOASYNCGEN in JIT. Review of attachment 8840146 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay.
Attachment #8840146 -
Flags: review?(jdemooij) → review+
Updated•7 years ago
|
Attachment #8840152 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 62•7 years ago
|
||
To test non-release-or-beta-only test, I need to skip it on release-or-beta. added release_or_beta boolean variable to reftest condition.
Attachment #8841806 -
Flags: review?(jmaher)
Assignee | ||
Comment 63•7 years ago
|
||
Enabled async-generator tests on non-release-or-beta, except expression-yield-as-statement.js that our local copy is obsolete.
Attachment #8841808 -
Flags: review?(andrebargull)
Assignee | ||
Comment 64•7 years ago
|
||
also, supported new opcodes in decompiler (for stack dump) we could improve "await" decompilation after changing await to not-using result object.
Attachment #8841810 -
Flags: review?(shu)
Updated•7 years ago
|
Attachment #8841806 -
Flags: review?(jmaher) → review+
Comment 65•7 years ago
|
||
Comment on attachment 8841808 [details] [diff] [review] Part 8.3: Enable test262 tests for async generators. Review of attachment 8841808 [details] [diff] [review]: ----------------------------------------------------------------- If you need to exclude a complete test directory, you should also be able to use "skip-if(release_or_beta) include test262/language/expressions/async-generators/jstests.list".
Attachment #8841808 -
Flags: review?(andrebargull) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #8841810 -
Flags: review?(shu) → review+
Assignee | ||
Comment 66•7 years ago
|
||
Comment on attachment 8832998 [details] [diff] [review] Part 1: Remove {JSFunction,JSScript,LazyScript}.isGenerator() method. I'll land Part 1-6 and 7.5 in bug 1316098.
Attachment #8832998 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8832999 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8833001 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8833003 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8833004 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8833005 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8838916 -
Attachment is obsolete: true
Assignee | ||
Comment 67•7 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #66) > Comment on attachment 8832998 [details] [diff] [review] > Part 1: Remove {JSFunction,JSScript,LazyScript}.isGenerator() method. > > I'll land Part 1-6 and 7.5 in bug 1316098. wrong bug number :P it's bug 1343481
Assignee | ||
Comment 68•7 years ago
|
||
We could optimize away result object allocation for yield/return in async generator, just like await case (bug 1316098), since the result object is not exposed to script. I'll file another bug for it after landing this.
Assignee | ||
Comment 69•7 years ago
|
||
depends on the bug in the proposal (the first one there) https://github.com/tc39/proposal-async-iteration/issues/86 I implemented that part to immediately throw.
Assignee | ||
Comment 70•7 years ago
|
||
looks like fixed by https://github.com/tc39/proposal-async-iteration/pull/87. and now "done" property is accessed only once. the patch and testcase in test262 needs fix
Assignee | ||
Comment 71•7 years ago
|
||
another non-critical issue around property access order. https://github.com/tc39/proposal-async-iteration/issues/88
Assignee | ||
Comment 72•7 years ago
|
||
one more issue https://github.com/tc39/proposal-async-iteration/issues/89 (affects only spec steps, no effect for implementation)
Assignee | ||
Comment 73•7 years ago
|
||
Removed access for "done" property in AsyncGeneratorResume. step 10.a there is totally unnecessary now.
Attachment #8844829 -
Flags: review?(shu)
Assignee | ||
Comment 74•7 years ago
|
||
actually, that fix is not sufficient. we should perform |if received.[[Type]] is throw| path once more for abrupt case.
Assignee | ||
Comment 75•7 years ago
|
||
filed https://github.com/tc39/proposal-async-iteration/issues/90 my patch doesn't match to current proposal, but I think it's the proposal's bug, not mine ;)
Reporter | ||
Comment 76•7 years ago
|
||
Comment on attachment 8844829 [details] [diff] [review] Part 11 (previous Part 14) : Fix property access in yield* to follow the change in proposal. Review of attachment 8844829 [details] [diff] [review]: ----------------------------------------------------------------- Could just fold this one into the previous patches.
Attachment #8844829 -
Flags: review?(shu) → review+
Updated•7 years ago
|
Reporter | ||
Comment 77•7 years ago
|
||
arai, let's land what we have when you're back. I see your issue 90 has been resolved. :) I'm afraid of this bitrotting. Leo from test262 also said that having this in Nightly would help test262 test async iter to help find possible spec bugs. We can draft something to dev-platform that says what we have an initial implementation for testing purposes, but please do not use yet due to unsettled spec semantic issues.
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 78•7 years ago
|
||
okay, hopefully I could land them this weekend, after renumbering the spec steps in the comments
Assignee | ||
Comment 80•7 years ago
|
||
now testing again and I hit the assertion failure on cgc build, but it's not reproducible locally. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bddc92512e86e0edf0d71a3a93eccb0a784b3a4 may take some more time.
Assignee | ||
Comment 81•7 years ago
|
||
while waiting for bug 1350762, here's the change from https://github.com/tc39/proposal-async-iteration/pull/92 now yield* awaits on the last value (done==true)
Attachment #8851524 -
Flags: review?(shu)
Comment 82•7 years ago
|
||
Pushed by arai_a@mac.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbde2a580e0 Part 1: Add Symbol.asyncIterator. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/0705d5b86ad7 Part 2: Implement Async Generator except yield*. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6e6211270b Part 3: Disable test262 test to check the non-existence of async generators. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/61e0eb18ba8c Part 4: Add release_or_beta variable to reftest condition. r=jmaher https://hg.mozilla.org/integration/mozilla-inbound/rev/8edcc256976d Part 5: Enable test262 tests for async generators. r=anba https://hg.mozilla.org/integration/mozilla-inbound/rev/72750b6e9e2a Part 6: Support JSOP_TOASYNCGEN in JIT. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/1d39acbc0922 Part 7: Implement Async Generator yield*. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/831d289bb6cc Part 8: Support JSOP_TOASYNCITER in JIT. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/3701b28662ac Part 9: Implement for-await-of. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/9b0c51ce33e8 Part 10: Support JSOP_TOASYNCGEN, JSOP_TOASYNCITER, and JSOP_AWAIT in decompiler. r=shu
Assignee | ||
Comment 83•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbde2a580e0d846247703ce3903500e30b83236 Bug 1331092 - Part 1: Add Symbol.asyncIterator. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/0705d5b86ad726d12ebfb73b2ac79dffddbee6a4 Bug 1331092 - Part 2: Implement Async Generator except yield*. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6e6211270ba84591b867a7ff554be14ecbc729 Bug 1331092 - Part 3: Disable test262 test to check the non-existence of async generators. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/61e0eb18ba8cf1d9278a6420aad9317c1a69f06d Bug 1331092 - Part 4: Add release_or_beta variable to reftest condition. r=jmaher https://hg.mozilla.org/integration/mozilla-inbound/rev/8edcc256976d217f744ebd5a7072b21a7261e042 Bug 1331092 - Part 5: Enable test262 tests for async generators. r=anba https://hg.mozilla.org/integration/mozilla-inbound/rev/72750b6e9e2a69f417257326a8af6f2b611592fd Bug 1331092 - Part 6: Support JSOP_TOASYNCGEN in JIT. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/1d39acbc09227c0e8187c9acbc1abde04201b09e Bug 1331092 - Part 7: Implement Async Generator yield*. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/831d289bb6ccac19db04eca0bfba9ccef0699d19 Bug 1331092 - Part 8: Support JSOP_TOASYNCITER in JIT. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/3701b28662ac43a7d2d2bc347eddaee6a3d65a11 Bug 1331092 - Part 9: Implement for-await-of. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/9b0c51ce33e8a5bfd25e06cca7d70164893b5113 Bug 1331092 - Part 10: Support JSOP_TOASYNCGEN, JSOP_TOASYNCITER, and JSOP_AWAIT in decompiler. r=shu
Comment 84•7 years ago
|
||
Backed out bug 1331092 and bug 1350762 for breaking js plain shell tests on Windows: bug 1350762: https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e4bb0a73edb848bb93af8ca63a3b6c7d85cc42 bug 1331092: https://hg.mozilla.org/integration/mozilla-inbound/rev/32f2480cd5aeed2a5d44fe918418d66b9f897b60 https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ac3610d777849e9d5cd8c71295bc17253f6dfe https://hg.mozilla.org/integration/mozilla-inbound/rev/14c3c0f2b7183d31cfe13f4edb2ef8f1eb0cbad6 https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ba8b772e65a74441edf8d88a67c0873aa76aa3 https://hg.mozilla.org/integration/mozilla-inbound/rev/c0484f2722f37109453196112b0b1bfe5560e11c https://hg.mozilla.org/integration/mozilla-inbound/rev/1d00d1e5a0f6a8ad28921c562e0fd52b9c4555e0 https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e4c5ed3ed5fea355d42a6767d4eb686ce171a5 https://hg.mozilla.org/integration/mozilla-inbound/rev/649797f1174f3e03b3b04feb54cf0cb2a660047b https://hg.mozilla.org/integration/mozilla-inbound/rev/1977f337bc2db8db43c00d42d22b0636dc0eb66f https://hg.mozilla.org/integration/mozilla-inbound/rev/97e7ac639161dc6dc30ae89646c34d156e2ae36e Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9b0c51ce33e8a5bfd25e06cca7d70164893b5113&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=86668970&repo=mozilla-inbound c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/_virtualenv/Scripts/python.exe -m mozbuild.action.cl c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/vs2015u3/VC/bin/amd64_x86/cl.exe -FoUnified_cpp_js_src4.obj -c -DNDEBUG=1 -DTRIMMED=1 -D_CRT_RAND_S -DENABLE_BINARYDATA -DENABLE_SIMD -DENABLE_SHARED_ARRAY_BUFFER -DEXPORT_JS_API -DMOZ_HAS_MOZGLUE -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/js/src -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/js/src -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/dist/include -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/dist/include/nspr -MD -FI c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/js/src/js-confdefs.h -DMOZILLA_CLIENT -arch:SSE2 -TP -nologo -utf-8 -wd4800 -wd4595 -D_CRT_SECURE_NO_WARNINGS -wd5026 -wd5027 -Zc:sizedDealloc- -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -Gw -wd4244 -wd4267 -wd4251 -we4553 -GR- -Zi -O2 -Oy- -WX -wd4805 -wd4661 -we4067 -we4258 -we4275 -wd4146 -wd4577 -wd4312 -Fdgenerated.pdb -FS c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/js/src/Unified_cpp_js_src4.cpp Unified_cpp_js_src37.cpp c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(352): error C2589: '(': illegal token on right side of '::' c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(342): note: while compiling class template member function 'bool mozilla::BufferList<js::SystemAllocPolicy>::WriteBytes(const char *,std::size_t)' c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\js/StructuredClone.h(193): note: see reference to class template instantiation 'mozilla::BufferList<js::SystemAllocPolicy>' being compiled c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(352): error C2059: syntax error: '::' c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(362): error C2589: '(': illegal token on right side of '::' c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(362): error C2059: syntax error: '::' c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(384): error C2589: '(': illegal token on right side of '::' c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(380): note: while compiling class template member function 'bool mozilla::BufferList<js::SystemAllocPolicy>::ReadBytes(mozilla::BufferList<js::SystemAllocPolicy>::IterImpl &,char *,std::size_t) const' c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(384): error C2059: syntax error: '::' c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(459): error C2589: '(': illegal token on right side of '::' c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(451): note: while compiling class template member function 'mozilla::BufferList<js::SystemAllocPolicy> mozilla::BufferList<js::SystemAllocPolicy>::Extract(mozilla::BufferList<js::SystemAllocPolicy>::IterImpl &,std::size_t,bool *)' c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(459): error C2059: syntax error: '::' c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/config/rules.mk:1011: recipe for target 'Unified_cpp_js_src37.obj' failed mozmake[3]: *** [Unified_cpp_js_src37.obj] Error 2 mozmake[3]: *** Waiting for unfinished jobs....
Assignee | ||
Comment 85•7 years ago
|
||
Added ParserBase::asyncIterationSupported that returns false for chrome/addon and release/beta. and replaced RELEASE_OR_BETA with it.
Attachment #8851852 -
Flags: review?(shu)
Assignee | ||
Comment 86•7 years ago
|
||
workaround for the windows plain build bustage, that is caused by min/max macros in windows.h, that can be suppressed by NOMINMAX.
Attachment #8851853 -
Flags: review?(shu)
Reporter | ||
Updated•7 years ago
|
Attachment #8851524 -
Flags: review?(shu) → review+
Reporter | ||
Comment 87•7 years ago
|
||
Comment on attachment 8851852 [details] [diff] [review] Part ??: Disable Async Iteration in chrome or add-ons code. Review of attachment 8851852 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!
Attachment #8851852 -
Flags: review?(shu) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #8851853 -
Flags: review?(shu) → review+
Comment 88•7 years ago
|
||
Pushed by arai_a@mac.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/68d774d6bb73 Part 0: Define NOMINMAX to avoid compile error from min/max macro on windows. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/d1c1a6c478cb Part 1: Add Symbol.asyncIterator. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/28bce039877f Part 2: Implement Async Generator except yield*. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/dee4db84bae0 Part 3: Disable test262 test to check the non-existence of async generators. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/9bfcb40a68b8 Part 4: Add release_or_beta variable to reftest condition. r=jmaher https://hg.mozilla.org/integration/mozilla-inbound/rev/4ddc9bbcfe35 Part 5: Enable test262 tests for async generators. r=anba https://hg.mozilla.org/integration/mozilla-inbound/rev/a51f54fd72ff Part 6: Support JSOP_TOASYNCGEN in JIT. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/5a506fa42cdc Part 7: Implement Async Generator yield*. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/5f68da99a98c Part 8: Support JSOP_TOASYNCITER in JIT. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/0b11639e4085 Part 9: Implement for-await-of. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/6564ccfdc704 Part 10: Support JSOP_TOASYNCGEN, JSOP_TOASYNCITER, and JSOP_AWAIT in decompiler. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/a8464c6a70c0 Part 11: Await on the innerResult.value when innerResult.done is true in yield*. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/5e15fb883887 Part 12: Disable Async Iteration in chrome or add-ons code. r=shu
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(arai.unmht)
Comment 89•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/68d774d6bb73 https://hg.mozilla.org/mozilla-central/rev/d1c1a6c478cb https://hg.mozilla.org/mozilla-central/rev/28bce039877f https://hg.mozilla.org/mozilla-central/rev/dee4db84bae0 https://hg.mozilla.org/mozilla-central/rev/9bfcb40a68b8 https://hg.mozilla.org/mozilla-central/rev/4ddc9bbcfe35 https://hg.mozilla.org/mozilla-central/rev/a51f54fd72ff https://hg.mozilla.org/mozilla-central/rev/5a506fa42cdc https://hg.mozilla.org/mozilla-central/rev/5f68da99a98c https://hg.mozilla.org/mozilla-central/rev/0b11639e4085 https://hg.mozilla.org/mozilla-central/rev/6564ccfdc704 https://hg.mozilla.org/mozilla-central/rev/a8464c6a70c0 https://hg.mozilla.org/mozilla-central/rev/5e15fb883887
Flags: in-testsuite+
Comment 90•7 years ago
|
||
Per IRC discussion with arai, let's move follow-up work into a new bug for simpler tracking.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
Summary: Implement Async Iteration → Implement Async Iteration for web content on Nightly builds
You need to log in
before you can comment on or make changes to this bug.
Description
•