[JSC] Simplify excludedSet handling in object rest expression
Created attachment 419832 [details] Patch
Created attachment 419833 [details] Patch
Created attachment 419834 [details] Patch
Created attachment 419891 [details] Patch
This should be definitely good for memory :)
Created attachment 420217 [details] Patch
Ah, failure is due to the test (and because copy-data-properties become faster by this patch possibly). Fixing it separately.
(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.
Created attachment 420219 [details] Patch
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 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.
<rdar://problem/74427713>
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 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.
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
Committed r273135 (234332@main): <https://commits.webkit.org/234332@main>