In some cases (e.g. when a Function object is instantiated from a string), the function body source may not include its braces. The parser needs to account for this when calculating its statementDepth.
Created attachment 233016[details]
work in progress. The layout test needs some refinement. It is not currently detecting the regression because the harness is not sensitive to it.
This is a work in progress. The layout test needs some refinement. Please feel free to provide feedback if so inclined.
The benchmark tests shows that the perf is about the same in aggregate. However, in 3 separate runs, I'm seeing JSRegress' array-with-double-mul-add showing about a 10% regression. I will post benchmark results soon, and will investigate the regressions later.
Created attachment 233033[details]
patch 2: fixed tests so that they are more expressive, and can detect the issue in this bug. Still need to look into the JSRegress perf issue.
Comment on attachment 233033[details]
patch 2: fixed tests so that they are more expressive, and can detect the issue in this bug. Still need to look into the JSRegress perf issue.
View in context: https://bugs.webkit.org/attachment.cgi?id=233033&action=review
r- due to tests and apparent perf issue
> Source/JavaScriptCore/parser/Parser.cpp:1129
> + failIfFalseIfStrict(m_statementDepth == 1, "In strict mode, functions may only be declared at top level or immediately within another function");
"Strict mode does not allow function declarations in a lexically nested statement"
> LayoutTests/ChangeLog:4
> + Parser statementDepth accounting needs to account for when a function body excludes its braces.
> + <https://webkit.org/b/133832>
Please do the rephrasing in a second patch as it's currently obscuring your meaningful tests invalid vs. valid is fine in our other tests so this rephrasing isn't critical to this patch.
(In reply to comment #8)
> Created an attachment (id=233146) [details]
> patch 3: undo result rephrasing to minimize the diff. Will work on the perf regression next.
The perf issue appears to be a red herring. I reverted all the "Source" changes and the 10% on array-with-double-mil-add still manifests. I will rebuild the baseline and redo the test.
Created attachment 233192[details]
patch 4: rebased + Oliver's suggested string change.
Perf runs shows that the perf is effectively neutral. The previously supposed regression is just noise. I'll upload perf results shortly.
In these new runs of the benchmarks (4 thru 7), the benchmark component that is consistently showing a regression is now bitops-nsieve-bits (in LongSpider) which claims to have about a 1.8x regression.
However, if I do another build of the baseline and run against that (run 8), bitops-nsieve-bits is no longer definitely slower.
If I run just LongSpider, bitops-nsieve-bits shows no difference in perf.
Thanks. Landed in r170034: <http://trac.webkit.org/r170034>.
The logging changes in the parser test was necessary to detect the issues in this bug. Per Oliver (offline), we'll take the changes as is this time, but in the future, we'll make changes like this in a separate test.
2014-06-12 18:30 PDT, Mark Lam
2014-06-12 18:34 PDT, Mark Lam
2014-06-12 18:34 PDT, Mark Lam
2014-06-12 18:35 PDT, Mark Lam
2014-06-13 01:41 PDT, Mark Lam
2014-06-15 13:30 PDT, Mark Lam
2014-06-16 15:42 PDT, Mark Lam
2014-06-16 15:43 PDT, Mark Lam
2014-06-16 15:43 PDT, Mark Lam
2014-06-16 15:43 PDT, Mark Lam
2014-06-16 15:44 PDT, Mark Lam
2014-06-16 15:45 PDT, Mark Lam