More compact string representation on 64 bit.

More compact string representation on 64 bit.

This moves the hash code into the header word for strings on 64 bit
platforms.  With the old layout, 9 character strings became 48-byte
objects.  With the new layout you have to go to 17 characters before
you are bumped from 4 to 6 words (32 to 48 bytes).

As a side effect, the class ID field is now 16 bits on all platforms
instead of having two different sizes, and the size field is 8 bits
on all platforms.

This also paves the way for moving the hash code for instance objects
into the header, so we won't need the side-lookup in the
hash-table-of-hash-codes on 64 bit platforms.

R=vegorov@google.com
BUG=

Committed: https://github.com/dart-lang/sdk/commit/5c8e472c82f8c403b3e628a40ca0be5fbefa7706

[Vyacheslav Egorov (Google), 2017-05-18 06:14:43]

lgtm

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/intrinsifier_arm...
File runtime/vm/intrinsifier_arm64.cc (right):

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/intrinsifier_arm...
runtime/vm/intrinsifier_arm64.cc:1827: __ SmiTag(R0);
if you use something that sets flags for tagging you should be able to detect 0
without comparison. (here and everywhere in similar places)

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/intrinsifier_arm...
runtime/vm/intrinsifier_arm64.cc:2076: __ AddImmediate(length_reg, 1);
maybe conditional add() here instead of branching?

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/intrinsifier_x64.cc
File runtime/vm/intrinsifier_x64.cc (right):

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/intrinsifier_x64...
runtime/vm/intrinsifier_x64.cc:1750: __ SmiTag(RAX);
you can probably:

