New try: Parser: Delay internalizing strings and values

New try: Parser: Delay internalizing strings and values

This is a reincarnation of r21841.

The previous try was https://codereview.chromium.org/314603004/ but it regressed
JSBench and morejs.

BUG=
R=rossberg@chromium.org

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

[marja, 2014-06-20 08:41:24]

rossberg@, ptal

Here's a new try of https://codereview.chromium.org/314603004 (I reverted the
previous try).

For your convenience, the final version of
https://codereview.chromium.org/314603004 is patch set 1, so you can just review
the changes.

Changes compared to the previous version:
- Added a possibility for AstString to be a cons string. This is important,
otherwise FuncNameInferrer will end up internalizing a ton of strings
(foo.bar.baz, foo.bar.quux) separately, and that's not good for the memory
consumption (and GC gets triggered and that affects the run time... -> horror).
- Rewrote the logic in FuncNameInferrer; now it matches the original logic
before this internalization work.
- Put AstStrings into the Zone instead of this weird "Collector of vectors".
It's more elegant that way.
- Added some ASSERTs in ast.h
- Fixed TestScanRegExp (we now need the Zone where to put the string)
- When passing Vector, pass const Vector& instead
- Minor variable renaming in ast-value-factory.h.

[marja, 2014-06-20 14:11:46]

Additional fix (patch set 4)
- Don't hash strings twice, and get rid of the self-baked hash implementation
(copied from preparse data).

[marja, 2014-06-20 14:57:38]

And the final efficiency fix:
- Create the string constants on demand. This is important, because we create a
ton of AstValueFactories when compiling different parts of the code, and not all
string constants are needed in all of them.

Now the efficiency is on par with bleedging_edge with JSBench, at least on local
runs. Phiew!

[rossberg, 2014-06-23 09:44:27]

Great work, I like it. Some comments, though.

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

https://codereview.chromium.org/335293004/diff/80001/src/ast-value-factory.h#...
src/ast-value-factory.h:281: const AstString* GetOneByteString(const
Vector<const uint8_t>& literal);
Hm, isn't the idea of the Vector abstraction that this shouldn't be necessary?

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

https://codereview.chromium.org/335293004/diff/130035/src/ast-value-factory.h...
src/ast-value-factory.h:60: // Creates a cons string. The only allowed
operations for cons strings are
Disallowing a whole set of operations in some cases is kind of nasty, as is
always using space for the cross product of both possible representations.

Do you think it would be possible to have the two different forms of strings as
concrete subclasses of (an abstract) AstString (say AstConsString and
AstRawString?). Then you can get rid of the type thingy and a bunch of
associated asserts, potentially use virtual dispatch for the common functions,
and checked downcasts to get to the representation-specific ones.

https://codereview.chromium.org/335293004/diff/130035/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/335293004/diff/130035/test/cctest/test-parsin...
test/cctest/test-parsing.cc:801: i::AstValueFactory ast_value_factory(&zone, 0);
Why is a hash seed of 0 okay here? Maybe add a comment.

[marja, 2014-06-23 11:46:45]

Thanks for comments!

One additional fix: I made AstString ctors private, since they're only meant to
be used by AstValueFactory (which is already a friend).

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

https://codereview.chromium.org/335293004/diff/80001/src/ast-value-factory.h#...
src/ast-value-factory.h:281: const AstString* GetOneByteString(const
Vector<const uint8_t>& literal);
On 2014/06/23 09:44:27, rossberg wrote:
> Hm, isn't the idea of the Vector abstraction that this shouldn't be necessary?

It's not a huge difference; this will copy the length and the data pointer (not
the actual data); I just got a comment in another code review that I should use
const Vector&; and why not... that's copying one pointer instead of 2.

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

