Bug 214294 - Remove ArrayNode::m_optional
Summary: Remove ArrayNode::m_optional
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Enhancement
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-13 22:23 PDT by Alexey Shvayka
Modified: 2020-07-23 01:25 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.85 KB, patch)
2020-07-13 22:26 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (7.79 KB, patch)
2020-07-21 10:46 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Shvayka 2020-07-13 22:23:50 PDT
Remove ArrayNode::m_optional
Comment 1 Alexey Shvayka 2020-07-13 22:26:54 PDT
Created attachment 404211 [details]
Patch
Comment 2 Mark Lam 2020-07-13 23:16:47 PDT
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?
Comment 3 Darin Adler 2020-07-15 11:23:45 PDT
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.
Comment 4 Mark Lam 2020-07-15 23:16:48 PDT
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.
Comment 5 Radar WebKit Bug Importer 2020-07-20 22:24:12 PDT
<rdar://problem/65866088>
Comment 6 Alexey Shvayka 2020-07-21 10:46:13 PDT
Created attachment 404836 [details]
Patch

Rewrite ChangeLog, benchmark arrays with trailing commas, convert isSpreadExpression() check to an ASSERT.
Comment 7 Alexey Shvayka 2020-07-21 10:54:51 PDT
(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.
Comment 8 EWS 2020-07-23 01:25:16 PDT
Committed r264750: <https://trac.webkit.org/changeset/264750>

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