Parser: Delay internalizing strings and values.

Parser: Delay internalizing strings and values.

This is needed so that we can run Parser on a non-main thread (independent
of the Isolate and the V8 heap).

BUG=
R=rossberg@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21841

[marja, 2014-06-03 09:28:11]

(rossberg, this is for you)

(ulan, mstarzinger, dcarney, jochen fyi)

Here's a new version of the "delay internalizing strings". I'd like another
round of comments...

cctest, mjsunit, webkit, mozilla and test262 pass. I'm planning to add more
tests for function name inferring.

Speed-wise it's a bit slower than baseline (5-7% on my non-lazy test data, both
on mobile and on desktop). That small regression shouldn't regress any
benchmarks.

[rossberg, 2014-06-11 14:58:13]

Looks mostly good. My only concern is with the name AstStringTable and its
construction API for AstValues.

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc
File src/ast-string-table.cc (right):

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc#newc...
src/ast-string-table.cc:71: if (!is_one_byte_) return false;
Merge into next if?

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc#newc...
src/ast-string-table.cc:145: default:
Don't use a default, so that you can get compiler warnings.

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc#newc...
src/ast-string-table.cc:149: return false;
Have the UNREACHABLE here.

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc#newc...
src/ast-string-table.cc:198: default:
Same here.

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h
File src/ast-string-table.h (right):

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newco...
src/ast-string-table.h:28: #ifndef V8_AST_STRING_TABLE_H_
How about renaming this module to AstValue, since strings are just one facet
now.

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newco...
src/ast-string-table.h:56: // The string is not NULL-terminated, use length() to
find out the length.
Nit: null-terminated, since it's not NULL the pointer.

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newco...
src/ast-string-table.h:92: enum Type {
Can we add the missing New* constructors below and make this enum private?

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newco...
src/ast-string-table.h:175: explicit AstValue(Type t) :
...and get rid of this one.

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newco...
src/ast-string-table.h:250: const AstValue* NewValue(const AstString* string);
It's weird that these functions are all members of AstStringTable. Rename this
to AstValueFactory or s.th like that?

Also, I'd prefer to name all these NewSomething consistently, for Something in
String, Number, Boolean, Undefined, etc. The Value suffix seems redundant once
these functions are in a proper place.

https://codereview.chromium.org/314603004/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode669
src/ast.h:669: int pos)
Nit: this should fit on prev line

https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode2347
src/ast.h:2347: inferred_name_ = Handle<String>();
Hm, can't you ASSERT that this is already the case?

