[Oilpan]: Make StylePropertySet fully garbage collected.

Source/core/dom/PresentationAttributeStyle.cpp

Index: Source/core/dom/PresentationAttributeStyle.cpp
diff --git a/Source/core/dom/PresentationAttributeStyle.cpp b/Source/core/dom/PresentationAttributeStyle.cpp
index 3d9a3e35496002585144b1fc427368e391cafac9..c6ea67ca79c4a0704ba47921e8c28e0c38237eda 100644
--- a/Source/core/dom/PresentationAttributeStyle.cpp
+++ b/Source/core/dom/PresentationAttributeStyle.cpp
@@ -56,18 +56,25 @@ static bool operator!=(const PresentationAttributeCacheKey& a, const Presentatio
     return a.attributesAndValues != b.attributesAndValues;
 }
 
-struct PresentationAttributeCacheEntry {
-    WTF_MAKE_FAST_ALLOCATED;
+struct PresentationAttributeCacheEntry : public NoBaseWillBeGarbageCollectedFinalized<PresentationAttributeCacheEntry> {
+    WTF_MAKE_FAST_ALLOCATED_WILL_BE_REMOVED;
 public:
+    void trace(Visitor* visitor) { visitor->trace(value); }
+
     PresentationAttributeCacheKey key;
-    RefPtr<StylePropertySet> value;
+    RefPtrWillBeMember<StylePropertySet> value;
 };
 
-typedef HashMap<unsigned, OwnPtr<PresentationAttributeCacheEntry>, AlreadyHashed> PresentationAttributeCache;
+typedef WillBeHeapHashMap<unsigned, OwnPtrWillBeMember<PresentationAttributeCacheEntry>, AlreadyHashed> PresentationAttributeCache;
 static PresentationAttributeCache& presentationAttributeCache()
 {
+#if ENABLE(OILPAN)
+    DEFINE_STATIC_LOCAL(Persistent<PresentationAttributeCache>, cache, (new PresentationAttributeCache()));
+    return *cache;
+#else
     DEFINE_STATIC_LOCAL(PresentationAttributeCache, cache, ());
     return cache;
+#endif // ENABLE(OILPAN)
 }
 
 class PresentationAttributeCacheCleaner {
@@ -152,7 +159,7 @@ static unsigned computePresentationAttributeCacheHash(const PresentationAttribut
     return WTF::pairIntHash(key.tagName->existingHash(), attributeHash);
 }
 
-PassRefPtr<StylePropertySet> computePresentationAttributeStyle(Element& element)
+PassRefPtrWillBeRawPtr<StylePropertySet> computePresentationAttributeStyle(Element& element)
 {
     DEFINE_STATIC_LOCAL(PresentationAttributeCacheCleaner, cacheCleaner, ());
 
@@ -163,18 +170,18 @@ PassRefPtr<StylePropertySet> computePresentationAttributeStyle(Element& element)
 
     unsigned cacheHash = computePresentationAttributeCacheHash(cacheKey);
 
-    PresentationAttributeCache::ValueType* cacheValue;
+    PresentationAttributeCacheEntry* cacheEntry;
     if (cacheHash) {
-        cacheValue = presentationAttributeCache().add(cacheHash, nullptr).storedValue;
-        if (cacheValue->value && cacheValue->value->key != cacheKey)
+        cacheEntry = presentationAttributeCache().get(cacheHash);
+        if (cacheEntry && cacheEntry->key != cacheKey)

[esprehn, 2014/05/14 20:25:27]

Why did you change this? Why doesn't the Oilpan type using the same API as the
WTF type?

[wibling-chromium, 2014/05/15 13:15:18]

I am not sure I follow? The reason for changing the type above is since we do a
get and get a hash map value back and not an entry which is (confusingly) named
ValueType.

             cacheHash = 0;
     } else {
-        cacheValue = 0;
+        cacheEntry = 0;
     }
 
-    RefPtr<StylePropertySet> style;
-    if (cacheHash && cacheValue->value) {
-        style = cacheValue->value->value;
+    RefPtrWillBeRawPtr<StylePropertySet> style;
+    if (cacheHash && cacheEntry) {
+        style = cacheEntry->value;
         cacheCleaner.didHitPresentationAttributeCache();
     } else {
         style = MutableStylePropertySet::create(element.isSVGElement() ? SVGAttributeMode : HTMLAttributeMode);
@@ -185,10 +192,10 @@ PassRefPtr<StylePropertySet> computePresentationAttributeStyle(Element& element)
         }
     }
 
-    if (!cacheHash || cacheValue->value)
+    if (!cacheHash || cacheEntry)
         return style.release();
 
-    OwnPtr<PresentationAttributeCacheEntry> newEntry = adoptPtr(new PresentationAttributeCacheEntry);
+    OwnPtrWillBeRawPtr<PresentationAttributeCacheEntry> newEntry = adoptPtrWillBeNoop(new PresentationAttributeCacheEntry);
     newEntry->key = cacheKey;
     newEntry->value = style;
 
@@ -197,10 +204,8 @@ PassRefPtr<StylePropertySet> computePresentationAttributeStyle(Element& element)
         // FIXME: Discarding the entire cache when it gets too big is probably bad
         // since it creates a perf "cliff". Perhaps we should use an LRU?
         presentationAttributeCache().clear();
-        presentationAttributeCache().set(cacheHash, newEntry.release());
-    } else {
-        cacheValue->value = newEntry.release();
     }
+    presentationAttributeCache().set(cacheHash, newEntry.release());

[esprehn, 2014/05/14 20:25:27]

This is again wrong, you make us do two hash lookups instead of one.

[wibling-chromium, 2014/05/15 13:15:18]

Yes, this is similar to the previous case where the allocation of the newEntry
could trigger a GC which could cause the backing of the hash map to move. I will
talk to Erik and Mads to see if we can find a way to avoid the additional
lookup.

 
     return style.release();
 }