Summary: | We should be able to eliminate cloned arguments objects that use the length property | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||
Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, mark.lam, msaboff, rniwa, saam | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Keith Miller
2016-03-12 08:13:08 PST
Created attachment 273989 [details]
Patch
Attachment 273989 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:122: Place brace on its own line for function definitions. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:129: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 2 in 26 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 273991 [details]
Benchmark results
!s appear to be noise on reruns.
Comment on attachment 273989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273989&action=review You should add a performance regression test, and also some adversarial tests that modify the structure of the arguments object in interesting ways and verify that doing so still gets the right behavior. > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:434 > + // Meh, this is kind of hackish - we use an Identity so that we can reuse the > + // getArrayLength() helper. I don't think you need this comment. Converting to identity is a pretty common idiom. > Source/JavaScriptCore/runtime/ClonedArguments.cpp:44 > ClonedArguments* ClonedArguments::createEmpty( All of the create functions in this file should end by ASSERTing that their structure is equal to globalObject->clonedArgumentsStructure(). > Source/JavaScriptCore/runtime/ClonedArguments.cpp:140 > + // FIXME: we should do a better job at choosing the indexing type for cloned arguments. We always choose contiguous > + // since it enables arguments elimination with the length property. https://bugs.webkit.org/show_bug.cgi?id=155396 This comment would be better if it just explained the rationale for the current way of doing things, rather than half-explaining and half-criticizing: // We use contiguous storage because optimizations in the FTL assume that cloned arguments creation always produces the same initial structure. It's not necessarily obvious whether this should change or not. We should reserve FIXME for cases when we know that something should change. Comment on attachment 273989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273989&action=review > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:126 > + if (m_candidates.remove(edge.node()) && verbose) > + dataLog("eliminating candidate: ", edge.node(), " because it escapes from: ", source, "\n"); Style: I'm not a big fan of having a logging branch and a necessary operation in the same "if" statement. I think it'd be better to assign the result to a local and branch on that and "verbose" Comment on attachment 273989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273989&action=review >> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:126 >> + dataLog("eliminating candidate: ", edge.node(), " because it escapes from: ", source, "\n"); > > Style: I'm not a big fan of having a logging branch and a necessary operation in the same "if" statement. > I think it'd be better to assign the result to a local and branch on that and "verbose" Fair enough, fixed. >> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:434 >> + // getArrayLength() helper. > > I don't think you need this comment. Converting to identity is a pretty common idiom. Removed. >> Source/JavaScriptCore/runtime/ClonedArguments.cpp:44 >> ClonedArguments* ClonedArguments::createEmpty( > > All of the create functions in this file should end by ASSERTing that their structure is equal to globalObject->clonedArgumentsStructure(). Added. >> Source/JavaScriptCore/runtime/ClonedArguments.cpp:140 >> + // since it enables arguments elimination with the length property. https://bugs.webkit.org/show_bug.cgi?id=155396 > > This comment would be better if it just explained the rationale for the current way of doing things, rather than half-explaining and half-criticizing: > > // We use contiguous storage because optimizations in the FTL assume that cloned arguments creation always produces the same initial structure. > > It's not necessarily obvious whether this should change or not. We should reserve FIXME for cases when we know that something should change. Ok, I changed it to your comment. Created attachment 274004 [details]
Patch for landing
Comment on attachment 274004 [details] Patch for landing Clearing flags on attachment: 274004 Committed r198154: <http://trac.webkit.org/changeset/198154> All reviewed patches have been landed. Closing bug. Looks like this was 2-5% improvement on Speedometer! |