[serializer] small fixes for blink snapshot.

src/address-map.h

Index: src/address-map.h
diff --git a/src/address-map.h b/src/address-map.h
index 95e9cb064b34af0dd36b75a33dc6b481ff120b50..6443c709d0ff9731f71b8911bfc606e9d53d7b89 100644
--- a/src/address-map.h
+++ b/src/address-map.h
@@ -12,49 +12,54 @@
 namespace v8 {
 namespace internal {
 
-class AddressMapBase {
- protected:
-  static void SetValue(base::HashMap::Entry* entry, uint32_t v) {
-    entry->value = reinterpret_cast<void*>(v);
+template <typename Type>
+class PointerToIndexHashMap
+    : public base::TemplateHashMapImpl<intptr_t, uint32_t,

[vogelheim, 2016/11/10 09:37:10]

nitpick: why intptr_t, not uintptr_t ? 

Do we have a use for the sign, or in fact for any interpretation of the values
other than as a opaque, comparable container for 1x void* ?

[Yang, 2016/11/10 10:36:19]

No reason really. Changed.

+                                       base::KeyEqualityMatcher<intptr_t>,
+                                       base::DefaultAllocationPolicy> {
+ public:
+  typedef base::TemplateHashMapEntry<intptr_t, uint32_t> Entry;
+
+  inline void Set(Type value, uint32_t index) {
+    intptr_t key = Key(value);
+    LookupOrInsert(key, Hash(key))->value = index;
   }
 
-  static uint32_t GetValue(base::HashMap::Entry* entry) {
-    return static_cast<uint32_t>(reinterpret_cast<intptr_t>(entry->value));
+  inline bool Has(Type value) {
+    intptr_t key = Key(value);
+    return Lookup(key, Hash(key)) != nullptr;
   }
 
-  inline static base::HashMap::Entry* LookupEntry(base::HashMap* map,
-                                                  HeapObject* obj,
-                                                  bool insert) {
-    if (insert) {
-      map->LookupOrInsert(Key(obj), Hash(obj));
-    }
-    return map->Lookup(Key(obj), Hash(obj));
+  inline uint32_t Get(Type value) {
+    DCHECK(Has(value));
+    intptr_t key = Key(value);
+    return Lookup(key, Hash(key))->value;
   }
 
  private:
-  static uint32_t Hash(HeapObject* obj) {
-    return static_cast<int32_t>(reinterpret_cast<intptr_t>(obj->address()));
-  }
+  static intptr_t Key(Type value) { return reinterpret_cast<intptr_t>(value); }

[vogelheim, 2016/11/10 09:37:10]

nitpick: So... 'typename Type' must be a pointer type. Wouldn't it make more
sense to take a Type* as the parameter, so that typename Type can be anything,
and the map will deal with Type*?

[Yang, 2016/11/10 10:36:19]

Yeah, but then I can't really use PointerToIndexHashMap<Address>, but have to do
PointerToIndexHashMap<byte> instead. Besides, the class name says Pointer...

 
-  static void* Key(HeapObject* obj) {
-    return reinterpret_cast<void*>(obj->address());
+  static uint32_t Hash(intptr_t key) {
+    return static_cast<uint32_t>(key >> kPointerSizeLog2);
   }
 };
 
-class RootIndexMap : public AddressMapBase {
+class AddressToIndexHashMap : public PointerToIndexHashMap<Address> {};
+class HeapObjectToIndexHashMap : public PointerToIndexHashMap<HeapObject*> {};
+
+class RootIndexMap {
  public:
   explicit RootIndexMap(Isolate* isolate);
 
   static const int kInvalidRootIndex = -1;
 
   int Lookup(HeapObject* obj) {
-    base::HashMap::Entry* entry = LookupEntry(map_, obj, false);
-    if (entry) return GetValue(entry);
+    if (map_->Has(obj)) return map_->Get(obj);
     return kInvalidRootIndex;
   }
 
  private:
-  base::HashMap* map_;
+  HeapObjectToIndexHashMap* map_;
 
   DISALLOW_COPY_AND_ASSIGN(RootIndexMap);
 };
@@ -186,21 +191,20 @@ class SerializerReference {
 
 // Mapping objects to their location after deserialization.
 // This is used during building, but not at runtime by V8.
-class SerializerReferenceMap : public AddressMapBase {
+class SerializerReferenceMap {
  public:
   SerializerReferenceMap()
       : no_allocation_(), map_(), attached_reference_index_(0) {}
 
   SerializerReference Lookup(HeapObject* obj) {
-    base::HashMap::Entry* entry = LookupEntry(&map_, obj, false);
-    return entry ? SerializerReference(GetValue(entry)) : SerializerReference();
+    return map_.Has(obj) ? SerializerReference(map_.Get(obj))

[vogelheim, 2016/11/10 09:37:10]

The way Has + Get are implemented, this Lookup performs the same operation
twice. Not sure if this is performance relevant, but why not map_.Get(..) it
once and then check for nullptr?


[The Has/Get pattern occurs a lot in this CL.]

[I find the code - as you've written it - nicer to read, but the way the map is
implemented, it's unfortunately less efficient. As said, not sure if this is
relevant.]

[Yang, 2016/11/10 10:36:19]

Unfortunately Get is allowed to return 0 as valid result. Both Has and Get are
inline functions that call TemplateHashMapImpl<Key, Value, MatchFun,
AllocationPolicy>::Lookup, which is a const function. So I was hoping the
compiler is smart enough.

[vogelheim, 2016/11/10 10:59:28]

Now that you mention it: maybe const-ify Get + Has?

I suspect const-ness isn't a sufficient guarantee for the compiler to eliminate
the 2nd call. I guess that's why the base class returns the Entry*, but that's
admittedly a somewhat ugly interface.

Either way, no big deal.

+                         : SerializerReference();
   }
 
   void Add(HeapObject* obj, SerializerReference b) {
     DCHECK(b.is_valid());
-    DCHECK_NULL(LookupEntry(&map_, obj, false));
-    base::HashMap::Entry* entry = LookupEntry(&map_, obj, true);
-    SetValue(entry, b.bitfield_);
+    DCHECK(!map_.Has(obj));
+    map_.Set(obj, b.bitfield_);
   }
 
   SerializerReference AddAttachedReference(HeapObject* attached_reference) {
@@ -212,7 +216,7 @@ class SerializerReferenceMap : public AddressMapBase {
 
  private:
   DisallowHeapAllocation no_allocation_;
-  base::HashMap map_;
+  HeapObjectToIndexHashMap map_;
   int attached_reference_index_;
   DISALLOW_COPY_AND_ASSIGN(SerializerReferenceMap);
 };