Improve constant pool implementation in the assembler.

Improve constant pool implementation in the assembler.

Instead of using RawObject* as keys, use Object*. Using handles eliminates any assumption
about the underlying GC implementation (e.g. requiring that old objects don't move).

Remove special handling of null_object by making HashMap more generic and
allowing the trait to specify the value used to indicate an empty element.

The goal is to use one unified, efficient implementation of the constant
pool on all architectures that require one (x64, arm64, mips).

R=vegorov@google.com, zra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=42901

[Florian Schneider, 2015-01-12 16:31:36]

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

[Florian Schneider, 2015-01-12 16:32:32]

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

[Florian Schneider, 2015-01-12 16:32:33]

+vegorov: Please take a look at changes to hash_map.h

[zra, 2015-01-12 17:34:00]

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

https://codereview.chromium.org/848703002/diff/1/runtime/vm/assembler_x64.h#n...
runtime/vm/assembler_x64.h:1038: ? key : &Object::ZoneHandle(key->raw())),
I'm not sure I understand what the implications are here. Are we stuck with
these objects in the zone, even if the code is collected?

https://codereview.chromium.org/848703002/diff/1/runtime/vm/assembler_x64.h#n...
runtime/vm/assembler_x64.h:1062: return key->GetClassId();
So, Bool::True and Bool::False will end up in the same chain? Is there anything
else we could do here? I guess this is okay for now since we're not putting too
many objects with the same class into the object pools, but it'd be nice if
there were at least a plan in a TODO for what to do in case that changes.

[Florian Schneider, 2015-01-13 09:54:19]

I'll go ahead and port this to ARM, MIPS and ARM64.

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

https://codereview.chromium.org/848703002/diff/1/runtime/vm/assembler_x64.h#n...
runtime/vm/assembler_x64.h:1038: ? key : &Object::ZoneHandle(key->raw())),
On 2015/01/12 17:33:59, zra wrote:
> I'm not sure I understand what the implications are here. Are we stuck with
> these objects in the zone, even if the code is collected?

The zone goes away as soon as compilation is finished.

https://codereview.chromium.org/848703002/diff/1/runtime/vm/assembler_x64.h#n...
runtime/vm/assembler_x64.h:1062: return key->GetClassId();
On 2015/01/12 17:33:59, zra wrote:
> So, Bool::True and Bool::False will end up in the same chain? Is there
anything
> else we could do here? I guess this is okay for now since we're not putting
too
> many objects with the same class into the object pools, but it'd be nice if
> there were at least a plan in a TODO for what to do in case that changes.

So, for Object::null, Bool::True/False we don't even need to lookup the index,
since those are at index 0, 1 and 2.
For other classes, the plan is to have a better hash function for those classes
eventually. I'll add a TODO.

[Vyacheslav Egorov (Google), 2015-01-13 13:47:30]

hash_map LGTM with comments. try to kill NoValue

https://codereview.chromium.org/848703002/diff/40001/runtime/vm/assembler.h
File runtime/vm/assembler.h (right):

https://codereview.chromium.org/848703002/diff/40001/runtime/vm/assembler.h#n...
runtime/vm/assembler.h:217: explicit ObjIndexPair(Value value) : key_(NULL),
value_(NoValue()) { }
I think parameter is redundant. Just provide default constructor.

https://codereview.chromium.org/848703002/diff/40001/runtime/vm/hash_map.h
File runtime/vm/hash_map.h (right):

https://codereview.chromium.org/848703002/diff/40001/runtime/vm/hash_map.h#ne...
runtime/vm/hash_map.h:120: const typename KeyValueTrait::Value kNoValue =
KeyValueTrait::NoValue();
You can probably use ValueOf(KeyValueTrait::Pair()) instead of NoValue() then
you don't need explicit NoValue - you just need to ensure that default
initialized Pair has correct Value

[Florian Schneider, 2015-01-13 14:09:08]

https://codereview.chromium.org/848703002/diff/40001/runtime/vm/assembler.h
File runtime/vm/assembler.h (right):

https://codereview.chromium.org/848703002/diff/40001/runtime/vm/assembler.h#n...
runtime/vm/assembler.h:217: explicit ObjIndexPair(Value value) : key_(NULL),
value_(NoValue()) { }
On 2015/01/13 13:47:30, Vyacheslav Egorov (Google) wrote:
> I think parameter is redundant. Just provide default constructor.

Done.

https://codereview.chromium.org/848703002/diff/40001/runtime/vm/hash_map.h
File runtime/vm/hash_map.h (right):

https://codereview.chromium.org/848703002/diff/40001/runtime/vm/hash_map.h#ne...
runtime/vm/hash_map.h:120: const typename KeyValueTrait::Value kNoValue =
KeyValueTrait::NoValue();
On 2015/01/13 13:47:30, Vyacheslav Egorov (Google) wrote:
> You can probably use ValueOf(KeyValueTrait::Pair()) instead of NoValue() then
> you don't need explicit NoValue - you just need to ensure that default
> initialized Pair has correct Value

Done.  KeyValueTrait::ValueOf(typename KeyValueTrait::Pair()) does the trick.

[zra, 2015-01-14 16:24:59]

lgtm

[Florian Schneider, 2015-01-15 10:43:43]

Committed patchset #6 (id:100001) manually as r42901 (presubmit successful).

[zra, 2015-01-15 15:21:26]

On 2015/01/15 10:43:43, Florian Schneider wrote:
> Committed patchset #6 (id:100001) manually as r42901 (presubmit successful).

Could you also update the issue? dartbug.com/13305 Thanks!

[Ivan Posva, 2015-01-15 23:59:57]

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

[Ivan Posva, 2015-01-15 23:59:58]

Thanks for fixing. Would be nice to share the abstraction and be able to
distinguish between patchable and non patchable entries in arm and mips.

-Ivan

https://codereview.chromium.org/848703002/diff/100001/runtime/vm/assembler_ar...
File runtime/vm/assembler_arm64.cc (right):

https://codereview.chromium.org/848703002/diff/100001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.cc:43:
object_pool_index_table_.Insert(ObjIndexPair(&Object::null_object(), 0));
We have three data structures in the assembler that need to be kept in sync:
object_pool_, patchable_pool_entries_ and object_pool_index_table_

It would probably make sense to abstract that into an assembler helper class
AssemblerObjectPool and use it across all architectures.

[Florian Schneider, 2015-01-16 10:12:49]

On 2015/01/15 23:59:58, Ivan Posva wrote:
> Thanks for fixing. Would be nice to share the abstraction and be able to
> distinguish between patchable and non patchable entries in arm and mips.
> 
> -Ivan
> 

Yes, there is still this difference between x64/arm64 and arm/mips. I'll take a
look at it.

It should be straight-forward to add this functionality to arm/mips and will
reduce the size of the constant pools there.

>
https://codereview.chromium.org/848703002/diff/100001/runtime/vm/assembler_ar...
> File runtime/vm/assembler_arm64.cc (right):
> 
>
https://codereview.chromium.org/848703002/diff/100001/runtime/vm/assembler_ar...
> runtime/vm/assembler_arm64.cc:43:
> object_pool_index_table_.Insert(ObjIndexPair(&Object::null_object(), 0));
> We have three data structures in the assembler that need to be kept in sync:
> object_pool_, patchable_pool_entries_ and object_pool_index_table_
> 
> It would probably make sense to abstract that into an assembler helper class
> AssemblerObjectPool and use it across all architectures.

Agree. Once we use the same abstractions on all platforms I can move the common
parts to assembler.h/.cc.