Summary: | parsing arrow function expressions slows down the parser by 8% lets recoup some loss | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | benjamin, buildbot, commit-queue, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, oliver, rniwa, sukolsak, ysuzuki | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | 157513 | ||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||
Attachments: |
|
Description
Saam Barati
2016-03-29 14:21:35 PDT
Created attachment 275222 [details]
patch
VMs tested: "og" at /Volumes/Data/WK/a/OpenSource/WebKitBuild/Release/jsc (r198848) "arrow" at /Volumes/Data/WK/b/OpenSource/WebKitBuild/Release/jsc (r198848) Collected 10 samples per benchmark/VM, with 10 VM invocations per benchmark. Emitted a call to gc() between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds. og arrow closure 0.76198+-0.00157 ^ 0.72355+-0.00140 ^ definitely 1.0531x faster jquery 9.87951+-0.31561 ^ 9.10787+-0.03122 ^ definitely 1.0847x faster <geometric> 2.74314+-0.04285 ^ 2.56709+-0.00460 ^ definitely 1.0686x faster Comment on attachment 275222 [details] patch Attachment 275222 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1071510 Number of test failures exceeded the failure limit. Created attachment 275225 [details]
Archive of layout-test-results from ews112 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 275222 [details] patch Attachment 275222 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1071532 New failing tests: inspector/runtime/parse.html Created attachment 275227 [details]
Archive of layout-test-results from ews103 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 275222 [details] patch Attachment 275222 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1071550 New failing tests: inspector/runtime/parse.html Created attachment 275228 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 275234 [details]
patch
fix inspector test failure
Comment on attachment 275234 [details] patch Attachment 275234 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1072029 Number of test failures exceeded the failure limit. Created attachment 275239 [details]
Archive of layout-test-results from ews113 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 275234 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=275234&action=review Looks good to me. Can you please check the test coverage before landing. We often have good coverage of valid syntax and poor coverage of various invalid syntax. > Source/JavaScriptCore/ChangeLog:36 > + `({x = 30}) => x;` Do you have test coverage for those failing cases? (In reply to comment #12) > Comment on attachment 275234 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275234&action=review > > Looks good to me. > Can you please check the test coverage before landing. We often have good > coverage of valid syntax and poor coverage of various invalid syntax. > > > Source/JavaScriptCore/ChangeLog:36 > > + `({x = 30}) => x;` > > Do you have test coverage for those failing cases? Thanks for the review. I'll add a bunch of tests to our parser syntax check tests. Created attachment 275255 [details]
patch for landing
I'm going to let EWS take a look.
Created attachment 275256 [details]
patch for landing
a few more tests
Comment on attachment 275256 [details] patch for landing Attachment 275256 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1072652 Number of test failures exceeded the failure limit. Created attachment 275257 [details]
Archive of layout-test-results from ews114 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
The reason the debug tests are very interesting. Look at the following program: ``` function foo() { var y = (x) => x; } ``` because we initially successfully parse "(x)", we assume that we're *using* the variable "x". This is done because if the line were written "var y = (x);" we would really be using "x". However, we aren't using "x" here because it's a parameter to the arrow function. But after we notice the "=>" it's too late to backtrack on this use. And because we store these uses in a hash table, it's not obvious on how to backtrack this. The biggest consequence that this has is that we might claim that certain variables are captured when they really aren't. But, because the outer-most function in a builtin isn't supposed to close over any variables, we crash from a debug assertion. I have an idea for how we might be able to do this. I think we can create a dummy used variable set and swap it in if we think we might be parsing an arrow function. If we are indeed parsing an arrow function, we can swap back in the original hash table. If we're not parsing an arrow function, we can add back any contents from the new hash table to the old one, and swap back in the old one. Maybe this is too crazy. I'm going to try it out. Another possibility is we can make the concession that our capture analysis is imprecise and that we won't have debugging code inside builtins to prevent global capture. Lets see if the hash table swapping degrades the perf progression. (In reply to comment #18) > The reason the debug tests are very interesting. Look at the following > program: > ``` > function foo() { > var y = (x) => x; > } > ``` > because we initially successfully parse "(x)", we assume that we're > *using* the variable "x". This is done because if the line were > written "var y = (x);" we would really be using "x". However, > we aren't using "x" here because it's a parameter to the arrow > function. But after we notice the "=>" it's too late to backtrack > on this use. And because we store these uses in a hash table, it's > not obvious on how to backtrack this. The biggest consequence that this > has is that we might claim that certain variables are captured when > they really aren't. But, because the outer-most function in a builtin > isn't supposed to close over any variables, we crash from a debug > assertion. > > I have an idea for how we might be able to do this. I think we can > create a dummy used variable set and swap it in if we think > we might be parsing an arrow function. If we are indeed parsing > an arrow function, we can swap back in the original hash table. > If we're not parsing an arrow function, we can add back any > contents from the new hash table to the old one, and swap back in > the old one. Maybe this is too crazy. I'm going to try it out. > Another possibility is we can make the concession that our capture > analysis is imprecise and that we won't have debugging code inside > builtins to prevent global capture. Lets see if the hash table > swapping degrades the perf progression. So the naive move() solution results in no perf progression, which means it's too high of overhead. I tried a few other solutions that involve marking variables used in such contexts. It takes away about half the progression. I was seeing about 2.5-3% progression instead of 5.5%-6%. I'll think about this more and see if we can recoup all of the loss. So I think we need to make the concession that we're not going to get a 6% progression from this. It seems pretty lame to slow down the runtime execution properties of the code based on how you name your parameters in an arrow function. I'm seeing a 3% progression while maintaining good capture analysis. We will probably get higher than 3% progression. I'm actually seeing 4-5%. VMs tested: "og" at /Volumes/Data/WK/a/OpenSource/WebKitBuild/Release/jsc (r198861) "parser" at /Volumes/Data/WK/b/OpenSource/WebKitBuild/Release/jsc (r198861) Collected 14 samples per benchmark/VM, with 14 VM invocations per benchmark. Emitted a call to gc() between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds. og parser closure 0.53500+-0.00306 ^ 0.51041+-0.00275 ^ definitely 1.0482x faster jquery 6.75072+-0.02721 ^ 6.42015+-0.01876 ^ definitely 1.0515x faster <geometric> 1.90039+-0.00654 ^ 1.81020+-0.00474 ^ definitely 1.0498x faster Created attachment 275336 [details]
patch for review/ews
I'm putting this up for review again because this has changed enough since Ben's previous review.
Comment on attachment 275336 [details] patch for review/ews Clearing flags on attachment: 275336 Committed r198927: <http://trac.webkit.org/changeset/198927> All reviewed patches have been landed. Closing bug. |