Cache hash code for closures.

Cache hash code for closures.

Computing the hash code for closures is fairly expensive since
it involves the function name and signature strings.

This CL caches the hash code in the ClosureData object.

Fixes #28161.

R=asiva@google.com

Committed: https://github.com/dart-lang/sdk/commit/7615e899be45747efd987db74033bf0f2ba9fb70

[Florian Schneider, 2016-12-21 15:25:26]

fschneider@google.com changed reviewers:
+ asiva@google.com

[Cutch, 2016-12-21 15:39:45]

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

[Cutch, 2016-12-21 15:39:45]

DBC

https://codereview.chromium.org/2598623002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2598623002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:6854: return
Smi::New(Smi::Value(Smi::RawCast(ClosureData::Cast(obj).hash())));
this can just be Smi::RawCast(ClosureData::Cast(obj).hash()))

[Florian Schneider, 2016-12-21 15:49:40]

https://codereview.chromium.org/2598623002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2598623002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:6854: return
Smi::New(Smi::Value(Smi::RawCast(ClosureData::Cast(obj).hash())));
On 2016/12/21 15:39:45, Cutch wrote:
> this can just be Smi::RawCast(ClosureData::Cast(obj).hash()))

Done.

[siva, 2016-12-21 16:46:04]

I believe you need to write out a null in
RawClosureData::WriteTo(...) for the hash value
otherwise ClosureData::ReadFrom will get out of sync.

I think in ReadFill of ClosureDataDeserializationCluster
you would have to set the hash field to null because I think
it is allocated uninitialized otherwise.

[Cutch, 2016-12-21 16:49:38]

DBC2

https://codereview.chromium.org/2598623002/diff/40001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2598623002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:6853:
ASSERT(Object::Handle(ClosureData::Cast(obj).hash()).IsSmi());
Smi::RawCast already ASSERTS that the object is a Smi.

[Florian Schneider, 2016-12-21 16:51:13]

On 2016/12/21 16:46:04, siva wrote:
> I believe you need to write out a null in
> RawClosureData::WriteTo(...) for the hash value
> otherwise ClosureData::ReadFrom will get out of sync.
> 
> I think in ReadFill of ClosureDataDeserializationCluster
> you would have to set the hash field to null because I think
> it is allocated uninitialized otherwise.

Right, the snapshot code is not symmetric - unfortunately (ReadFrom used from()
and to(), WriteTo does not.

Strangely, all tests passed. I'll fix the snapshot writer.

I think I can leave it default-initialized (which should be null always)

[siva, 2016-12-21 16:54:25]

On 2016/12/21 16:51:13, Florian Schneider wrote:
> On 2016/12/21 16:46:04, siva wrote:
> > I believe you need to write out a null in
> > RawClosureData::WriteTo(...) for the hash value
> > otherwise ClosureData::ReadFrom will get out of sync.
> > 
> > I think in ReadFill of ClosureDataDeserializationCluster
> > you would have to set the hash field to null because I think
> > it is allocated uninitialized otherwise.
> 
> Right, the snapshot code is not symmetric - unfortunately (ReadFrom used
from()
> and to(), WriteTo does not.
> 
> Strangely, all tests passed. I'll fix the snapshot writer.
> 
> I think I can leave it default-initialized (which should be null always)

I don't think it is default-initialized, if you see
ClosureDataDeserializationCluster::ReadAlloc
it calls AllocateUnitialized which does not plop Object::null in the fields.

[Florian Schneider, 2016-12-22 10:11:17]

Ptal.

Added to_snapshot() for ClosureData.

Snapshot size is unchanged.

https://codereview.chromium.org/2598623002/diff/40001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2598623002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:6853:
ASSERT(Object::Handle(ClosureData::Cast(obj).hash()).IsSmi());
On 2016/12/21 16:49:38, Cutch wrote:
> Smi::RawCast already ASSERTS that the object is a Smi.

Done.

[siva, 2016-12-22 18:24:55]

lgtm

[Florian Schneider, 2016-12-23 10:39:30]

Description was changed from

==========
Cache hash code for closures.

Computing the hash code for closures is fairly expensive since
it involves the function name and signature strings.

This CL caches the hash code in the ClosureData object.

Fixes #28161.
==========

to

==========
Cache hash code for closures.

Computing the hash code for closures is fairly expensive since
it involves the function name and signature strings.

This CL caches the hash code in the ClosureData object.

Fixes #28161.

R=asiva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/7615e899be45747efd987db74033bf0f2ba9fb70
==========

[Florian Schneider, 2016-12-23 10:39:31]

Committed patchset #5 (id:80001) manually as
7615e899be45747efd987db74033bf0f2ba9fb70 (presubmit successful).