Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2297733002:
[parser] Refactor bookmark in SkipLazyFunctionBody (Closed)
Description[parser] Refactor bookmark in SkipLazyFunctionBody
This patch refactors the scanner bookmark in SkipLazyFunctionBody,
so that it is only used locally, instead of being passed to several
other methods. It is replaced by a "may_abort" parameter and an
appropriate result denoting whether lazy parsing has been aborted.
It also applies the hack of aborting lazy parsing for arrow
functions that are considered to be "initialization functions".
R=adamk@chromium.org, vogelheim@chromium.org
BUG=
LOG=N
Committed: https://crrev.com/5d6eabb0eba0970091af936bc3fc80d420461373
Cr-Commit-Position: refs/heads/master@{#39072}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Change according to reviewers' comments #Patch Set 3 : Rebase #Patch Set 4 : Fix formatting #Patch Set 5 : Fix error with UNREACHABLE #
Messages
Total messages: 26 (12 generated)
lgtm (just some nitpicks.) https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:4060: // which may decide to abort lazy parsing if it suspect that wasn't a good nitpick: suspect -> suspects https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:4061: // idea. If so (in which case the parser is expected to have backtracked), nitpick: One space after "idea." https://codereview.chromium.org/2297733002/diff/1/src/parsing/preparser.h File src/parsing/preparser.h (right): https://codereview.chromium.org/2297733002/diff/1/src/parsing/preparser.h#new... src/parsing/preparser.h:717: return ParseStatementList(end_token, false, ok); If may_abort is set to false, ParseStatementList would only ever return false, right? Then maybe just have this: V8_INLINE void ParseStatementList(int end_token, bool* ok) { ParseStatementList(end_token, false, ok); } Or maybe add a DCHECK for the ParserStatementList's return value.
Do we have any test coverage for this aborting logic? https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:3365: typename Types::StatementList body = impl()->NullStatementList(); FunctionLiteral-consuming code is OK with null body? https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:3400: function_state.materialized_literal_count(); Was this wrong before? Why did it change? https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:3481: function_literal->set_should_be_used_once_hint(); Same style nit, please add { } https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:4196: if (!ok) return false; I think you mean "if (!*ok)". bool* is about my least favorite type. Consider adding a CHECK_OK_FALSE macro? https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:4229: if (!ok) return false; Ok, I think the same error twice is enough to convince me that you should add a CHECK_OK_FALSE. https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser.h#newcod... src/parsing/parser.h:548: // in order to force the function to be eagerly parsed, after all. Mention when it'll return true vs false? I was surprised to see that "true" means aborted and "false" means "not aborted". Having read through the code I'm wondering if we'd be better off with an enum (or maybe a pair of enums) here. It's a bit strange to see the unusual case as "true". https://codereview.chromium.org/2297733002/diff/1/src/parsing/preparser.cc File src/parsing/preparser.cc (right): https://codereview.chromium.org/2297733002/diff/1/src/parsing/preparser.cc#ne... src/parsing/preparser.cc:217: if (!starts_with_identifier) Please add curly braces around the if/else statements, which is the usual style for multi-line if statements in V8. https://codereview.chromium.org/2297733002/diff/1/src/parsing/preparser.cc#ne... src/parsing/preparser.cc:1117: if (aborted) return true; And having gotten this far I also think it would be more readable to have two enums, all these bools are really hard to keep in my head.
On 2016/08/30 22:13:15, adamk wrote: > Do we have any test coverage for this aborting logic? I checked this will Daniel and the quick answer is no. In the next paragraph, I'm copying parts of Daniel's answer from a conversation we had offline. We don't have unit tests. We regularly run Octane/mandreel-latency, which was the picture book base for that logic, and that both discovers whether the logic triggers (because it modifies the score), and also whether it breaks stuff (because it's known to actually trigger). We could write unit tests for this, but it will not be easy to check that the logic actually triggers, as it is not observable in JS. As far as I can see, they would have to go to cctest, where we would call SkipLazyFunctionBody directly. If you want this, I suggest that we create an issue and postpone it for a later CL, because subsequent refactoring depends on this one and (my) time is ticking... :-) https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:3365: typename Types::StatementList body = impl()->NullStatementList(); On 2016/08/30 22:13:14, adamk wrote: > FunctionLiteral-consuming code is OK with null body? Yes, this is how it works already, in the case of non-arrow function literals. https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:3400: function_state.materialized_literal_count(); On 2016/08/30 22:13:14, adamk wrote: > Was this wrong before? Why did it change? I changed because it was different from the case of non-arrow function literals (parser.cc:4071). I'm changing it back, as I'm not sure I understand the logic of materialized literals. https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:3481: function_literal->set_should_be_used_once_hint(); On 2016/08/30 22:13:14, adamk wrote: > Same style nit, please add { } Done. But look at the very next line... :-) https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:4060: // which may decide to abort lazy parsing if it suspect that wasn't a good On 2016/08/30 16:28:42, vogelheim wrote: > nitpick: suspect -> suspects Done. https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:4061: // idea. If so (in which case the parser is expected to have backtracked), On 2016/08/30 16:28:42, vogelheim wrote: > nitpick: One space after "idea." Done. https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:4196: if (!ok) return false; On 2016/08/30 22:13:14, adamk wrote: > I think you mean "if (!*ok)". bool* is about my least favorite type. > > Consider adding a CHECK_OK_FALSE macro? Done. https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:4229: if (!ok) return false; On 2016/08/30 22:13:14, adamk wrote: > Ok, I think the same error twice is enough to convince me that you should add a > CHECK_OK_FALSE. Done. https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser.h#newcod... src/parsing/parser.h:548: // in order to force the function to be eagerly parsed, after all. On 2016/08/30 22:13:14, adamk wrote: > Mention when it'll return true vs false? I was surprised to see that "true" > means aborted and "false" means "not aborted". > > Having read through the code I'm wondering if we'd be better off with an enum > (or maybe a pair of enums) here. It's a bit strange to see the unusual case as > "true". Done. https://codereview.chromium.org/2297733002/diff/1/src/parsing/preparser.cc File src/parsing/preparser.cc (right): https://codereview.chromium.org/2297733002/diff/1/src/parsing/preparser.cc#ne... src/parsing/preparser.cc:217: if (!starts_with_identifier) On 2016/08/30 22:13:14, adamk wrote: > Please add curly braces around the if/else statements, which is the usual style > for multi-line if statements in V8. Done. https://codereview.chromium.org/2297733002/diff/1/src/parsing/preparser.cc#ne... src/parsing/preparser.cc:1117: if (aborted) return true; On 2016/08/30 22:13:14, adamk wrote: > And having gotten this far I also think it would be more readable to have two > enums, all these bools are really hard to keep in my head. Done. https://codereview.chromium.org/2297733002/diff/1/src/parsing/preparser.h File src/parsing/preparser.h (right): https://codereview.chromium.org/2297733002/diff/1/src/parsing/preparser.h#new... src/parsing/preparser.h:717: return ParseStatementList(end_token, false, ok); On 2016/08/30 16:28:42, vogelheim wrote: > If may_abort is set to false, ParseStatementList would only ever return false, > right? > > Then maybe just have this: > > V8_INLINE void ParseStatementList(int end_token, bool* ok) { > ParseStatementList(end_token, false, ok); > } > > Or maybe add a DCHECK for the ParserStatementList's return value. Done.
lgtm, and agreed that this needn't block on tests, though it would be good to have confidence that this logic is covered, such that a test would fail if it changed in a bad way. Sounds like maybe we already think this is well-covered by perf tests. https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2297733002/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:3481: function_literal->set_should_be_used_once_hint(); On 2016/08/31 13:23:36, nickie wrote: > On 2016/08/30 22:13:14, adamk wrote: > > Same style nit, please add { } > > Done. But look at the very next line... :-) The difference is whether the entire if-statement fits on a single line. Sadly I don't think this is written down anywhere, but it's the generally-followed practice in v8 from what I can tell (and I'm scared of non-curly-wrapped if-statements :)
The CQ bit was checked by nikolaos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vogelheim@chromium.org Link to the patchset: https://codereview.chromium.org/2297733002/#ps20001 (title: "Change according to reviewers' comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/11862)
The CQ bit was checked by nikolaos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org, vogelheim@chromium.org Link to the patchset: https://codereview.chromium.org/2297733002/#ps40001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/23010)
The CQ bit was checked by nikolaos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org, vogelheim@chromium.org Link to the patchset: https://codereview.chromium.org/2297733002/#ps60001 (title: "Fix formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by nikolaos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org, vogelheim@chromium.org Link to the patchset: https://codereview.chromium.org/2297733002/#ps80001 (title: "Fix error with UNREACHABLE")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [parser] Refactor bookmark in SkipLazyFunctionBody This patch refactors the scanner bookmark in SkipLazyFunctionBody, so that it is only used locally, instead of being passed to several other methods. It is replaced by a "may_abort" parameter and an appropriate result denoting whether lazy parsing has been aborted. It also applies the hack of aborting lazy parsing for arrow functions that are considered to be "initialization functions". R=adamk@chromium.org, vogelheim@chromium.org BUG= LOG=N ========== to ========== [parser] Refactor bookmark in SkipLazyFunctionBody This patch refactors the scanner bookmark in SkipLazyFunctionBody, so that it is only used locally, instead of being passed to several other methods. It is replaced by a "may_abort" parameter and an appropriate result denoting whether lazy parsing has been aborted. It also applies the hack of aborting lazy parsing for arrow functions that are considered to be "initialization functions". R=adamk@chromium.org, vogelheim@chromium.org BUG= LOG=N Committed: https://crrev.com/5d6eabb0eba0970091af936bc3fc80d420461373 Cr-Commit-Position: refs/heads/master@{#39072} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5d6eabb0eba0970091af936bc3fc80d420461373 Cr-Commit-Position: refs/heads/master@{#39072} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