https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode3028
src/ast.h:3028: const AstString* name, int pos) {
Nit: fits prev line

https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode3156
src/ast.h:3156: Literal* NewLiteral(const AstString* string, int pos) {
I think we should name all these `NewSomethingLiteral`.

https://codereview.chromium.org/314603004/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/314603004/diff/1/src/parser.h#newcode522
src/parser.h:522: const char* char_arg = NULL,
Nit: char_arg -> arg

https://codereview.chromium.org/314603004/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/314603004/diff/1/src/preparser.h#newcode152
src/preparser.h:152: typename Traits::Type::Zone* extra_param = NULL,
extra_param??

https://codereview.chromium.org/314603004/diff/1/src/prettyprinter.h
File src/prettyprinter.h (right):

https://codereview.chromium.org/314603004/diff/1/src/prettyprinter.h#newcode49
src/prettyprinter.h:49: void PrintLiteral(Handle<Object> value, bool quote);
Maybe unify these two into one PrintValue(AstValue*)?

https://codereview.chromium.org/314603004/diff/1/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/314603004/diff/1/src/scopes.cc#newcode43
src/scopes.cc:43: Entry* p = ZoneHashMap::Lookup(const_cast<AstString*>(name),
name->hash(),
Would it be possible to avoid this ugly casting by fixing the type of Lookup
itself?

[marja, 2014-06-12 09:57:54]

Thanks for comments! rossberg@, could you take another look?

The first new patch set is just rebasing, no interesting changes.

The second new patch set contains the code review modifications.

I also detected a bug with the FuncNameInferrer tests I had just added an fixed
it as a separate patch set.

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc
File src/ast-string-table.cc (right):

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc#newc...
src/ast-string-table.cc:71: if (!is_one_byte_) return false;
On 2014/06/11 14:58:12, rossberg wrote:
> Merge into next if?

Done.

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc#newc...
src/ast-string-table.cc:145: default:
On 2014/06/11 14:58:12, rossberg wrote:
> Don't use a default, so that you can get compiler warnings.

Done.

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc#newc...
src/ast-string-table.cc:149: return false;
On 2014/06/11 14:58:12, rossberg wrote:
> Have the UNREACHABLE here.

Done.

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.cc#newc...
src/ast-string-table.cc:198: default:
On 2014/06/11 14:58:12, rossberg wrote:
> Same here.

Done (except now I cannot have "UNREACHABLE()" at the bottom, because the cases
don't return).

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h
File src/ast-string-table.h (right):

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newco...
src/ast-string-table.h:28: #ifndef V8_AST_STRING_TABLE_H_
On 2014/06/11 14:58:12, rossberg wrote:
> How about renaming this module to AstValue, since strings are just one facet
> now.

Done (as discussed, renamed AstStringTable to AstValueFactory, and it takes care
of both AstStrings and AstValues).

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newco...
src/ast-string-table.h:56: // The string is not NULL-terminated, use length() to
find out the length.
On 2014/06/11 14:58:12, rossberg wrote:
> Nit: null-terminated, since it's not NULL the pointer.

Done.

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newco...
src/ast-string-table.h:92: enum Type {
On 2014/06/11 14:58:12, rossberg wrote:
> Can we add the missing New* constructors below and make this enum private?

Done.

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newco...
src/ast-string-table.h:175: explicit AstValue(Type t) :
On 2014/06/11 14:58:12, rossberg wrote:
> ...and get rid of this one.

No, I still need this one to be able to construct them. It'll be only used by
AstValueFactory::NewUndefined() etc., so not visible to outside (Parser or
such).

https://codereview.chromium.org/314603004/diff/1/src/ast-string-table.h#newco...
src/ast-string-table.h:250: const AstValue* NewValue(const AstString* string);
On 2014/06/11 14:58:12, rossberg wrote:
> It's weird that these functions are all members of AstStringTable. Rename this
> to AstValueFactory or s.th like that?
> 
> Also, I'd prefer to name all these NewSomething consistently, for Something in
> String, Number, Boolean, Undefined, etc. The Value suffix seems redundant once
> these functions are in a proper place.

Done.

https://codereview.chromium.org/314603004/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode669
src/ast.h:669: int pos)
On 2014/06/11 14:58:12, rossberg wrote:
> Nit: this should fit on prev line

Done.

https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode2347
src/ast.h:2347: inferred_name_ = Handle<String>();
On 2014/06/11 14:58:12, rossberg wrote:
> Hm, can't you ASSERT that this is already the case?

Done (note that inferred_name() above already ASSERTed that only one of the
names is something meaningful.

https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode3028
src/ast.h:3028: const AstString* name, int pos) {
On 2014/06/11 14:58:12, rossberg wrote:
> Nit: fits prev line

Done.

https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode3156
src/ast.h:3156: Literal* NewLiteral(const AstString* string, int pos) {
On 2014/06/11 14:58:12, rossberg wrote:
> I think we should name all these `NewSomethingLiteral`.

Done. And I named the one which takes an AstValue* "NewSpecialLiteral". (It's
used for null, undefined and the hole.)

https://codereview.chromium.org/314603004/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/314603004/diff/1/src/parser.h#newcode522
src/parser.h:522: const char* char_arg = NULL,
On 2014/06/11 14:58:12, rossberg wrote:
> Nit: char_arg -> arg

Done.

https://codereview.chromium.org/314603004/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/314603004/diff/1/src/preparser.h#newcode152
src/preparser.h:152: typename Traits::Type::Zone* extra_param = NULL,
On 2014/06/11 14:58:12, rossberg wrote:
> extra_param??

Oops, this was unintentional. Fixed. It's actually called unconsistently
"extra_param" and "zone" in other places, but I don't want to touch that in this
CL...

https://codereview.chromium.org/314603004/diff/1/src/prettyprinter.h
File src/prettyprinter.h (right):

https://codereview.chromium.org/314603004/diff/1/src/prettyprinter.h#newcode49
src/prettyprinter.h:49: void PrintLiteral(Handle<Object> value, bool quote);
On 2014/06/11 14:58:13, rossberg wrote:
> Maybe unify these two into one PrintValue(AstValue*)?

I can't, since not all the strings to be printed are inside AstValues. For
example node->proxy()->name() is an AstString*, a label is an AstString*.

What I could do is to change the one that takes Handle<Object> value to take an
AstValue* and dispatch based on its type, but that's a bit unnecessary change.

https://codereview.chromium.org/314603004/diff/1/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/314603004/diff/1/src/scopes.cc#newcode43
src/scopes.cc:43: Entry* p = ZoneHashMap::Lookup(const_cast<AstString*>(name),
name->hash(),
On 2014/06/11 14:58:13, rossberg wrote:
> Would it be possible to avoid this ugly casting by fixing the type of Lookup
> itself?

I tried it, but it's a bit messy. I'd rather not do it on this CL. I added a
FIXME here to do a follow-up.

[rossberg, 2014-06-12 12:20:29]

LGTM, module a few nits.

https://codereview.chromium.org/314603004/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/314603004/diff/1/src/ast.h#newcode3156
src/ast.h:3156: Literal* NewLiteral(const AstString* string, int pos) {
On 2014/06/12 09:57:53, marja wrote:
> On 2014/06/11 14:58:12, rossberg wrote:
> > I think we should name all these `NewSomethingLiteral`.
> 
> Done. And I named the one which takes an AstValue* "NewSpecialLiteral". (It's
> used for null, undefined and the hole.)

I'd prefer to just have NewNull, NewUndefined, NewHole instead (cf. comments
below).

https://codereview.chromium.org/314603004/diff/140001/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/314603004/diff/140001/src/ast-value-factory.h...
src/ast-value-factory.h:188: type_(t) {}
Perhaps ASSERT that t in {NULL, UNDEFINED, HOLE}.

https://codereview.chromium.org/314603004/diff/140001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/314603004/diff/140001/src/parser.cc#newcode692
src/parser.cc:692: return factory->NewSpecialLiteral(
Nit: just for symmetry, I would wrap this up into a NewNullLiteral.

https://codereview.chromium.org/314603004/diff/140001/src/parser.cc#newcode734
src/parser.cc:734: return factory->NewSpecialLiteral(
...and similarly here.

https://codereview.chromium.org/314603004/diff/140001/src/parser.cc#newcode3673
src/parser.cc:3673: Expression* undefined = factory()->NewSpecialLiteral(
...and here.

https://codereview.chromium.org/314603004/diff/140001/src/parser.cc#newcode3769
src/parser.cc:3769: return
factory()->NewSpecialLiteral(ast_value_factory_->NewUndefined(),
...and here.

[marja, 2014-06-12 12:35:04]

Wohoo, thanks for reviewing!

I lifted the New(Null,Undefined,TheHole) one level up as you suggested and
removed NewSpecialLiteral.

I also clang-formatted some constructors in ast-value-factory.h which were
formatted weirdly before.

https://codereview.chromium.org/314603004/diff/140001/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/314603004/diff/140001/src/ast-value-factory.h...
src/ast-value-factory.h:188: type_(t) {}
On 2014/06/12 12:20:28, rossberg wrote:
> Perhaps ASSERT that t in {NULL, UNDEFINED, HOLE}.

Done.

https://codereview.chromium.org/314603004/diff/140001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/314603004/diff/140001/src/parser.cc#newcode692
src/parser.cc:692: return factory->NewSpecialLiteral(
On 2014/06/12 12:20:28, rossberg wrote:
> Nit: just for symmetry, I would wrap this up into a NewNullLiteral.

Done.

https://codereview.chromium.org/314603004/diff/140001/src/parser.cc#newcode734
src/parser.cc:734: return factory->NewSpecialLiteral(
On 2014/06/12 12:20:28, rossberg wrote:
> ...and similarly here.

Done.

https://codereview.chromium.org/314603004/diff/140001/src/parser.cc#newcode3673
src/parser.cc:3673: Expression* undefined = factory()->NewSpecialLiteral(
On 2014/06/12 12:20:28, rossberg wrote:
> ...and here.

Done.

https://codereview.chromium.org/314603004/diff/140001/src/parser.cc#newcode3769
src/parser.cc:3769: return
factory()->NewSpecialLiteral(ast_value_factory_->NewUndefined(),
On 2014/06/12 12:20:28, rossberg wrote:
> ...and here.

Done.

[rossberg, 2014-06-12 12:46:41]

Thanks. Still LGTM, just one more nit. :)

https://codereview.chromium.org/314603004/diff/160001/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/314603004/diff/160001/src/ast-value-factory.h...
src/ast-value-factory.h:177: ASSERT(t == AstValue::NULL_TYPE || t ==
AstValue::UNDEFINED ||
Nit: you shouldn't need the AstValue:: qualification here.

[marja, 2014-06-12 12:48:06]

https://codereview.chromium.org/314603004/diff/160001/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/314603004/diff/160001/src/ast-value-factory.h...
src/ast-value-factory.h:177: ASSERT(t == AstValue::NULL_TYPE || t ==
AstValue::UNDEFINED ||
On 2014/06/12 12:46:41, rossberg wrote:
> Nit: you shouldn't need the AstValue:: qualification here.

Done.

[marja, 2014-06-13 09:41:32]

rossberg@,

I had to do a non-trivial rebase because
https://codereview.chromium.org/332663004 got committed first.

So I'd need to internalize Symbol (as in, JavaScript symbols). There is only one
type of Symbol produced by parsing as of now, so it's kinda hard to see what's
the most elegant design (enum for symbols? NewFooSymbolLiteral for all possible
symbols?). So I went with storing the symbol name as const char* and dispatching
based on that.

[rossberg, 2014-06-13 13:00:42]

lgtm

https://codereview.chromium.org/314603004/diff/220001/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/314603004/diff/220001/src/ast-value-factory.h...
src/ast-value-factory.h:159: SYMBOL
Nitty nit: move up next to string

https://codereview.chromium.org/314603004/diff/240001/src/ast-value-factory.cc
File src/ast-value-factory.cc (right):

https://codereview.chromium.org/314603004/diff/240001/src/ast-value-factory.c...
src/ast-value-factory.cc:199: value_ = Object::GetProperty(
Yeah, I suppose that's fine. I had hoped to keep those variables an
implementation detail of the symbol module (and an unfortunate artefact that
they are actually exposed). But re-calling InternalSymbol instead probably isn't
any nicer.

[marja, 2014-06-13 13:06:01]

thx again!

https://codereview.chromium.org/314603004/diff/220001/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/314603004/diff/220001/src/ast-value-factory.h...
src/ast-value-factory.h:159: SYMBOL
On 2014/06/13 13:00:41, rossberg wrote:
> Nitty nit: move up next to string

Done.

[marja, 2014-06-13 13:32:24]

Committed patchset #12 manually as r21841 (presubmit successful).