https://codereview.chromium.org/335293004/diff/130035/src/ast-value-factory.h...
src/ast-value-factory.h:60: // Creates a cons string. The only allowed
operations for cons strings are
On 2014/06/23 09:44:27, rossberg wrote:
> Disallowing a whole set of operations in some cases is kind of nasty, as is
> always using space for the cross product of both possible representations.
> 
> Do you think it would be possible to have the two different forms of strings
as
> concrete subclasses of (an abstract) AstString (say AstConsString and
> AstRawString?). Then you can get rid of the type thingy and a bunch of
> associated asserts, potentially use virtual dispatch for the common functions,
> and checked downcasts to get to the representation-specific ones.

Done.

I named the classes AstStringBase, AstString (the same as AstString used to be
before) and AstConsString.

Most places use AstStrings (like before), only function names are stored as
AstStringBase. We don't need to downcast ever. The only needed operations for
cons strings are string() and length(), and AstStringBase takes care of them.

https://codereview.chromium.org/335293004/diff/130035/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/335293004/diff/130035/test/cctest/test-parsin...
test/cctest/test-parsing.cc:801: i::AstValueFactory ast_value_factory(&zone, 0);
On 2014/06/23 09:44:27, rossberg wrote:
> Why is a hash seed of 0 okay here? Maybe add a comment.

Why wouldn't it be? Afaics, it's usually 0, except when FLAG_randomize_hashes is
there.

[marja, 2014-06-23 12:16:18]

https://codereview.chromium.org/335293004/diff/130035/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/335293004/diff/130035/test/cctest/test-parsin...
test/cctest/test-parsing.cc:801: i::AstValueFactory ast_value_factory(&zone, 0);
On 2014/06/23 11:46:45, marja wrote:
> On 2014/06/23 09:44:27, rossberg wrote:
> > Why is a hash seed of 0 okay here? Maybe add a comment.
> 
> Why wouldn't it be? Afaics, it's usually 0, except when FLAG_randomize_hashes
is
> there.

I changed this to get the hash seed from the isolate.

[rossberg, 2014-06-23 13:30:53]

Thanks, like it, though I'm not so sure about the naming...

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

https://codereview.chromium.org/335293004/diff/80001/src/ast-value-factory.h#...
src/ast-value-factory.h:281: const AstString* GetOneByteString(const
Vector<const uint8_t>& literal);
On 2014/06/23 11:46:45, marja wrote:
> On 2014/06/23 09:44:27, rossberg wrote:
> > Hm, isn't the idea of the Vector abstraction that this shouldn't be
necessary?
> 
> It's not a huge difference; this will copy the length and the data pointer
(not
> the actual data); I just got a comment in another code review that I should
use
> const Vector&; and why not... that's copying one pointer instead of 2.

But you pay with an extra indirection for all uses. Consequently, this is not
only a micro optimisation, it's one that could actually cost micro performance.
;)

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

https://codereview.chromium.org/335293004/diff/130035/src/ast-value-factory.h...
src/ast-value-factory.h:60: // Creates a cons string. The only allowed
operations for cons strings are
On 2014/06/23 11:46:45, marja wrote:
> On 2014/06/23 09:44:27, rossberg wrote:
> > Disallowing a whole set of operations in some cases is kind of nasty, as is
> > always using space for the cross product of both possible representations.
> > 
> > Do you think it would be possible to have the two different forms of strings
> as
> > concrete subclasses of (an abstract) AstString (say AstConsString and
> > AstRawString?). Then you can get rid of the type thingy and a bunch of
> > associated asserts, potentially use virtual dispatch for the common
functions,
> > and checked downcasts to get to the representation-specific ones.
> 
> Done.
> 
> I named the classes AstStringBase, AstString (the same as AstString used to be
> before) and AstConsString.
> 
> Most places use AstStrings (like before), only function names are stored as
> AstStringBase.

Hm, from a stylistic point, I don't like naming an abstract interface
something-Base -- that's usually reserved for pure implementation inheritance,
not interface inheritance.

