Changes to shrink the token stream representation from two words to one word.

Changes to shrink the token stream representation from two words to one word.
On standalone dartium which has about 50000 tokens in the libraries (core, coreimpl, builtin, io etc.) this reduces the heap size by about 180kb. The size of the standalone snapshot buffer reduces by about 72kb.

Also moved the keyword symbol table to the object store so that it does not have to repopulated on every script compilation. Should benefit regular user script load times.
Committed: https://code.google.com/p/dart/source/detail?r=4734

[hausner, 2012-02-25 00:41:57]

LGTM w/ comments.

Additional question: you calculated the space savings by just measuring how much
smaller the token stream objects are. But you should subtract from that number
the space that is needed for the TokenLiteral objects and the canonicalization
list. Are we really saving any space?

http://codereview.chromium.org/9462003/diff/1040/vm/object.cc
File vm/object.cc (right):

http://codereview.chromium.org/9462003/diff/1040/vm/object.cc#newcode3798
vm/object.cc:3798: while (index < canonical_list_len) {
This linear search might become a bottleneck once we load large libraries.

http://codereview.chromium.org/9462003/diff/1040/vm/object.cc#newcode3805
vm/object.cc:3805: if (literal_value.Equals(literal, 0, literal.Length())) {
As discussed in person: no need to call Equals, you can do a pointer comparison.

http://codereview.chromium.org/9462003/diff/1040/vm/object.h
File vm/object.h (right):

http://codereview.chromium.org/9462003/diff/1040/vm/object.h#newcode1618
vm/object.h:1618: (index * RawTokenStream::kNumberOfEntries) + entry_offset;
Does it still make sense to perform this computation when we now only have one
entry per token, i.e. kNumberOfEntries == 1? I think this can be eliminated.

http://codereview.chromium.org/9462003/diff/1040/vm/object_store.cc
File vm/object_store.cc (right):

http://codereview.chromium.org/9462003/diff/1040/vm/object_store.cc#newcode111
vm/object_store.cc:111: if (keywords.IsNull()) {
Could you move the creation of this table to the isolate initialization code?
That way you save a null check on this somewhat critical path.

It would also be nice if the keyword array were accessible to the scanner so it
can cache a handle to the array locally, saving a handle creation at each table
access.

http://codereview.chromium.org/9462003/diff/1040/vm/raw_object.h
File vm/raw_object.h (right):

http://codereview.chromium.org/9462003/diff/1040/vm/raw_object.h#newcode538
vm/raw_object.h:538: RawString* literal_;  // The actual token string.
// literal characters as they appear in source text.

http://codereview.chromium.org/9462003/diff/1040/vm/scanner.cc
File vm/scanner.cc (right):

http://codereview.chromium.org/9462003/diff/1040/vm/scanner.cc#newcode263
vm/scanner.cc:263: String& symbol = String::ZoneHandle();
NB: for each keyword occurrence in the source text we now call Isolate::Current
and allocate a zone handle. Before, we only allocated one handle for each
keyword kind. Hopefully not a big deal.

[siva, 2012-02-28 19:57:31]

Thanks for the review, I have addressed the comments, please take another look.

The space saving computation is based on the numbers,
We have 50000 tokens in corelib, coreimpllib, builtin, io etc.

Out of these 50000 only 1966 are literal tokens and these tokens result in a 2x
increase so I subtracted 5000 tokens from 50000 as the ones for which there are
no space savings
for the remaining 45000 tokens we save 4 bytes on each of them resulting in the
180k number. The snapshot space reduction is a direct computation from the
snapshot buffer size.

https://chromiumcodereview.appspot.com/9462003/diff/1040/vm/object.cc
File vm/object.cc (right):

https://chromiumcodereview.appspot.com/9462003/diff/1040/vm/object.cc#newcode...
vm/object.cc:3798: while (index < canonical_list_len) {
I removed this canonicalization step, the literal tokens are not canonicalized.
I think we need to evaluate further as to whether there is any benefit from
canonicalizing these. 

On 2012/02/25 00:41:57, hausner wrote:
> This linear search might become a bottleneck once we load large libraries.

https://chromiumcodereview.appspot.com/9462003/diff/1040/vm/object.cc#newcode...
vm/object.cc:3805: if (literal_value.Equals(literal, 0, literal.Length())) {
This code has been removed.

On 2012/02/25 00:41:57, hausner wrote:
> As discussed in person: no need to call Equals, you can do a pointer
comparison.

https://chromiumcodereview.appspot.com/9462003/diff/1040/vm/object.h
File vm/object.h (right):

https://chromiumcodereview.appspot.com/9462003/diff/1040/vm/object.h#newcode1618
vm/object.h:1618: (index * RawTokenStream::kNumberOfEntries) + entry_offset;
I added an ASSERT(kNumberofEntries == 1);
and eliminated the computation.

On 2012/02/25 00:41:57, hausner wrote:
> Does it still make sense to perform this computation when we now only have one
> entry per token, i.e. kNumberOfEntries == 1? I think this can be eliminated.

https://chromiumcodereview.appspot.com/9462003/diff/1040/vm/object_store.cc
File vm/object_store.cc (right):

https://chromiumcodereview.appspot.com/9462003/diff/1040/vm/object_store.cc#n...
vm/object_store.cc:111: if (keywords.IsNull()) {
On 2012/02/25 00:41:57, hausner wrote:
> Could you move the creation of this table to the isolate initialization code?
> That way you save a null check on this somewhat critical path.
> 
> It would also be nice if the keyword array were accessible to the scanner so
it
> can cache a handle to the array locally, saving a handle creation at each
table
> access.

Done.

https://chromiumcodereview.appspot.com/9462003/diff/1040/vm/raw_object.h
File vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/9462003/diff/1040/vm/raw_object.h#newc...
vm/raw_object.h:538: RawString* literal_;  // The actual token string.
On 2012/02/25 00:41:57, hausner wrote:
> // literal characters as they appear in source text.

Done.

https://chromiumcodereview.appspot.com/9462003/diff/1040/vm/scanner.cc
File vm/scanner.cc (right):

https://chromiumcodereview.appspot.com/9462003/diff/1040/vm/scanner.cc#newcod...
vm/scanner.cc:263: String& symbol = String::ZoneHandle();
I have redone this code a bit to address the concern you had about handle
creation, please take another look.

On 2012/02/25 00:41:57, hausner wrote:
> NB: for each keyword occurrence in the source text we now call
Isolate::Current
> and allocate a zone handle. Before, we only allocated one handle for each
> keyword kind. Hopefully not a big deal.

[hausner, 2012-02-29 00:22:37]

LEBTM.

https://chromiumcodereview.appspot.com/9462003/diff/1040/vm/scanner.cc
File vm/scanner.cc (right):

https://chromiumcodereview.appspot.com/9462003/diff/1040/vm/scanner.cc#newcod...
vm/scanner.cc:263: String& symbol = String::ZoneHandle();
Nice, thank you.

On 2012/02/28 19:57:31, asiva wrote:
> I have redone this code a bit to address the concern you had about handle
> creation, please take another look.
> 
> On 2012/02/25 00:41:57, hausner wrote:
> > NB: for each keyword occurrence in the source text we now call
> Isolate::Current
> > and allocate a zone handle. Before, we only allocated one handle for each
> > keyword kind. Hopefully not a big deal. 
>

https://chromiumcodereview.appspot.com/9462003/diff/10001/vm/object.h
File vm/object.h (right):

https://chromiumcodereview.appspot.com/9462003/diff/10001/vm/object.h#newcode...
vm/object.h:1619: ASSERT(RawTokenStream::kNumberOfEntries == 1);
Seems like a high price to pay for this assertion. This code is executed many
times. Or is the compiler smart enough (in debug mode) to eliminate the assert,
since the condition is a compile time constant? Do we have a compile time
assertion macro? Or maybe just get rid of the kNumberOfEntries  and its
enumeration completely?

[siva, 2012-02-29 02:07:55]

Thanks.

https://chromiumcodereview.appspot.com/9462003/diff/10001/vm/object.h
File vm/object.h (right):

https://chromiumcodereview.appspot.com/9462003/diff/10001/vm/object.h#newcode...
vm/object.h:1619: ASSERT(RawTokenStream::kNumberOfEntries == 1);
On 2012/02/29 00:22:37, hausner wrote:
> Seems like a high price to pay for this assertion. This code is executed many
> times. Or is the compiler smart enough (in debug mode) to eliminate the
assert,
> since the condition is a compile time constant? Do we have a compile time
> assertion macro? Or maybe just get rid of the kNumberOfEntries  and its
> enumeration completely?

Done (i.e. got rid of the enum).