Bug 221668 - [JSC] Simplify excludedSet handling in object rest expression
Summary: [JSC] Simplify excludedSet handling in object rest expression
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-10 02:54 PST by Yusuke Suzuki
Modified: 2021-02-19 01:44 PST (History)
9 users (show)

See Also:


Attachments
Patch (30.34 KB, patch)
2021-02-10 02:55 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (30.79 KB, patch)
2021-02-10 02:57 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (30.99 KB, patch)
2021-02-10 03:00 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (31.13 KB, patch)
2021-02-10 12:43 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (34.12 KB, patch)
2021-02-13 01:00 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (34.11 KB, patch)
2021-02-13 03:01 PST, Yusuke Suzuki
shvaikalesh: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2021-02-10 02:54:47 PST
[JSC] Simplify excludedSet handling in object rest expression
Comment 1 Yusuke Suzuki 2021-02-10 02:55:00 PST
Created attachment 419832 [details]
Patch
Comment 2 Yusuke Suzuki 2021-02-10 02:57:27 PST
Created attachment 419833 [details]
Patch
Comment 3 Yusuke Suzuki 2021-02-10 03:00:51 PST
Created attachment 419834 [details]
Patch
Comment 4 Yusuke Suzuki 2021-02-10 12:43:28 PST
Created attachment 419891 [details]
Patch
Comment 5 Yusuke Suzuki 2021-02-10 13:29:30 PST
This should be definitely good for memory :)
Comment 6 Yusuke Suzuki 2021-02-13 01:00:32 PST
Created attachment 420217 [details]
Patch
Comment 7 Yusuke Suzuki 2021-02-13 02:59:44 PST
Ah, failure is due to the test (and because copy-data-properties become faster by this patch possibly). Fixing it separately.
Comment 8 Yusuke Suzuki 2021-02-13 03:00:03 PST
(In reply to Yusuke Suzuki from comment #7)
> Ah, failure is due to the test (and because copy-data-properties become
> faster by this patch possibly). Fixing it separately.

<https://commits.webkit.org/234068@main> for that.
Comment 9 Yusuke Suzuki 2021-02-13 03:01:45 PST
Created attachment 420219 [details]
Patch
Comment 10 Caio Lima 2021-02-15 03:08:16 PST
Comment on attachment 420219 [details]
Patch

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

Informal r+. This patch is really good. Does it impact any other benchmark?

> Source/JavaScriptCore/ChangeLog:13
> +        need to call Set#add in JS side.

Very nice!

> Source/JavaScriptCore/ChangeLog:18
> +        side-effect free if the input is number or string.

It would be nice have in the ChangeLog the microbenchmark results.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:5620
> +    // This must be non-tail-call because of @copyDataProperties's assumption.

It would be nice to state this assumption here. For instance, it's not clear to me which assumption you are talking about.

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:-904
> -                continue;

Great that this allows move this check back to `forEachProperty`
Comment 11 Alexey Shvayka 2021-02-15 15:03:43 PST
Comment on attachment 420219 [details]
Patch

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

This is a thing of beauty, r=me.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:5616
> +    CallArguments args(generator, nullptr, 1);

-1 bytecode op, nice.

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:875
> +                auto propertyName = callFrame->uncheckedArgument(index).toPropertyKey(globalObject);

nit: Can we have a comment saying "this isn't observable since ObjectPatternNode::bindValue() also performs ToPropertyKey"?

> JSTests/ChangeLog:8
> +        * microbenchmarks/object-rest-computed-destructuring.js: Added.

nit: Stating microbenchmark progression would be nice for future ChangeLog readers.
Comment 12 Radar WebKit Bug Importer 2021-02-17 02:55:13 PST
<rdar://problem/74427713>
Comment 13 Yusuke Suzuki 2021-02-19 01:23:29 PST
Comment on attachment 420219 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:875
>> +                auto propertyName = callFrame->uncheckedArgument(index).toPropertyKey(globalObject);
> 
> nit: Can we have a comment saying "this isn't observable since ObjectPatternNode::bindValue() also performs ToPropertyKey"?

Yeah, added :)
Comment 14 Yusuke Suzuki 2021-02-19 01:23:52 PST
Comment on attachment 420219 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:5620
>> +    // This must be non-tail-call because of @copyDataProperties's assumption.
> 
> It would be nice to state this assumption here. For instance, it's not clear to me which assumption you are talking about.

Yup, added.
Comment 15 Yusuke Suzuki 2021-02-19 01:36:35 PST
Yeah, this is showing 2x perf boost.

                                               ToT                     Patched

object-rest-computed-destructuring       62.9960+-6.5072     ^     30.2157+-1.1420        ^ definitely 2.0849x faster
Comment 16 Yusuke Suzuki 2021-02-19 01:44:22 PST
Committed r273135 (234332@main): <https://commits.webkit.org/234332@main>