Bug 221668 – [JSC] Simplify excludedSet handling in object rest expression

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Simplify excludedSet handling in object rest expression

[Yusuke Suzuki]

Created attachment 419832 [details]
Patch

[Yusuke Suzuki]

Created attachment 419833 [details]
Patch

[Yusuke Suzuki]

Created attachment 419834 [details]
Patch

[Yusuke Suzuki]

Created attachment 419891 [details]
Patch

[Yusuke Suzuki]

This should be definitely good for memory :)

[Yusuke Suzuki]

Created attachment 420217 [details]
Patch

[Yusuke Suzuki]

Ah, failure is due to the test (and because copy-data-properties become faster by this patch possibly). Fixing it separately.

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Created attachment 420219 [details]
Patch

[Caio Lima]

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`

[Alexey Shvayka]

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.

[Radar WebKit Bug Importer]

<rdar://problem/74427713>

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Committed r273135 (234332@main): <https://commits.webkit.org/234332@main>