Improve constant pool implementation in the assembler.

runtime/vm/assembler_x64.h

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #ifndef VM_ASSEMBLER_X64_H_
 #define VM_ASSEMBLER_X64_H_
 
 #ifndef VM_ASSEMBLER_H_
 #error Do not include assembler_x64.h directly; use assembler.h instead.
 #endif
@@ skipping 1008 matching lines @@
 
   // Objects and jump targets.
   GrowableObjectArray& object_pool_;
 
   // Patchability of pool entries.
   GrowableArray<Patchability> patchable_pool_entries_;
 
   // Pair type parameter for DirectChainedHashMap.
   class ObjIndexPair {
    public:
-    // TODO(zra): A WeakTable should be used here instead, but then it would
-    // also have to be possible to register and de-register WeakTables with the
-    // heap. Also, the Assembler would need to become a StackResource.
-    // Issue 13305. In the meantime...
-    // CAUTION: the RawObject* below is only safe because:
-    // The HashMap that will use this pair type will not contain any RawObject*
-    // keys that are not in the object_pool_ array. Since the keys will be
-    // visited by the GC when it visits the object_pool_, and since all objects
-    // in the object_pool_ are Old (and so will not be moved) the GC does not
-    // also need to visit the keys here in the HashMap.
-
     // Typedefs needed for the DirectChainedHashMap template.
-    typedef RawObject* Key;
+    typedef const Object* Key;
     typedef intptr_t Value;
     typedef ObjIndexPair Pair;
 
-    ObjIndexPair(Key key, Value value) : key_(key), value_(value) { }
+    explicit ObjIndexPair(Value value) : key_(NULL), value_(NoValue()) { }
+
+    ObjIndexPair(Key key, Value value)
+       : key_(key->IsNotTemporaryScopedHandle()
+           ? key : &Object::ZoneHandle(key->raw())),

[zra, 2015/01/12 17:33:59]

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?

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

The zone goes away as soon as compilation is finished.

+         value_(value) { }
 
     static Key KeyOf(Pair kv) { return kv.key_; }
 
     static Value ValueOf(Pair kv) { return kv.value_; }
 
+    static Value NoValue() { return -1; }
+
     static intptr_t Hashcode(Key key) {
-      return reinterpret_cast<intptr_t>(key) >> kObjectAlignmentLog2;
+      if (key->IsSmi()) {
+        return Smi::Cast(*key).Value();
+      }
+      if (key->IsDouble()) {
+        return static_cast<intptr_t>(
+            bit_cast<int32_t, float>(
+                static_cast<float>(Double::Cast(*key).value())));
+      }
+      if (key->IsMint()) {
+        return static_cast<intptr_t>(Mint::Cast(*key).value());
+      }
+      if (key->IsString()) {
+        return String::Cast(*key).Hash();
+      }
+      return key->GetClassId();

[zra, 2015/01/12 17:33:59]

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]

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.

     }
 
     static inline bool IsKeyEqual(Pair kv, Key key) {
-      return kv.key_ == key;
+      return kv.key_->raw() == key->raw();
     }
 
    private:
     Key key_;
     Value value_;
   };
 
   // Hashmap for fast lookup in object pool.
   DirectChainedHashMap<ObjIndexPair> object_pool_index_table_;
 
@@ skipping 159 matching lines @@
 }
 
 
 inline void Assembler::EmitOperandSizeOverride() {
   EmitUint8(0x66);
 }
 
 }  // namespace dart
 
 #endif  // VM_ASSEMBLER_X64_H_