Amend PersistentValueMap:

- Use the surrounding map (instead of Traits::Impl) for weak callback.
- Provide for a fast reference to a mapped value.
- Restructure Traits to accomondate for the first point above.

[Why?] As discussed, I proceeded to replace Impl with the map.
The problem I encountered with that version is that now the
Traits class depends on itself: The weak-related methods require the
map type in their signature. But the map type includes the Traits class
and hence the Traits class method signatures depend on the specific Traits class. That
makes them practically un-derivable: While you can derive a Traits class
from another one, since the compiler now expects methods with a different
signature. To accommodate, I pulled the dispose traits into the weak traits
class. I also removed the Impl*/MapType* parameter from the Dispose call,
since no implementation seems to need it.

R=dcarney@chromium.org
BUG=

Review URL: https://codereview.chromium.org/212893007

Patch from Daniel Vogelheim <vogelheim@chromium.org>.

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@20326 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

include/v8-util.h

@@ -42,6 +42,10 @@ namespace v8 {
 
 typedef uintptr_t PersistentContainerValue;
 static const uintptr_t kPersistentContainerNotFound = 0;
+enum PersistentContainerCallbackType {
+  kNotWeak,
+  kWeak
+};
 
 
 /**
@@ -92,38 +96,34 @@ class StdMapTraits {
 /**
  * A default trait implementation for PersistentValueMap, which inherits
  * a std:map backing map from StdMapTraits and holds non-weak persistent
- * objects.
+ * objects and has no special Dispose handling.
  *
- * Users have to implement their own dispose trait.
+ * You should not derive from this class, since MapType depends on the
+ * surrounding class, and hence a subclass cannot simply inherit the methods.
  */
 template<typename K, typename V>
-class StrongMapTraits : public StdMapTraits<K, V> {
+class DefaultPersistentValueMapTraits : public StdMapTraits<K, V> {
  public:
   // Weak callback & friends:
-  static const bool kIsWeak = false;
-  typedef typename StdMapTraits<K, V>::Impl Impl;
+  static const PersistentContainerCallbackType kCallbackType = kNotWeak;
+  typedef PersistentValueMap<K, V, DefaultPersistentValueMapTraits<K, V> >
+      MapType;
   typedef void WeakCallbackDataType;
+
   static WeakCallbackDataType* WeakCallbackParameter(
-      Impl* impl, const K& key, Local<V> value);
-  static Impl* ImplFromWeakCallbackData(
-      const WeakCallbackData<V, WeakCallbackDataType>& data);
+      MapType* map, const K& key, Local<V> value) {
+    return NULL;
+  }
+  static MapType* MapFromWeakCallbackData(
+          const WeakCallbackData<V, WeakCallbackDataType>& data) {
+    return NULL;
+  }
   static K KeyFromWeakCallbackData(
-      const WeakCallbackData<V, WeakCallbackDataType>& data);
-  static void DisposeCallbackData(WeakCallbackDataType* data);
-};
-
-
-/**
- * A default trait implementation for PersistentValueMap, with a std::map
- * backing map, non-weak persistents as values, and no special dispose
- * handling. Can be used as-is.
- */
-template<typename K, typename V>
-class DefaultPersistentValueMapTraits : public StrongMapTraits<K, V> {
- public:
-  typedef typename StrongMapTraits<K, V>::Impl Impl;
-  static void Dispose(Isolate* isolate, UniquePersistent<V> value,
-      Impl* impl, K key) { }
+      const WeakCallbackData<V, WeakCallbackDataType>& data) {
+    return K();
+  }
+  static void DisposeCallbackData(WeakCallbackDataType* data) { }
+  static void Dispose(Isolate* isolate, UniquePersistent<V> value, K key) { }
 };
 
 
