Bug 200875 – [JSC] Ensure x?.y ?? z is fast

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Ross Kirsling]

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.)

[Ross Kirsling]

Created attachment 376713 [details]
Patch

[Ross Kirsling]

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.

[Ross Kirsling]

(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.

[Ross Kirsling]

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...

[Yusuke Suzuki]

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.

[Ross Kirsling]

(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.

[Ross Kirsling]

Created attachment 376926 [details]
Patch

[Ross Kirsling]

Comment on attachment 376926 [details]
Patch

Addressed feedback.

[Ross Kirsling]

Ping?

[Yusuke Suzuki]

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.

[WebKit Commit Bot]

Comment on attachment 376926 [details]
Patch

Clearing flags on attachment: 376926

Committed r249117: <https://trac.webkit.org/changeset/249117>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54721317>