__ SmiTag(RAX)
__ j(ZERO, ...)

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:6795: intptr_t result = GetRawHash();
Maybe call it GetCachedHash(...)?

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:6810: static intptr_t hash_offset() { return kInt32Size; } 
// Wrong for big-endian?
we don't support big-endian afaik

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:17: #define HASH_IN_OBJECT_HEADER 1
maybe this needs to go to globals.h?

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:1833: static RawSmi* New(intptr_t value) {
It's very uncommon to have these here. Especially because it essentially
duplicates Smi::New(...).

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:1894: #if defined(HASH_IN_OBJECT_HEADER)
I think this needs to go to object.h you can have

static ... GetCachedHash(RawString* str)
static  ... SetCachedHash() there

then you also don't need to introduce RawSmi::New

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:1896: void SetRawHash(uintptr_t hash) {
can this be uint32_t hash for consistency with SetRawHash?

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:1898: SetHeaderHash(hash);
I would better call is SetHashInHeader() or SetHashInTags(...)

[erikcorry, 2017-05-18 09:55:18]

Description was changed from

==========
More compact string representation on 64 bit.

This moves the hash code into the header word for strings on 64 bit
platforms.  With the old layout, 9 character strings became 48-byte
objects.  With the new layout you have to go to 17 characters before
you are bumped from 4 to 6 words (32 to 48 bytes).

As a side effect, the class ID field is now 16 bits on all platforms
instead of having two different sizes, and the size field is 8 bits
on all platforms.

This also paves the way for moving the hash code for instance objects
into the header, so we won't need the side-lookup in the
hash-table-of-hash-codes on 64 bit platforms.

R=vegorov@google.com
BUG=
==========

to

==========
More compact string representation on 64 bit.

This moves the hash code into the header word for strings on 64 bit
platforms.  With the old layout, 9 character strings became 48-byte
objects.  With the new layout you have to go to 17 characters before
you are bumped from 4 to 6 words (32 to 48 bytes).

As a side effect, the class ID field is now 16 bits on all platforms
instead of having two different sizes, and the size field is 8 bits
on all platforms.

This also paves the way for moving the hash code for instance objects
into the header, so we won't need the side-lookup in the
hash-table-of-hash-codes on 64 bit platforms.

R=vegorov@google.com
BUG=

Committed:
https://github.com/dart-lang/sdk/commit/5c8e472c82f8c403b3e628a40ca0be5fbefa7706
==========

[erikcorry, 2017-05-18 09:55:49]

Committed patchset #2 (id:20001) manually as
5c8e472c82f8c403b3e628a40ca0be5fbefa7706 (presubmit successful).

[erikcorry, 2017-05-18 14:28:38]

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/intrinsifier_arm...
File runtime/vm/intrinsifier_arm64.cc (right):

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/intrinsifier_arm...
runtime/vm/intrinsifier_arm64.cc:1827: __ SmiTag(R0);
On 2017/05/18 06:14:42, Vyacheslav Egorov (Google) wrote:
> if you use something that sets flags for tagging you should be able to detect
0
> without comparison. (here and everywhere in similar places)

Done.

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/intrinsifier_arm...
runtime/vm/intrinsifier_arm64.cc:2076: __ AddImmediate(length_reg, 1);
On 2017/05/18 06:14:42, Vyacheslav Egorov (Google) wrote:
> maybe conditional add() here instead of branching?

Done.

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/intrinsifier_x64.cc
File runtime/vm/intrinsifier_x64.cc (right):

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/intrinsifier_x64...
runtime/vm/intrinsifier_x64.cc:1750: __ SmiTag(RAX);
On 2017/05/18 06:14:42, Vyacheslav Egorov (Google) wrote:
> you can probably:
> 
> __ SmiTag(RAX)
> __ j(ZERO, ...)

Done.

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:6795: intptr_t result = GetRawHash();
On 2017/05/18 06:14:42, Vyacheslav Egorov (Google) wrote:
> Maybe call it GetCachedHash(...)?

Done.

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:6810: static intptr_t hash_offset() { return kInt32Size; } 
// Wrong for big-endian?
On 2017/05/18 06:14:42, Vyacheslav Egorov (Google) wrote:
> we don't support big-endian afaik

No, but if we ever do, it's nice to leave hints that can be grepped for.

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:17: #define HASH_IN_OBJECT_HEADER 1
On 2017/05/18 06:14:43, Vyacheslav Egorov (Google) wrote:
> maybe this needs to go to globals.h?

Done.

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:1833: static RawSmi* New(intptr_t value) {
On 2017/05/18 06:14:42, Vyacheslav Egorov (Google) wrote:
> It's very uncommon to have these here. Especially because it essentially
> duplicates Smi::New(...).

I managed to move them

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:1894: #if defined(HASH_IN_OBJECT_HEADER)
On 2017/05/18 06:14:43, Vyacheslav Egorov (Google) wrote:
> I think this needs to go to object.h you can have
> 
> static ... GetCachedHash(RawString* str)
> static  ... SetCachedHash() there
> 
> then you also don't need to introduce RawSmi::New

Done.

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:1896: void SetRawHash(uintptr_t hash) {
On 2017/05/18 06:14:43, Vyacheslav Egorov (Google) wrote:
> can this be uint32_t hash for consistency with SetRawHash?

No, then the assert doesn't work.

I want to ensure that the assert catches the case where we generate a hash code
that is out of range, store the truncated version, then return the untruncated
version.

https://codereview.chromium.org/2893553002/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:1898: SetHeaderHash(hash);
On 2017/05/18 06:14:43, Vyacheslav Egorov (Google) wrote:
> I would better call is SetHashInHeader() or SetHashInTags(...)

This no longer exists.

[sra1, 2017-05-18 15:13:13]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2017-05-18 15:13:14]

Is reducing the class-id bits a wise direction?
Our largest app has a hair under 15000 classes. (21k if you closure convert to
classes like dart2js does). This is a browser app.

It is not inconceivable that an server to support such an app would be close to
the 64k limit.

I once worked on a Java server app that could process arbitrary protobufs by
name. There were too many protobuf message kinds to allow that app to be
rewritten in dart with a 64k class limit.

[sra1, 2017-05-18 15:46:22]

On 2017/05/18 15:13:14, sra1 wrote:
> Is reducing the class-id bits a wise direction?
> Our largest app has a hair under 15000 classes. (21k if you closure convert to
> classes like dart2js does). This is a browser app.
> 
> It is not inconceivable that an server to support such an app would be close
to
> the 64k limit.
> 
> I once worked on a Java server app that could process arbitrary protobufs by
> name. There were too many protobuf message kinds to allow that app to be
> rewritten in dart with a 64k class limit.

It would be nice of Observatory broke down strings into size classes so we could
estimate the win on different apps.
Dart2js has lots of short strings so I hope it makes a difference.

[sra1, 2017-05-18 15:46:24]

On 2017/05/18 15:13:14, sra1 wrote:
> Is reducing the class-id bits a wise direction?
> Our largest app has a hair under 15000 classes. (21k if you closure convert to
> classes like dart2js does). This is a browser app.
> 
> It is not inconceivable that an server to support such an app would be close
to
> the 64k limit.
> 
> I once worked on a Java server app that could process arbitrary protobufs by
> name. There were too many protobuf message kinds to allow that app to be
> rewritten in dart with a 64k class limit.

It would be nice of Observatory broke down strings into size classes so we could
estimate the win on different apps.
Dart2js has lots of short strings so I hope it makes a difference.

[sra1, 2017-05-18 15:46:25]

On 2017/05/18 15:13:14, sra1 wrote:
> Is reducing the class-id bits a wise direction?
> Our largest app has a hair under 15000 classes. (21k if you closure convert to
> classes like dart2js does). This is a browser app.
> 
> It is not inconceivable that an server to support such an app would be close
to
> the 64k limit.
> 
> I once worked on a Java server app that could process arbitrary protobufs by
> name. There were too many protobuf message kinds to allow that app to be
> rewritten in dart with a 64k class limit.

It would be nice of Observatory broke down strings into size classes so we could
estimate the win on different apps.
Dart2js has lots of short strings so I hope it makes a difference.

[sra1, 2017-05-18 15:46:28]

On 2017/05/18 15:13:14, sra1 wrote:
> Is reducing the class-id bits a wise direction?
> Our largest app has a hair under 15000 classes. (21k if you closure convert to
> classes like dart2js does). This is a browser app.
> 
> It is not inconceivable that an server to support such an app would be close
to
> the 64k limit.
> 
> I once worked on a Java server app that could process arbitrary protobufs by
> name. There were too many protobuf message kinds to allow that app to be
> rewritten in dart with a 64k class limit.

It would be nice of Observatory broke down strings into size classes so we could
estimate the win on different apps.
Dart2js has lots of short strings so I hope it makes a difference.

[sra1, 2017-05-18 15:46:30]

On 2017/05/18 15:13:14, sra1 wrote:
> Is reducing the class-id bits a wise direction?
> Our largest app has a hair under 15000 classes. (21k if you closure convert to
> classes like dart2js does). This is a browser app.
> 
> It is not inconceivable that an server to support such an app would be close
to
> the 64k limit.
> 
> I once worked on a Java server app that could process arbitrary protobufs by
> name. There were too many protobuf message kinds to allow that app to be
> rewritten in dart with a 64k class limit.

It would be nice of Observatory broke down strings into size classes so we could
estimate the win on different apps.
Dart2js has lots of short strings so I hope it makes a difference.

[sra1, 2017-05-18 15:46:31]

On 2017/05/18 15:13:14, sra1 wrote:
> Is reducing the class-id bits a wise direction?
> Our largest app has a hair under 15000 classes. (21k if you closure convert to
> classes like dart2js does). This is a browser app.
> 
> It is not inconceivable that an server to support such an app would be close
to
> the 64k limit.
> 
> I once worked on a Java server app that could process arbitrary protobufs by
> name. There were too many protobuf message kinds to allow that app to be
> rewritten in dart with a 64k class limit.

It would be nice of Observatory broke down strings into size classes so we could
estimate the win on different apps.
Dart2js has lots of short strings so I hope it makes a difference.

[Vyacheslav Egorov (Google), 2017-05-22 10:39:35]

For the sake of future generations who will find this code review: we have
discussed Stephen's concerns over chat with him and arrived to the conclusion
that optimizing current memory usage is maintaining current ability to run
gigantic apps: because memory savings are concrete, but gigantic apps are
hypothetical.

We can always move back to the old representation of CID field later if gigantic
apps become reality.