[Oilpan]: Make StylePropertySet fully garbage collected.

Source/core/css/resolver/MatchedPropertiesCache.h

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Library General Public License for more details.
  *
  * You should have received a copy of the GNU Library General Public License
  * along with this library; see the file COPYING.LIB.  If not, write to
  * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
  * Boston, MA 02110-1301, USA.
  *
  */
 
 #ifndef MatchedPropertiesCache_h
 #define MatchedPropertiesCache_h
 
+#include "core/css/StylePropertySet.h"
 #include "core/css/resolver/MatchResult.h"
-
 #include "platform/Timer.h"
+#include "platform/heap/Handle.h"
 #include "wtf/Forward.h"
 #include "wtf/HashMap.h"
 #include "wtf/Noncopyable.h"
 
 namespace WebCore {
 
 class RenderStyle;
 class StyleResolverState;
 
-struct CachedMatchedProperties {
-    Vector<MatchedProperties> matchedProperties;
+class CachedMatchedProperties FINAL : public NoBaseWillBeGarbageCollectedFinalized<CachedMatchedProperties> {
+
+public:
+    WillBeHeapVector<MatchedProperties> matchedProperties;
     MatchRanges ranges;
     RefPtr<RenderStyle> renderStyle;
     RefPtr<RenderStyle> parentRenderStyle;
 
     void set(const RenderStyle*, const RenderStyle* parentStyle, const MatchResult&);
     void clear();
+    void trace(Visitor* visitor) { visitor->trace(matchedProperties); }
 };
 
+// Specialize the HashTraits for CachedMatchedProperties to check for dead
+// entries in the MatchedPropertiesCache.
+#if ENABLE(OILPAN)
+struct CachedMatchedPropertiesHashTraits : HashTraits<Member<CachedMatchedProperties> > {

[esprehn, 2014/06/26 09:48:53]

Does this really need to be inline? I'd rather it didn't add so much code to
this header.

+    static const WTF::WeakHandlingFlag weakHandlingFlag = WTF::WeakHandlingInCollections;
+    static bool traceInCollection(Visitor* visitor, Member<CachedMatchedProperties>& cachedProperties, WTF::ShouldWeakPointersBeMarkedStrongly strongify)
+    {
+        // Only honor the cache's weakness semantics if the collection
+        // is traced without the strongify flag set to WeakPointersActStrong.

[haraken, 2014/06/25 13:37:11]

... if the collection is traced with WeakPointersActWeak.

[wibling-chromium, 2014/06/26 09:31:00]

Done.

+        // Otherwise just trace the cachedProperties strongly, ie. call trace
+        // on it.
+        if (strongify != WTF::WeakPointersActStrong) {

[haraken, 2014/06/25 13:37:11]

if (strongify == WTF::WeakPointersActWeak)

[wibling-chromium, 2014/06/26 09:31:00]

Done.

+            // A given cache entry is only kept alive if none of the MatchedProperties
+            // in the CachedMatchedProperties value contain a dead "properties" field.
+            // If there is a dead field the entire cache entry is removed.
+            HeapVector<MatchedProperties>::iterator it = cachedProperties->matchedProperties.begin();
+            HeapVector<MatchedProperties>::iterator end = cachedProperties->matchedProperties.end();
+            for (; it != end; ++it) {
+                if (!visitor->isAlive(it->properties)) {
+                    // For now report the cache entry as dead. This might not
+                    // be the final result if in a subsequent call to this

[haraken, 2014/06/25 13:37:11]

in a subsequent call to this the properties... => in a subsequent call the
properties...

[wibling-chromium, 2014/06/26 09:30:59]

I have rephrased it a bit.

+                    // the "properties" field has been marked via another
+                    // path.
+                    return true;
+                }
+            }
+        }
+        // At this point none of the entries in the matchedProperties vector
+        // had a dead "properties" field so trace CachedMatchedProperties normally.

[haraken, 2014/06/25 13:37:11]

normally => strongly

[wibling-chromium, 2014/06/26 09:30:59]

Done.

+        visitor->trace(cachedProperties);
+        return false;
+    }
+};
+#endif
+
 class MatchedPropertiesCache {
+    DISALLOW_ALLOCATION();
     WTF_MAKE_NONCOPYABLE(MatchedPropertiesCache);
 public:
     MatchedPropertiesCache();
 
     const CachedMatchedProperties* find(unsigned hash, const StyleResolverState&, const MatchResult&);
     void add(const RenderStyle*, const RenderStyle* parentStyle, unsigned hash, const MatchResult&);
 
     void clear();
     void clearViewportDependent();
 
     static bool isCacheable(const Element*, const RenderStyle*, const RenderStyle* parentStyle);
 
+    void trace(Visitor*);
+
 private:
+#if ENABLE(OILPAN)
+    typedef HeapHashMap<unsigned, Member<CachedMatchedProperties>, DefaultHash<unsigned>::Hash, HashTraits<unsigned>, CachedMatchedPropertiesHashTraits > Cache;

[haraken, 2014/06/25 13:37:11]

I don't fully understand why this is not a WeakMember.

Without making it a WeakMember, the GC won't remove dead entries from the map
when the GC does the thread-specific weak processing for the map, will it? (The
ephemeron iteration just determines whether each entry should be alive or dead
and does not do any work to remove dead entries from the map.)

Probably I'm misunderstanding.

[wibling-chromium, 2014/06/26 09:31:00]

There is no requirement for a WeakMember to be in the HashMap where we want
weakness. The reason is that we can simulate the weakness by only tracing a
given member (in this case cachedProperties) if it is already alive or if we are
asked to trace it strongly due to an iterator pointing to the map.

In our case we cannot have the nested properties field be a WeakMember because
the same data structure is used in MatchResult and in that case it must be a
Member to ensure we keep the properties alive when doing a conservative GC and
scanning it from the stack.

[haraken, 2014/06/26 10:31:36]

Makes sense, thanks for the clarification!

+#else
     // Every N additions to the matched declaration cache trigger a sweep where entries holding
     // the last reference to a style declaration are garbage collected.
     void sweep(Timer<MatchedPropertiesCache>*);
 
     unsigned m_additionsSinceLastSweep;
 
     typedef HashMap<unsigned, OwnPtr<CachedMatchedProperties> > Cache;
+    Timer<MatchedPropertiesCache> m_sweepTimer;
+#endif
     Cache m_cache;
-
-    Timer<MatchedPropertiesCache> m_sweepTimer;
 };
 
 }
 
 #endif