It is anticipated that `x?.y ?? z` will quickly become a common idiom in JS (as it is in C#, Swift, etc.). Indeed, such an example is given as motivation for the proposal: (https://github.com/tc39/proposal-optional-chaining/#overview-and-motivation) > const animationDuration = response.settings?.animationDuration ?? 300; As such, it seems unfortunate that we currently generate the following: (get x) ----- jundefined_or_null | (get y) | --- jmp > | (load undefined) > - jnundefined_or_null | (get z) > end When it clearly could be reduced to: (get x) --- jundefined_or_null | (get y) | - jnundefined_or_null > | (get z) > end What I'm not sure about: - Is this appropriate to handle at initial bytecode generation? (If so, we could do so with a combined chain-and-coalesce node.) - Is this the sort of thing we expect to leave for DFG or FTL to handle? (Looking at the dumped graph, this does not seem to come for free.)
Created attachment 376713 [details] Patch
Comment on attachment 376713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376713&action=review Guess this is really easy after all -- we don't even need a new node, just a new field. A couple of notes: > Source/JavaScriptCore/parser/ASTBuilder.h:1370 > + if (!optionalChain->isDelete()) { We absolutely could reduce `delete x?.y ?? z` too, but it'd just equate to dropping `?? z`, which seems extreme and only really affects bytecode size, since branch prediction would take care of the rest. > Source/JavaScriptCore/parser/ASTBuilder.h:1372 > + return new (m_parserArena) CoalesceNode(location, optionalChain->expr(), expr2, hasAbsorbedOptionalChain); Why throw away the OptionalChainNode here? Because to preserve it, we'd have to not only clear isOutermost on it here and then *check* whether it was cleared over in NodesCodegen. It's simpler just to say that we "absorbed" the thing.
(In reply to Ross Kirsling from comment #2) > Why throw away the OptionalChainNode here? Because to preserve it, we'd have > to not only clear isOutermost on it here and then *check* whether it was > cleared over in NodesCodegen. It's simpler just to say that we "absorbed" > the thing. s/and then/but also/, grrr.
Created attachment 376798 [details] Alternate patch (prunes delete x?.y ?? z) Just for consideration, here is what the patch would look like if we were turn `delete x?.y ?? z` into `delete x?.y` instead. We don't need to do this, since it should be handled by DFG instead, but it's still interesting because the patch isn't really any more complex than the other way...
Comment on attachment 376713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376713&action=review > Source/JavaScriptCore/ChangeLog:9 > + We anticipate `x?.y ?? z` to quickly become a common idiom in JS. With a little bytecode rearrangement, > + we can avoid the "load undefined and check it" dance in the middle and just turn this into two jumps. Can you describe the previous bytecode and optimized bytecode in ChangeLog to make the change clear? > Source/JavaScriptCore/parser/ASTBuilder.h:1371 > + static constexpr bool hasAbsorbedOptionalChain = true; `static` is not necessary. > Source/JavaScriptCore/parser/ASTBuilder.h:1375 > + return new (m_parserArena) CoalesceNode(location, expr1, expr2); Let's define `constexpr bool hasAbsorbedOptionalChain = false;` and pass it explicitly. > Source/JavaScriptCore/parser/Nodes.h:1315 > + CoalesceNode(const JSTokenLocation&, ExpressionNode* expr1, ExpressionNode* expr2, bool = false); Only one place is using this default bool parameter. So I think not making it default parameter is better. Pass `false` explicitly. > Source/JavaScriptCore/parser/Nodes.h:1333 > void setIsDelete() { m_isDelete = true; } > + bool isDelete() const { return m_isDelete; } Can we change the name of this functions to like, `isHoldingDelete` or something like this? This function name looks like this `OptionalChainNode` itself looks DeleteNode.
(In reply to Yusuke Suzuki from comment #5) > > Source/JavaScriptCore/parser/Nodes.h:1333 > > void setIsDelete() { m_isDelete = true; } > > + bool isDelete() const { return m_isDelete; } > > Can we change the name of this functions to like, `isHoldingDelete` or > something like this? This function name looks like this `OptionalChainNode` > itself looks DeleteNode. Actually, now that you mention it... since this really means `childIsDelete`, I should probably just add `isDeleteNode` to ExpressionNode instead.
Created attachment 376926 [details] Patch
Comment on attachment 376926 [details] Patch Addressed feedback.
Ping?
Comment on attachment 376926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376926&action=review r=me > Source/JavaScriptCore/parser/ASTBuilder.h:-1162 > - optionalChain->setIsDelete(); Nice.
Comment on attachment 376926 [details] Patch Clearing flags on attachment: 376926 Committed r249117: <https://trac.webkit.org/changeset/249117>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54721317>