@@ -154,7 +154,7 @@ class PersistentValueMap {
   /**
    * Return whether the map holds weak persistents.
    */
-  V8_INLINE bool IsWeak() { return Traits::kIsWeak; }
+  V8_INLINE bool IsWeak() { return Traits::kCallbackType != kNotWeak; }
 
   /**
    * Get value stored in map.
@@ -167,7 +167,7 @@ class PersistentValueMap {
    * Check whether a value is contained in the map.
    */
   V8_INLINE bool Contains(const K& key) {
-    return Traits::Get(&impl_, key) != 0;
+    return Traits::Get(&impl_, key) != kPersistentContainerNotFound;
   }
 
   /**
@@ -175,14 +175,8 @@ class PersistentValueMap {
    * Return true if a value was found.
    */
   V8_INLINE bool SetReturnValue(const K& key,
-      ReturnValue<Value>& returnValue) {
-    PersistentContainerValue value = Traits::Get(&impl_, key);
-    bool hasValue = value != 0;
-    if (hasValue) {
-      returnValue.SetInternal(
-          *reinterpret_cast<internal::Object**>(FromVal(value)));
-    }
-    return hasValue;
+      ReturnValue<Value> returnValue) {
+    return SetReturnValueFromVal(returnValue, Traits::Get(&impl_, key));
   }
 
   /**
@@ -231,12 +225,76 @@ class PersistentValueMap {
       typename Traits::Impl impl;
       Traits::Swap(impl_, impl);
       for (It i = Traits::Begin(&impl); i != Traits::End(&impl); ++i) {
-        Traits::Dispose(isolate_, Release(Traits::Value(i)).Pass(), &impl,
+        Traits::Dispose(isolate_, Release(Traits::Value(i)).Pass(),
                         Traits::Key(i));
       }
     }
   }
 
+  /**
+   * Helper class for GetReference/SetWithReference. Do not use outside
+   * that context.
+   */
+  class PersistentValueReference {
+   public:
+    PersistentValueReference() : value_(kPersistentContainerNotFound) { }
+    PersistentValueReference(const PersistentValueReference& other)
+        : value_(other.value_) { }
+
+    Local<V> NewLocal(Isolate* isolate) const {
+      return Local<V>::New(isolate, FromVal(value_));
+    }
+    bool IsEmpty() const {
+      return value_ == kPersistentContainerNotFound;
+    }
+    template<typename T>
+    bool SetReturnValue(ReturnValue<T> returnValue) {
+      return SetReturnValueFromVal(returnValue, value_);
+    }
+    void Reset() {
+      value_ = kPersistentContainerNotFound;
+    }
+    void operator=(const PersistentValueReference& other) {
+      value_ = other.value_;
+    }
+
+   private:
+    friend class PersistentValueMap;
+
+    explicit PersistentValueReference(PersistentContainerValue value)
+        : value_(value) { }
+
+    void operator=(PersistentContainerValue value) {
+      value_ = value;
+    }
+
+    PersistentContainerValue value_;
+  };
+
+  /**
+   * Get a reference to a map value. This enables fast, repeated access
+   * to a value stored in the map while the map remains unchanged.
+   *
+   * Careful: This is potentially unsafe, so please use with care.
+   * The value will become invalid if the value for this key changes
+   * in the underlying map, as a result of Set or Remove for the same
+   * key; as a result of the weak callback for the same key; or as a
+   * result of calling Clear() or destruction of the map.
+   */
+  V8_INLINE PersistentValueReference GetReference(const K& key) {
+    return PersistentValueReference(Traits::Get(&impl_, key));
+  }
+
+  /**
+   * Put a value into the map and update the reference.
+   * Restrictions of GetReference apply here as well.
+   */
+  UniquePersistent<V> Set(const K& key, UniquePersistent<V> value,
+                          PersistentValueReference* reference) {
+    *reference = Leak(&value);
+    return SetUnique(key, &value);
+  }
+
  private:
   PersistentValueMap(PersistentValueMap&);
   void operator=(PersistentValueMap&);
@@ -246,10 +304,10 @@ class PersistentValueMap {
    * by the Traits class.
    */
   UniquePersistent<V> SetUnique(const K& key, UniquePersistent<V>* persistent) {
-    if (Traits::kIsWeak) {
+    if (Traits::kCallbackType != kNotWeak) {
       Local<V> value(Local<V>::New(isolate_, *persistent));
       persistent->template SetWeak<typename Traits::WeakCallbackDataType>(
-        Traits::WeakCallbackParameter(&impl_, key, value), WeakCallback);
+        Traits::WeakCallbackParameter(this, key, value), WeakCallback);
     }
     PersistentContainerValue old_value =
         Traits::Set(&impl_, key, ClearAndLeak(persistent));
@@ -258,11 +316,12 @@ class PersistentValueMap {
 
   static void WeakCallback(
       const WeakCallbackData<V, typename Traits::WeakCallbackDataType>& data) {
-    if (Traits::kIsWeak) {
-      typename Traits::Impl* impl = Traits::ImplFromWeakCallbackData(data);
+    if (Traits::kCallbackType != kNotWeak) {
+      PersistentValueMap<K, V, Traits>* persistentValueMap =
+          Traits::MapFromWeakCallbackData(data);
       K key = Traits::KeyFromWeakCallbackData(data);
-      PersistentContainerValue value = Traits::Remove(impl, key);
-      Traits::Dispose(data.GetIsolate(), Release(value).Pass(), impl, key);
+      Traits::Dispose(data.GetIsolate(),
+                      persistentValueMap->Remove(key).Pass(), key);
     }
   }
 
@@ -270,6 +329,16 @@ class PersistentValueMap {
     return reinterpret_cast<V*>(v);
   }
 
+  V8_INLINE static bool SetReturnValueFromVal(
+      ReturnValue<Value>& returnValue, PersistentContainerValue value) {
+    bool hasValue = value != kPersistentContainerNotFound;
+    if (hasValue) {
+      returnValue.SetInternal(
+          *reinterpret_cast<internal::Object**>(FromVal(value)));
+    }
+    return hasValue;
+  }
+
   V8_INLINE static PersistentContainerValue ClearAndLeak(
       UniquePersistent<V>* persistent) {
     V* v = persistent->val_;
@@ -277,6 +346,11 @@ class PersistentValueMap {
     return reinterpret_cast<PersistentContainerValue>(v);
   }
 
+  V8_INLINE static PersistentContainerValue Leak(
+      UniquePersistent<V>* persistent) {
+    return reinterpret_cast<PersistentContainerValue>(persistent->val_);
+  }
+
   /**
    * Return a container value as UniquePersistent and make sure the weak
    * callback is properly disposed of. All remove functionality should go
@@ -285,7 +359,7 @@ class PersistentValueMap {
   V8_INLINE static UniquePersistent<V> Release(PersistentContainerValue v) {
     UniquePersistent<V> p;
     p.val_ = FromVal(v);
-    if (Traits::kIsWeak && !p.IsEmpty()) {
+    if (Traits::kCallbackType != kNotWeak && !p.IsEmpty()) {
       Traits::DisposeCallbackData(
           p.template ClearWeak<typename Traits::WeakCallbackDataType>());
     }
@@ -312,44 +386,6 @@ class StdPersistentValueMap : public PersistentValueMap<K, V, Traits> {
       : PersistentValueMap<K, V, Traits>(isolate) {}
 };
 
-
-/**
- * Empty default implementations for StrongTraits methods.
- *
- * These should not be necessary, since they're only used in code that
- * is surrounded by if(Traits::kIsWeak), which for StrongMapTraits is
- * compile-time false. Most compilers can live without them; however
- * the compiler we use from 64-bit Win differs.
- *
- * TODO(vogelheim): Remove these once they're no longer necessary.
- */
-template<typename K, typename V>
-typename StrongMapTraits<K, V>::WeakCallbackDataType*
-    StrongMapTraits<K, V>::WeakCallbackParameter(
-        Impl* impl, const K& key, Local<V> value) {
-  return NULL;
-}
-
-
-template<typename K, typename V>
-typename StrongMapTraits<K, V>::Impl*
-    StrongMapTraits<K, V>::ImplFromWeakCallbackData(
-        const WeakCallbackData<V, WeakCallbackDataType>& data) {
-  return NULL;
-}
-
-
-template<typename K, typename V>
-K StrongMapTraits<K, V>::KeyFromWeakCallbackData(
-    const WeakCallbackData<V, WeakCallbackDataType>& data) {
-  return K();
-}
-
-
-template<typename K, typename V>
-void StrongMapTraits<K, V>::DisposeCallbackData(WeakCallbackDataType* data) {
-}
-
 }  // namespace v8
 
 #endif  // V8_UTIL_H_

test/cctest/test-api.cc

@@ -3498,22 +3498,23 @@ THREADED_TEST(UniquePersistent) {
 template<typename K, typename V>
 class WeakStdMapTraits : public v8::StdMapTraits<K, V> {
  public:
-  typedef typename v8::DefaultPersistentValueMapTraits<K, V>::Impl Impl;
-  static const bool kIsWeak = true;
+  typedef typename v8::PersistentValueMap<K, V, WeakStdMapTraits<K, V> >
+      MapType;
+  static const v8::PersistentContainerCallbackType kCallbackType = v8::kWeak;
   struct WeakCallbackDataType {
-    Impl* impl;
+    MapType* map;
     K key;
   };
   static WeakCallbackDataType* WeakCallbackParameter(
-      Impl* impl, const K& key, Local<V> value) {
+      MapType* map, const K& key, Local<V> value) {
     WeakCallbackDataType* data = new WeakCallbackDataType;
-    data->impl = impl;
+    data->map = map;
     data->key = key;
     return data;
   }
-  static Impl* ImplFromWeakCallbackData(
+  static MapType* MapFromWeakCallbackData(
       const v8::WeakCallbackData<V, WeakCallbackDataType>& data) {
-    return data.GetParameter()->impl;
+    return data.GetParameter()->map;
   }
   static K KeyFromWeakCallbackData(
       const v8::WeakCallbackData<V, WeakCallbackDataType>& data) {
@@ -3523,7 +3524,7 @@ class WeakStdMapTraits : public v8::StdMapTraits<K, V> {
     delete data;
   }
   static void Dispose(v8::Isolate* isolate, v8::UniquePersistent<V> value,
-      Impl* impl, K key) { }
+      K key) { }
 };
 
 
@@ -3545,6 +3546,10 @@ static void TestPersistentValueMap() {
     CHECK_EQ(1, static_cast<int>(map.Size()));
     obj = map.Get(7);
     CHECK_EQ(expected, obj);
+    {
+      typename Map::PersistentValueReference ref = map.GetReference(7);
+      CHECK_EQ(expected, ref.NewLocal(isolate));
+    }
     v8::UniquePersistent<v8::Object> removed = map.Remove(7);
     CHECK_EQ(0, static_cast<int>(map.Size()));
     CHECK(expected == removed);
@@ -3554,6 +3559,15 @@ static void TestPersistentValueMap() {
     CHECK_EQ(1, static_cast<int>(map.Size()));
     map.Set(8, expected);
     CHECK_EQ(1, static_cast<int>(map.Size()));
+    {
+      typename Map::PersistentValueReference ref;
+      Local<v8::Object> expected2 = v8::Object::New(isolate);
+      removed = map.Set(8,
+          v8::UniquePersistent<v8::Object>(isolate, expected2), &ref);
+      CHECK_EQ(1, static_cast<int>(map.Size()));
+      CHECK(expected == removed);
+      CHECK_EQ(expected2, ref.NewLocal(isolate));
+    }
   }
   CHECK_EQ(initial_handle_count + 1, global_handles->global_handles_count());
   if (map.IsWeak()) {
@@ -3572,7 +3586,7 @@ TEST(PersistentValueMap) {
   TestPersistentValueMap<v8::StdPersistentValueMap<int, v8::Object> >();
 
   // Custom traits with weak callbacks:
-  typedef v8::StdPersistentValueMap<int, v8::Object,
+  typedef v8::PersistentValueMap<int, v8::Object,
       WeakStdMapTraits<int, v8::Object> > WeakPersistentValueMap;
   TestPersistentValueMap<WeakPersistentValueMap>();
 }