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.
<rdar://problem/17056898>
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 233017 [details] perf run 1
Created attachment 233018 [details] perf run 2: (inversed) new patch as Conf#1, baseline as Conf#2.
Created attachment 233019 [details] perf run 3
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.
Created attachment 233146 [details] patch 3: undo result rephrasing to minimize the diff. Will work on the perf regression next.
(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.
Created attachment 233193 [details] perf run 4
Created attachment 233194 [details] perf run 5
Created attachment 233195 [details] perf run 6
Created attachment 233196 [details] perf run 7: inversed (new code first followed by baseline)
Created attachment 233197 [details] perf run 8: inversed (new code first followed by a 2nd build of the baseline)
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.
Comment on attachment 233192 [details] patch 4: rebased + Oliver's suggested string change. r-, please remove the changes to logging in the parser test
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.