Would renaming AstString->AstRawString, AstStringBase->AstString be particularly
terrible?


> We don't need to downcast ever. The only needed operations for
> cons strings are string() and length(), and AstStringBase takes care of them.

Ah, even better then.

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

https://codereview.chromium.org/335293004/diff/230001/src/ast-value-factory.c...
src/ast-value-factory.cc:129: // AstStrings are internalized before
AstConsStrings so left and right are
Maybe ASSERT that here?

[marja, 2014-06-23 14:56:42]

The only changes in the patch set:
- Renaming: AstString -> AstRawString, AstStringBase->AstString
- clang-formatting lines which got too long because of that
- const Vector& -> Vector in ast-value-factory.cc/h

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

https://codereview.chromium.org/335293004/diff/80001/src/ast-value-factory.h#...
src/ast-value-factory.h:281: const AstString* GetOneByteString(const
Vector<const uint8_t>& literal);
On 2014/06/23 13:30:52, rossberg wrote:
> On 2014/06/23 11:46:45, marja wrote:
> > On 2014/06/23 09:44:27, rossberg wrote:
> > > Hm, isn't the idea of the Vector abstraction that this shouldn't be
> necessary?
> > 
> > It's not a huge difference; this will copy the length and the data pointer
> (not
> > the actual data); I just got a comment in another code review that I should
> use
> > const Vector&; and why not... that's copying one pointer instead of 2.
> 
> But you pay with an extra indirection for all uses. Consequently, this is not
> only a micro optimisation, it's one that could actually cost micro
performance.
> ;)

As you wish. (Changed back to Vector.)

[rossberg, 2014-06-23 15:02:32]

LGTM, up to the one comment below that you probably missed. :)

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

https://codereview.chromium.org/335293004/diff/230001/src/ast-value-factory.c...
src/ast-value-factory.cc:129: // AstStrings are internalized before
AstConsStrings so left and right are
On 2014/06/23 13:30:53, rossberg wrote:
> Maybe ASSERT that here?

This one.

[marja, 2014-06-23 15:29:42]

On 2014/06/23 15:02:32, rossberg wrote:
> LGTM, up to the one comment below that you probably missed. :)
> 
> https://codereview.chromium.org/335293004/diff/230001/src/ast-value-factory.cc
> File src/ast-value-factory.cc (right):
> 
>
https://codereview.chromium.org/335293004/diff/230001/src/ast-value-factory.c...
> src/ast-value-factory.cc:129: // AstStrings are internalized before
> AstConsStrings so left and right are
> On 2014/06/23 13:30:53, rossberg wrote:
> > Maybe ASSERT that here?
> 
> This one.

string() already asserts. I was calling string() on left_ and right_ with the
purpose of getting this assert asserted.

Is that okay?

[danno, 2014-06-23 16:08:49]

On 2014/06/23 15:29:42, marja wrote:
> On 2014/06/23 15:02:32, rossberg wrote:
> > LGTM, up to the one comment below that you probably missed. :)
> > 
> >
https://codereview.chromium.org/335293004/diff/230001/src/ast-value-factory.cc
> > File src/ast-value-factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/335293004/diff/230001/src/ast-value-factory.c...
> > src/ast-value-factory.cc:129: // AstStrings are internalized before
> > AstConsStrings so left and right are
> > On 2014/06/23 13:30:53, rossberg wrote:
> > > Maybe ASSERT that here?
> > 
> > This one.
> 
> string() already asserts. I was calling string() on left_ and right_ with the
> purpose of getting this assert asserted.
> 
> Is that okay?

Very cool Marja, nicely done!

[marja, 2014-06-24 14:04:48]

Committed patchset #11 manually as r21972 (presubmit successful).

[marja, 2014-06-24 14:08:12]

The committed patch set contains a trivial fix in
AstRawStringInternalizationKey. rossberg@, you don't necessarily need to have a
look at it, unless you want to see how silly code I wrote.