Remove ArrayNode::m_optional

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alexey Shvayka]

Remove ArrayNode::m_optional

[Alexey Shvayka]

Created attachment 404211 [details]
Patch

[Mark Lam]

Comment on attachment 404211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404211&action=review

> Source/JavaScriptCore/ChangeLog:10
> +        m_optional has undescriptive name, its value depends solely on which ArrayNode
> +        constructor overload was used, and causes ArrayNode::isSimpleArray() to return
> +        `false` for empty array literals. 

Can you explain what the intention of m_optional was in the first place, and why it is ok to remove it?

[Darin Adler]

Comment on attachment 404211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404211&action=review

>> Source/JavaScriptCore/ChangeLog:10
>> +        `false` for empty array literals. 
> 
> Can you explain what the intention of m_optional was in the first place, and why it is ok to remove it?

m_optional dates back to before the WebKit project began. It was already in KJS when we started the WebKit project, although back then it was named "opt":

https://trac.webkit.org/browser/webkit/trunk/JavaScriptCore/kjs/nodes.h?rev=6

During that early stage KJS reflected many misunderstandings of the JavaScript language specification that we cleared up over time.

If you look carefully you can see that the opt boolean only ever meant "is this an array with elisions, an empty array, or one with a trailing comma". That wasn’t an important distinction. I don’t think this ever had a valuable function.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:509
> -    if (m_elision || m_optional)
> +    if (m_elision)

This is the only behavior change, so this is where we should focus. This is obviously correct. Nothing about an empty array or one with a trailing comma makes it different, or necessary to return false to this predicate.

[Mark Lam]

Comment on attachment 404211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404211&action=review

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:509
>> +    if (m_elision)
> 
> This is the only behavior change, so this is where we should focus. This is obviously correct. Nothing about an empty array or one with a trailing comma makes it different, or necessary to return false to this predicate.

Sounds correct to me.

[Radar WebKit Bug Importer]

<rdar://problem/65866088>

[Alexey Shvayka]

Created attachment 404836 [details]
Patch

Rewrite ChangeLog, benchmark arrays with trailing commas, convert isSpreadExpression() check to an ASSERT.

[Alexey Shvayka]

(In reply to Darin Adler from comment #3)
> If you look carefully you can see that the opt boolean only ever meant "is
> this an array with elisions, an empty array, or one with a trailing comma".
> That wasn’t an important distinction. I don’t think this ever had a valuable
> function.

Thank you, Darin. I appreciate the write-up.
I've updated the patch with far more real-world microbenchmarks (trailing comma).

```
Warmed-up runs, --outer 50:

                                           r264376                    patch

function-dot-apply-array-literal      258.1299+-7.6209     ^     22.5709+-2.0999        ^ definitely 11.4364x faster
destructuring-array-literal            50.5916+-2.9614     ^     32.4705+-1.3748        ^ definitely 1.5581x faster

<geometric>                           113.6893+-2.9768     ^     26.8028+-1.0915        ^ definitely 4.2417x faster
```

Without DFG/FTL, destructuring progresses by a factor of 4.

[EWS]

Committed r264750: <https://trac.webkit.org/changeset/264750>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404836 [details].