Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
792083002:
Add materialized literals for tagged templates in preparser (Closed)
|
Created:
6 years ago by caitp (gmail) Modified:
6 years ago CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Project:
v8 Visibility:
Public. |
DescriptionAdd materialized literals for tagged templates in preparser
LOG=N
R=arv@chromium.org, dslomov@chromium.org, marja@chromium.org
BUG=
Patch Set 1 #
Total comments: 3
Patch Set 2 : Assert preparser materialized literals === parser materialized literals #Patch Set 3 : Oops. #
Total comments: 4
Patch Set 4 : Simplify test case, also test untagged templates #Patch Set 5 : Remove changes to preparse_min_length flag #Patch Set 6 : Oops nit. #Patch Set 7 : Rebased #
Total comments: 3
Messages
Total messages: 20 (3 generated)
https://codereview.chromium.org/792083002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/792083002/diff/1/test/cctest/test-parsing.cc#... test/cctest/test-parsing.cc:1428: actual_materialized_literals = function->materialized_literal_count(); have to do this here because the contents of the function literal die once info is out of scope :( https://codereview.chromium.org/792083002/diff/1/test/cctest/test-parsing.cc#... test/cctest/test-parsing.cc:4507: "'use strict';", (lldb) p actual_materialized_literals (int) $16 = 0 (lldb) up frame #1: 0x00000001002ee98d cctest`TestParserSync(source=0x0000000103a04e70, varying_flags=0x0000000103a04e60, varying_flags_length=2, result=kSuccess, always_true_flags=0x0000000101130120, always_true_flags_length=1, always_false_flags=0x0000000000000000, always_false_flags_length=0, literals=2) + 413 at test-parsing.cc:1532 1529 ++flag_index) { 1530 flags.Remove(always_false_flags[flag_index]); 1531 } -> 1532 TestParserSyncWithFlags(str, flags, result, literals); 1533 } 1534 } 1535 (lldb) p str (v8::internal::Handle<v8::internal::String>) $17 = { location_ = 0x000000010482ecc8 } (lldb) p str->ToAsciiArray() (char *) $18 = 0x0000000103f00050 "function test(){ 'use strict'; function tag() {} var a, b, c; return tag``;}" --- So I was slightly wrong, it's not a "use strict" context, just an empty context.
dslomov@chromium.org changed reviewers: + dslomov@chromium.org
DBC https://codereview.chromium.org/792083002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/792083002/diff/1/test/cctest/test-parsing.cc#... test/cctest/test-parsing.cc:1394: int literals = -1) { Instead of passing the expected number of literals here, could you just compare results coming from preparser and from parser and assert that those are the same?
On 2014/12/10 18:49:32, Dmitry Lomov (chromium) wrote: > DBC > > https://codereview.chromium.org/792083002/diff/1/test/cctest/test-parsing.cc > File test/cctest/test-parsing.cc (right): > > https://codereview.chromium.org/792083002/diff/1/test/cctest/test-parsing.cc#... > test/cctest/test-parsing.cc:1394: int literals = -1) { > Instead of passing the expected number of literals here, could you just compare > results coming from preparser and from parser and assert that those are the > same? Okay, I see --- Would it maybe make more sense to make that a DCHECK in the code itself? Maybe it's not possible to do that.
On 2014/12/10 19:37:06, caitp wrote: > On 2014/12/10 18:49:32, Dmitry Lomov (chromium) wrote: > > DBC > > > > https://codereview.chromium.org/792083002/diff/1/test/cctest/test-parsing.cc > > File test/cctest/test-parsing.cc (right): > > > > > https://codereview.chromium.org/792083002/diff/1/test/cctest/test-parsing.cc#... > > test/cctest/test-parsing.cc:1394: int literals = -1) { > > Instead of passing the expected number of literals here, could you just > compare > > results coming from preparser and from parser and assert that those are the > > same? > > Okay, I see --- Would it maybe make more sense to make that a DCHECK in the code > itself? Maybe it's not possible to do that. That's even better of course.
On 2014/12/10 19:46:42, Dmitry Lomov (chromium) wrote: > On 2014/12/10 19:37:06, caitp wrote: > > On 2014/12/10 18:49:32, Dmitry Lomov (chromium) wrote: > > > DBC > > > > > > https://codereview.chromium.org/792083002/diff/1/test/cctest/test-parsing.cc > > > File test/cctest/test-parsing.cc (right): > > > > > > > > > https://codereview.chromium.org/792083002/diff/1/test/cctest/test-parsing.cc#... > > > test/cctest/test-parsing.cc:1394: int literals = -1) { > > > Instead of passing the expected number of literals here, could you just > > compare > > > results coming from preparser and from parser and assert that those are the > > > same? > > > > Okay, I see --- Would it maybe make more sense to make that a DCHECK in the > code > > itself? Maybe it's not possible to do that. > That's even better of course. I couldn't find a way to do that easily, it seems like a bigger refactoring --- it's gross, but there's a temporary way to make this assertion for top-level scopes at least.
dslomov@chromium.org changed reviewers: + marja@chromium.org
I see; maybe it is too complex (I didn't realize we do not have a ParserRecorder for top-level) marja@, WDYT?
We talked with dslomov@ about the testing approach. Ultimately we should make the test call PreParseLazyFunction instead of PreParseProgram (that is also closer to the real usage of PreParser), but exposing the materialized literal count in PreParseProgram is an okay approach for now. https://codereview.chromium.org/792083002/diff/40001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/792083002/diff/40001/src/preparser.h#newcode1495 src/preparser.h:1495: function_state_->materialized_literal_count(); Nit: the body is not a one-liner, so add { }. https://codereview.chromium.org/792083002/diff/40001/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/792083002/diff/40001/test/cctest/test-parsing... test/cctest/test-parsing.cc:4490: "// xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n" Hmm... so I guess the purpose of this is to force preparsing? 1) I don't get why you need it. So without it, we'd test parser against preparser, with it, we'll test parser-which-falls-back-to-preparser against preparser. Is that needed for reproducing the situation you want to reproduce? 2) If you need to do it, you should set i::FLAG_min_preparse_length = 0; instead (and add a comment).
https://codereview.chromium.org/792083002/diff/40001/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/792083002/diff/40001/test/cctest/test-parsing... test/cctest/test-parsing.cc:4490: "// xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n" On 2014/12/11 11:31:12, marja wrote: > Hmm... so I guess the purpose of this is to force preparsing? > > 1) I don't get why you need it. So without it, we'd test parser against > preparser, with it, we'll test parser-which-falls-back-to-preparser against > preparser. Is that needed for reproducing the situation you want to reproduce? > > 2) If you need to do it, you should set i::FLAG_min_preparse_length = 0; instead > (and add a comment). 1) Yes --- because materialized literals counts were not set during preparsing but were during parsing, some problems appeared when preparsing was used. 2) that seems a lot cleaner, will update to do that.
https://codereview.chromium.org/792083002/diff/40001/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/792083002/diff/40001/test/cctest/test-parsing... test/cctest/test-parsing.cc:4490: "// xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n" But but... RunParserSyncTest already tests preparsing against parsing. What am I missing?
On 2014/12/11 14:01:17, marja wrote: > https://codereview.chromium.org/792083002/diff/40001/test/cctest/test-parsing.cc > File test/cctest/test-parsing.cc (right): > > https://codereview.chromium.org/792083002/diff/40001/test/cctest/test-parsing... > test/cctest/test-parsing.cc:4490: "// > xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n" > But but... RunParserSyncTest already tests preparsing against parsing. What am I > missing? hmmm --- well lets a fair question --- without the parser fixes, the test does fail even without the flag, so I guess it's not needed.
On 2014/12/11 14:04:46, caitp wrote: > On 2014/12/11 14:01:17, marja wrote: > > > https://codereview.chromium.org/792083002/diff/40001/test/cctest/test-parsing.cc > > File test/cctest/test-parsing.cc (right): > > > > > https://codereview.chromium.org/792083002/diff/40001/test/cctest/test-parsing... > > test/cctest/test-parsing.cc:4490: "// > > xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n" > > But but... RunParserSyncTest already tests preparsing against parsing. What am > I > > missing? > > hmmm --- well lets a fair question --- without the parser fixes, the test does > fail even without the flag, so I guess it's not needed. In real life (tm) we make decision to preparse or not to preparse based on code size. In the test we force preparsing no matter what.
lgtm modulo my previous formatting nit
lgtm
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/792083002/120001
LGTM Thanks for fixing. https://codereview.chromium.org/792083002/diff/120001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/792083002/diff/120001/src/preparser.h#newcode762 src/preparser.h:762: kNoTemplateTagExpression)); Is this the formatting `git cl format` gives you? It looks pretty strange to me. https://codereview.chromium.org/792083002/diff/120001/src/preparser.h#newcode... src/preparser.h:1415: // Emulate generation of array literals for tag callsite This comment goes better with PreParserTraits::MaterializeTemplateCallsiteLiterals
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
https://codereview.chromium.org/792083002/diff/120001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/792083002/diff/120001/src/preparser.h#newcode762 src/preparser.h:762: kNoTemplateTagExpression)); On 2014/12/11 15:29:38, arv wrote: > Is this the formatting `git cl format` gives you? It looks pretty strange to me. clang-formatter doesn't complain about this because it matches the styleguide, I'm not sure if it would have produced something different though |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
