[Oilpan]: Make StylePropertySet fully garbage collected.

[Oilpan]: Make StylePropertySet fully garbage collected.

This also required moving ElementData and subclasses to the oilpan heap to
avoid a RefPtr -> Persistent -> Member cycle.

This change makes use of the ephemeron (weak hashmap with fix point)
support to remove elements in the MatchedPropertiesCache during a GC.
This is done by specializing the HashTrait for the value in the hash
map since it is the one with the embedded WeakMember. The specialized
HashTrait's traceInCollection method is called iteratively during GC
tracing to mark the entries that are live in the map. It is also called
during GC weak processing where it must report back whether a specific
element is alive or dead. If dead it is removed from the map.

One caveat is if the map is being traced strongly in which case the
weak members are traced as normal members. This can happen if there
is an iterator to the map on the stack.

I have also changed back to the add/assign pattern for putting entries
into hash maps to avoid the extra lookup I added. This should be safe
since we will have a pointer to the hash map's backing on the stack if
a GC occur so the backing and its entries should be marked strongly,
meaning it should be moved.
  
R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, tkent@chromium.org, vegorov@chromium.org, zerny@chromium.org
BUG=341815
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177012

[haraken, 2014-05-08 11:43:44]

+tasak, +esprehn

I'm not sure if the handling of MatcheProperties is correct.

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/StylePro...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/StylePro...
Source/core/css/StylePropertySet.h:44: class StylePropertySet : public
RefCountedWillBeGarbageCollectedFinalized<StylePropertySet> {

I think you can now make StylePropertySet GarbageCollected (not
GarbageCollectedFinalized), and remove finalizeGarbageCollected(). However,
let's do that in a follow-up CL.

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/MatchedPropertiesCache.cpp (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/MatchedPropertiesCache.cpp:73: sweep(0);

I understand what you're doing here, but this significantly increases the
likelihood of sweeping the cache. So we want to invent a better approach to
handle this.

> that caused the HeapHashMap in MatchedPropertiesCache to be resized > when
removing the dead elements which is not allowed during weak
> processing (it reallocates the backing)

Can we just disallow resize() during a GC? (I think that previously we were
disallowing resize() during a GC because we were not allowed to allocate any
on-heap object during a GC.)

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/MatchedPropertiesCache.cpp:151: if
(matchedProperties[i].properties->hasOneRef())

Even if we make m_cache a hash map of weak members, I'm afraid it won't solve
the problem. Given that StyleRule holds a strong reference to StylePropertySets
and that StyleRule is basically alive while a Document is alive (because
StyleElement has the same lifetime as the Document in common cases), the weak
members won't be cleared until the Document dies. Thus, even if we make m_cache
a hash map of weak members, we will end up bloating the cache until the Document
dies.

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/MatchedPropertiesCache.h (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/MatchedPropertiesCache.h:39: struct
CachedMatchedProperties : public
NoBaseWillBeGarbageCollectedFinalized<CachedMatchedProperties> {

Nit: struct => class

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:110:
DEFINE_STATIC_LOCAL(Persistent<MutableStylePropertySet>, leftToRightDecl,
(MutableStylePropertySet::create()));

You can avoid #if by using DEFINE_STATIC_REF_WILL_BE_PERSISTENT.

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:122:
DEFINE_STATIC_LOCAL(Persistent<MutableStylePropertySet>, rightToLeftDecl,
(MutableStylePropertySet::create()));

You can avoid #if by using DEFINE_STATIC_REF_WILL_BE_PERSISTENT.

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/ElementD...
File Source/core/dom/ElementData.cpp (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/ElementD...
Source/core/dom/ElementData.cpp:43: void* ptrs[3];

Nit: pointers

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/ElementD...
File Source/core/dom/ElementDataCache.cpp (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/ElementD...
Source/core/dom/ElementDataCache.cpp:50: ShareableElementDataCache::ValueType*
it = m_shareableElementDataCache.add(attributeHash(attributes),
nullptr).storedValue;

I think this is safe, but shall we rewrite this to the get/set pattern?

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/ElementD...
File Source/core/dom/ElementDataCache.h (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/ElementD...
Source/core/dom/ElementDataCache.h:44: class ElementDataCache {

You might want to move this to the heap in a follow-up.

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/Presenta...
File Source/core/dom/PresentationAttributeStyle.cpp (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/Presenta...
Source/core/dom/PresentationAttributeStyle.cpp:77: #endif // ENABLE(OILPAN)

We've introduced a bunch of #if's for this pattern. Probably we can avoid using
#if by doing something like:

  DEFINE_STATIC_LOCAL(RawPtrWillBePersistent<T>, xxx, (new T));

Let's consider doing that in a follow-up CL.

https://codereview.chromium.org/273843003/diff/20001/Source/core/html/HTMLTab...
File Source/core/html/HTMLTableElement.cpp (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableElement.cpp:540:
DEFINE_STATIC_LOCAL(Persistent<StylePropertySet>, rowBorderStyle,
(createGroupBorderStyle(true)));

Can you use DEFINE_STATIC_REF_WILL_BE_PERSISTENT?

https://codereview.chromium.org/273843003/diff/20001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableElement.cpp:547:
DEFINE_STATIC_LOCAL(Persistent<StylePropertySet>, columnBorderStyle,
(createGroupBorderStyle(false)));

Ditto.

[zerny-chromium, 2014-05-08 12:22:26]

It would be wonderful to localize the weak processing of the matched properties
cache, but that does not appear straight forward. The "dirty" flag you've added
here seems like the best way forward. At least it keeps the code very similar to
the non-oilpan build. lgtm

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/StylePro...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/StylePro...
Source/core/css/StylePropertySet.h:245: inline MutableStylePropertySet*
toMutableStylePropertySet(const RefPtrWillBeRawPtr<StylePropertySet>& set)
I wonder why the implicit conversion from Persistent/Member to RawPtr is not
sufficient to avoid the two definitions below.

[wibling-chromium, 2014-05-08 12:38:54]

Thanks for the review! Replies inline below.

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/StylePro...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/StylePro...
Source/core/css/StylePropertySet.h:44: class StylePropertySet : public
RefCountedWillBeGarbageCollectedFinalized<StylePropertySet> {
On 2014/05/08 11:43:45, haraken wrote:
> 
> I think you can now make StylePropertySet GarbageCollected (not
> GarbageCollectedFinalized), and remove finalizeGarbageCollected(). However,
> let's do that in a follow-up CL.

Yes, that seems right.

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/MatchedPropertiesCache.cpp (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/MatchedPropertiesCache.cpp:73: sweep(0);
On 2014/05/08 11:43:45, haraken wrote:
> 
> I understand what you're doing here, but this significantly increases the
> likelihood of sweeping the cache. So we want to invent a better approach to
> handle this.

It does increase the likelihood, but it will only happen once per GC or when the
timer fires (and that is only in oilpan). I have talked to Erik about if we can
get a way to process collections during weak callbacks and ensure there is no
reallocations which would be ideal.

> > that caused the HeapHashMap in MatchedPropertiesCache to be resized > when
> removing the dead elements which is not allowed during weak
> > processing (it reallocates the backing)
> 
> Can we just disallow resize() during a GC? (I think that previously we were
> disallowing resize() during a GC because we were not allowed to allocate any
> on-heap object during a GC.)

See above.

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/MatchedPropertiesCache.cpp:151: if
(matchedProperties[i].properties->hasOneRef())
On 2014/05/08 11:43:45, haraken wrote:
> 
> Even if we make m_cache a hash map of weak members, I'm afraid it won't solve
> the problem. Given that StyleRule holds a strong reference to
StylePropertySets
> and that StyleRule is basically alive while a Document is alive (because
> StyleElement has the same lifetime as the Document in common cases), the weak
> members won't be cleared until the Document dies. Thus, even if we make
m_cache
> a hash map of weak members, we will end up bloating the cache until the
Document
> dies.

I don't quite follow. If we make m_cache a hash map of weak members what will
keep CachedMatchedProperties alive? It was previously an OwnPtr so if we make it
weak it will just die in the next GC round. Or are you suggesting something
else?

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/MatchedPropertiesCache.h (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/MatchedPropertiesCache.h:39: struct
CachedMatchedProperties : public
NoBaseWillBeGarbageCollectedFinalized<CachedMatchedProperties> {
On 2014/05/08 11:43:45, haraken wrote:
> 
> Nit: struct => class

Done.

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:110:
DEFINE_STATIC_LOCAL(Persistent<MutableStylePropertySet>, leftToRightDecl,
(MutableStylePropertySet::create()));
On 2014/05/08 11:43:45, haraken wrote:
> 
> You can avoid #if by using DEFINE_STATIC_REF_WILL_BE_PERSISTENT.

Done.

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:122:
DEFINE_STATIC_LOCAL(Persistent<MutableStylePropertySet>, rightToLeftDecl,
(MutableStylePropertySet::create()));
On 2014/05/08 11:43:45, haraken wrote:
> 
> You can avoid #if by using DEFINE_STATIC_REF_WILL_BE_PERSISTENT.

Done.

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/ElementD...
File Source/core/dom/ElementData.cpp (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/ElementD...
Source/core/dom/ElementData.cpp:43: void* ptrs[3];
On 2014/05/08 11:43:45, haraken wrote:
> 
> Nit: pointers

Done.

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/ElementD...
File Source/core/dom/ElementDataCache.cpp (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/ElementD...
Source/core/dom/ElementDataCache.cpp:50: ShareableElementDataCache::ValueType*
it = m_shareableElementDataCache.add(attributeHash(attributes),
nullptr).storedValue;
On 2014/05/08 11:43:45, haraken wrote:
> 
> I think this is safe, but shall we rewrite this to the get/set pattern?

Done.

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/ElementD...
File Source/core/dom/ElementDataCache.h (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/ElementD...
Source/core/dom/ElementDataCache.h:44: class ElementDataCache {
On 2014/05/08 11:43:45, haraken wrote:
> 
> You might want to move this to the heap in a follow-up.

Yes, that is a prime candidate for a subsequent change.

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/Presenta...
File Source/core/dom/PresentationAttributeStyle.cpp (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/dom/Presenta...
Source/core/dom/PresentationAttributeStyle.cpp:77: #endif // ENABLE(OILPAN)
On 2014/05/08 11:43:45, haraken wrote:
> 
> We've introduced a bunch of #if's for this pattern. Probably we can avoid
using
> #if by doing something like:
> 
>   DEFINE_STATIC_LOCAL(RawPtrWillBePersistent<T>, xxx, (new T));
> 
> Let's consider doing that in a follow-up CL.

Okay. The reason for this is to avoid the non-oilpan version doing two
allocations, though I am not sure how much that will matter given it is defining
a static.

https://codereview.chromium.org/273843003/diff/20001/Source/core/html/HTMLTab...
File Source/core/html/HTMLTableElement.cpp (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableElement.cpp:540:
DEFINE_STATIC_LOCAL(Persistent<StylePropertySet>, rowBorderStyle,
(createGroupBorderStyle(true)));
On 2014/05/08 11:43:45, haraken wrote:
> 
> Can you use DEFINE_STATIC_REF_WILL_BE_PERSISTENT?

Done.

https://codereview.chromium.org/273843003/diff/20001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableElement.cpp:547:
DEFINE_STATIC_LOCAL(Persistent<StylePropertySet>, columnBorderStyle,
(createGroupBorderStyle(false)));
On 2014/05/08 11:43:45, haraken wrote:
> 
> Ditto.

Done.

[wibling-chromium, 2014-05-08 12:46:40]

Thanks for the review!

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/StylePro...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/StylePro...
Source/core/css/StylePropertySet.h:245: inline MutableStylePropertySet*
toMutableStylePropertySet(const RefPtrWillBeRawPtr<StylePropertySet>& set)
On 2014/05/08 12:22:26, zerny-chromium wrote:
> I wonder why the implicit conversion from Persistent/Member to RawPtr is not
> sufficient to avoid the two definitions below.

The problem is the compiler gets confused if there is not an exact match since
the DEFINE_TYPE_CASTS above declares a pointer version
toMutableStylePropertySet(StylePropertySet* set). I am getting

../../third_party/WebKit/Source/core/css/StyleRule.cpp:340:13: error: call to
'toMutableStylePropertySet' is ambiguous
    return *toMutableStylePropertySet(m_properties);
            ^~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/WebKit/Source/core/css/StylePropertySet.h:243:1: note:
candidate function
DEFINE_TYPE_CASTS(MutableStylePropertySet, StylePropertySet, set,
set->isMutable(), set.isMutable());
^
../../third_party/WebKit/Source/wtf/Assertions.h:368:18: note: expanded from
macro 'DEFINE_TYPE_CASTS'
inline thisType* to##thisType(argumentType* argumentName) \
                 ^
<scratch space>:43:1: note: expanded from here
toMutableStylePropertySet
^
../../third_party/WebKit/Source/core/css/StylePropertySet.h:243:1: note:
candidate function
../../third_party/WebKit/Source/wtf/Assertions.h:373:24: note: expanded from
macro 'DEFINE_TYPE_CASTS'
inline const thisType* to##thisType(const argumentType* argumentName) \
                       ^
<scratch space>:47:1: note: expanded from here
toMutableStylePropertySet
^
../../third_party/WebKit/Source/core/css/StylePropertySet.h:245:33: note:
candidate function
inline MutableStylePropertySet* toMutableStylePropertySet(const
RefPtrWillBeRawPtr<StylePropertySet>& set)
                                ^
../../third_party/WebKit/Source/core/css/StyleRule.cpp:436:13: error: call to
'toMutableStylePropertySet' is ambiguous
    return *toMutableStylePropertySet(m_properties);
            ^~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/WebKit/Source/core/css/StylePropertySet.h:243:1: note:
candidate function
DEFINE_TYPE_CASTS(MutableStylePropertySet, StylePropertySet, set,
set->isMutable(), set.isMutable());
^
../../third_party/WebKit/Source/wtf/Assertions.h:368:18: note: expanded from
macro 'DEFINE_TYPE_CASTS'
inline thisType* to##thisType(argumentType* argumentName) \
                 ^
<scratch space>:43:1: note: expanded from here
toMutableStylePropertySet
^

[haraken, 2014-05-08 12:53:37]

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/MatchedPropertiesCache.cpp (right):

https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/MatchedPropertiesCache.cpp:151: if
(matchedProperties[i].properties->hasOneRef())
On 2014/05/08 12:38:55, wibling-chromium wrote:
> On 2014/05/08 11:43:45, haraken wrote:
> > 
> > Even if we make m_cache a hash map of weak members, I'm afraid it won't
solve
> > the problem. Given that StyleRule holds a strong reference to
> StylePropertySets
> > and that StyleRule is basically alive while a Document is alive (because
> > StyleElement has the same lifetime as the Document in common cases), the
weak
> > members won't be cleared until the Document dies. Thus, even if we make
> m_cache
> > a hash map of weak members, we will end up bloating the cache until the
> Document
> > dies.
> 
> I don't quite follow. If we make m_cache a hash map of weak members what will
> keep CachedMatchedProperties alive? It was previously an OwnPtr so if we make
it
> weak it will just die in the next GC round. Or are you suggesting something
> else?
> 

(Probably I'm misunderstanding.)

- First of all, WillBeHeapHashMap<unsigned,
OwnPtrWillBeWeakMember<CachedMatchedProperties> > won't solve the issue. The
target of the weak processing should be
CachedMatchedProperties::matchedProperties[i].properties, not
CachedMatchedProperties.

- Even if we implement the weak processing for
CachedMatchedProperties::matchedProperties[i].properties, it won't solve the
issue either. This is because the StyleRule retains a strong member to
CachedMatchedProperties::matchedProperties[i].properties, and thus the weak
member is not cleared until the Document that contains the StyleRule dies. If
so, we will end up bloating the cache until the Document dies. (This is why
we're currently removing the cache entry when matchedProperties[i].properties
has one ref. The one ref is a reference from the StyleRule.)

tasak@ would know better about this. tasak@, would you correct me if I'm wrong?

[wibling-chromium, 2014-05-08 13:00:07]

On 2014/05/08 12:53:37, haraken wrote:
>
https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
> File Source/core/css/resolver/MatchedPropertiesCache.cpp (right):
> 
>
https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
> Source/core/css/resolver/MatchedPropertiesCache.cpp:151: if
> (matchedProperties[i].properties->hasOneRef())
> On 2014/05/08 12:38:55, wibling-chromium wrote:
> > On 2014/05/08 11:43:45, haraken wrote:
> > > 
> > > Even if we make m_cache a hash map of weak members, I'm afraid it won't
> solve
> > > the problem. Given that StyleRule holds a strong reference to
> > StylePropertySets
> > > and that StyleRule is basically alive while a Document is alive (because
> > > StyleElement has the same lifetime as the Document in common cases), the
> weak
> > > members won't be cleared until the Document dies. Thus, even if we make
> > m_cache
> > > a hash map of weak members, we will end up bloating the cache until the
> > Document
> > > dies.
> > 
> > I don't quite follow. If we make m_cache a hash map of weak members what
will
> > keep CachedMatchedProperties alive? It was previously an OwnPtr so if we
make
> it
> > weak it will just die in the next GC round. Or are you suggesting something
> > else?
> > 
> 
> (Probably I'm misunderstanding.)
> 
> - First of all, WillBeHeapHashMap<unsigned,
> OwnPtrWillBeWeakMember<CachedMatchedProperties> > won't solve the issue. The
> target of the weak processing should be
> CachedMatchedProperties::matchedProperties[i].properties, not
> CachedMatchedProperties.
> 

Correct. I guess I was confused about the "make it a hash map of weak members"
comment.

> - Even if we implement the weak processing for
> CachedMatchedProperties::matchedProperties[i].properties, it won't solve the
> issue either. This is because the StyleRule retains a strong member to
> CachedMatchedProperties::matchedProperties[i].properties, and thus the weak
> member is not cleared until the Document that contains the StyleRule dies. If
> so, we will end up bloating the cache until the Document dies. (This is why
> we're currently removing the cache entry when matchedProperties[i].properties
> has one ref. The one ref is a reference from the StyleRule.)
> 
> tasak@ would know better about this. tasak@, would you correct me if I'm
wrong?

As I understand it the cache will be cleared until it is the only thing holding
a RefPtr to the StylePropertySet. The one ref is not the one from StyleRule, but
the one from the MatchedProperties struct (it has its own RefPtr and only when
it is the last RefPtr will the entry be cleared).

[haraken, 2014-05-08 13:03:10]

On 2014/05/08 13:00:07, wibling-chromium wrote:
> On 2014/05/08 12:53:37, haraken wrote:
> >
>
https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
> > File Source/core/css/resolver/MatchedPropertiesCache.cpp (right):
> > 
> >
>
https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
> > Source/core/css/resolver/MatchedPropertiesCache.cpp:151: if
> > (matchedProperties[i].properties->hasOneRef())
> > On 2014/05/08 12:38:55, wibling-chromium wrote:
> > > On 2014/05/08 11:43:45, haraken wrote:
> > > > 
> > > > Even if we make m_cache a hash map of weak members, I'm afraid it won't
> > solve
> > > > the problem. Given that StyleRule holds a strong reference to
> > > StylePropertySets
> > > > and that StyleRule is basically alive while a Document is alive (because
> > > > StyleElement has the same lifetime as the Document in common cases), the
> > weak
> > > > members won't be cleared until the Document dies. Thus, even if we make
> > > m_cache
> > > > a hash map of weak members, we will end up bloating the cache until the
> > > Document
> > > > dies.
> > > 
> > > I don't quite follow. If we make m_cache a hash map of weak members what
> will
> > > keep CachedMatchedProperties alive? It was previously an OwnPtr so if we
> make
> > it
> > > weak it will just die in the next GC round. Or are you suggesting
something
> > > else?
> > > 
> > 
> > (Probably I'm misunderstanding.)
> > 
> > - First of all, WillBeHeapHashMap<unsigned,
> > OwnPtrWillBeWeakMember<CachedMatchedProperties> > won't solve the issue. The
> > target of the weak processing should be
> > CachedMatchedProperties::matchedProperties[i].properties, not
> > CachedMatchedProperties.
> > 
> 
> Correct. I guess I was confused about the "make it a hash map of weak members"
> comment.
> 
> > - Even if we implement the weak processing for
> > CachedMatchedProperties::matchedProperties[i].properties, it won't solve the
> > issue either. This is because the StyleRule retains a strong member to
> > CachedMatchedProperties::matchedProperties[i].properties, and thus the weak
> > member is not cleared until the Document that contains the StyleRule dies.
If
> > so, we will end up bloating the cache until the Document dies. (This is why
> > we're currently removing the cache entry when
matchedProperties[i].properties
> > has one ref. The one ref is a reference from the StyleRule.)
> > 
> > tasak@ would know better about this. tasak@, would you correct me if I'm
> wrong?
> 
> As I understand it the cache will be cleared until it is the only thing
holding
> a RefPtr to the StylePropertySet. The one ref is not the one from StyleRule,
but
> the one from the MatchedProperties struct (it has its own RefPtr and only when
> it is the last RefPtr will the entry be cleared).

ah, sorry I was misunderstanding.

Then if we can clear the cache using the weak processing for
CachedMatchedProperties::matchedProperties[i].properties, it will address the
issue.

[wibling-chromium, 2014-05-08 13:53:39]

On 2014/05/08 13:03:10, haraken wrote:
> On 2014/05/08 13:00:07, wibling-chromium wrote:
> > On 2014/05/08 12:53:37, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
> > > File Source/core/css/resolver/MatchedPropertiesCache.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/273843003/diff/20001/Source/core/css/resolver...
> > > Source/core/css/resolver/MatchedPropertiesCache.cpp:151: if
> > > (matchedProperties[i].properties->hasOneRef())
> > > On 2014/05/08 12:38:55, wibling-chromium wrote:
> > > > On 2014/05/08 11:43:45, haraken wrote:
> > > > > 
> > > > > Even if we make m_cache a hash map of weak members, I'm afraid it
won't
> > > solve
> > > > > the problem. Given that StyleRule holds a strong reference to
> > > > StylePropertySets
> > > > > and that StyleRule is basically alive while a Document is alive
(because
> > > > > StyleElement has the same lifetime as the Document in common cases),
the
> > > weak
> > > > > members won't be cleared until the Document dies. Thus, even if we
make
> > > > m_cache
> > > > > a hash map of weak members, we will end up bloating the cache until
the
> > > > Document
> > > > > dies.
> > > > 
> > > > I don't quite follow. If we make m_cache a hash map of weak members what
> > will
> > > > keep CachedMatchedProperties alive? It was previously an OwnPtr so if we
> > make
> > > it
> > > > weak it will just die in the next GC round. Or are you suggesting
> something
> > > > else?
> > > > 
> > > 
> > > (Probably I'm misunderstanding.)
> > > 
> > > - First of all, WillBeHeapHashMap<unsigned,
> > > OwnPtrWillBeWeakMember<CachedMatchedProperties> > won't solve the issue.
The
> > > target of the weak processing should be
> > > CachedMatchedProperties::matchedProperties[i].properties, not
> > > CachedMatchedProperties.
> > > 
> > 
> > Correct. I guess I was confused about the "make it a hash map of weak
members"
> > comment.
> > 
> > > - Even if we implement the weak processing for
> > > CachedMatchedProperties::matchedProperties[i].properties, it won't solve
the
> > > issue either. This is because the StyleRule retains a strong member to
> > > CachedMatchedProperties::matchedProperties[i].properties, and thus the
weak
> > > member is not cleared until the Document that contains the StyleRule dies.
> If
> > > so, we will end up bloating the cache until the Document dies. (This is
why
> > > we're currently removing the cache entry when
> matchedProperties[i].properties
> > > has one ref. The one ref is a reference from the StyleRule.)
> > > 
> > > tasak@ would know better about this. tasak@, would you correct me if I'm
> > wrong?
> > 
> > As I understand it the cache will be cleared until it is the only thing
> holding
> > a RefPtr to the StylePropertySet. The one ref is not the one from StyleRule,
> but
> > the one from the MatchedProperties struct (it has its own RefPtr and only
when
> > it is the last RefPtr will the entry be cleared).
> 
> ah, sorry I was misunderstanding.
> 
> Then if we can clear the cache using the weak processing for
> CachedMatchedProperties::matchedProperties[i].properties, it will address the
> issue.

That's correct. We can do that once we have support in the heap collections for
removing elements during weak processing. Erik is planning to look into that.

[wibling-chromium, 2014-05-09 06:26:11]

PTAL.

tasak@ or esprehn@ do you have time to take a look at this?

[esprehn, 2014-05-09 13:25:38]

On 2014/05/09 06:26:11, wibling-chromium wrote:
> PTAL.
> 
> tasak@ or esprehn@ do you have time to take a look at this?

I'll look today

[haraken, 2014-05-14 20:07:01]

Just a random idea:

- Probably can we use thread-local weak processing (instead of normal
processing) for CachedMatchedProperties::matchedProperties[i].properties? Since
thread-local weak processing is executed after a GC, it would be OK to allocate
objects in the thread-local weak processing.

- However, even if it works, I'm not sure if it's a right thing to do. The fact
that we have both normal weak processing and thread-local weak processing is
nasty, and we should fix it at some point. Ideally, I think we should have
thread-local weak processing that is executed before objects are swept. (Note:
currently, the normal weak processing is executed before objects are swept, and
the thread-local weak processing is executed after objects are swept.) If we're
going that way, we'll anyway need to disallow allocation during thread-local
weak processing. If so, it is wrong to try to address the issue by using
thread-local weak processing in which object allocation is currently allowed (by
accident).

- In conclusion, I think that a right fix here is to disallow allocation of heap
collections during weak processing, as wibling and Erik mentioned.

[esprehn, 2014-05-14 20:25:26]

The hash table changes in this patch are not correct, like
ElementDataCache::cachedShareableElementDataWithAttributes and the other one.

You're adding extra hash table lookups in all these code paths.

I'm also not comfortable with allowing dead pointers to live in the cache even
after a GC, why is the cache not properly setup so that the Weak members are
either cleared by the GC?

https://codereview.chromium.org/273843003/diff/60001/Source/core/css/resolver...
File Source/core/css/resolver/MatchedPropertiesCache.cpp (right):

https://codereview.chromium.org/273843003/diff/60001/Source/core/css/resolver...
Source/core/css/resolver/MatchedPropertiesCache.cpp:70: // If we had a GC since
the last find we could have entries that are now invalid.
This feels very wrong, why are we allowing dead pointers to live after a GC?
This feels like a use-after-free waiting to happen. We shouldn't switch
something to oilpan until we can ensure that dead pointers don't live after the
GC kills an object.

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/ElementD...
File Source/core/dom/ElementData.cpp (right):

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/ElementD...
Source/core/dom/ElementData.cpp:163:
static_cast<UniqueElementData*>(this)->traceAfterDispatch(visitor);
What is traceAfterDispatch? what is dispatch? Why does trace() sometimes need to
call traceAfterDispatch, and sometimes need to call trace?

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/ElementD...
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/ElementD...
Source/core/dom/ElementData.h:144: void* operator new(std::size_t, void*
location)
Why is this needed? Can you add a comment?

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/ElementD...
File Source/core/dom/ElementDataCache.cpp (right):

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/ElementD...
Source/core/dom/ElementDataCache.cpp:56: elementData =
m_shareableElementDataCache.set(attributeHash(attributes),
ShareableElementData::createWithAttributes(attributes)).storedValue->value.get();
You make us do two hash lookups now instead of one, this is wrong.

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/Presenta...
File Source/core/dom/PresentationAttributeStyle.cpp (right):

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/Presenta...
Source/core/dom/PresentationAttributeStyle.cpp:176: if (cacheEntry &&
cacheEntry->key != cacheKey)
Why did you change this? Why doesn't the Oilpan type using the same API as the
WTF type?

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/Presenta...
Source/core/dom/PresentationAttributeStyle.cpp:208:
presentationAttributeCache().set(cacheHash, newEntry.release());
This is again wrong, you make us do two hash lookups instead of one.

[haraken, 2014-05-14 21:45:35]

> I'm also not comfortable with allowing dead pointers to live in the cache even
> after a GC, why is the cache not properly setup so that the Weak members are
> either cleared by the GC?

Your concern is very valid. The CL uses the GC hack because we currently can't
use weak members for the cache entry because:

- object allocation is not allowed during weak processing
- weak processing for the cache entry can remove items from heap collections,
which can trigger resize(), which can allocate objects

The point of my previous comment #12 is that we should disable resize() of heap
collections during weak processing so that we can use weak members for the cache
entry. Then the GC hack will be gone.

Erik is already working on it and I think we can reupload a CL after that.

[wibling-chromium, 2014-05-15 13:15:17]

Thanks for the review.

I have replied inline below to your specific questions/comments.

Given your feedback I will wait with the change until we get support for
removing elements during weak pointer processing.

https://codereview.chromium.org/273843003/diff/60001/Source/core/css/resolver...
File Source/core/css/resolver/MatchedPropertiesCache.cpp (right):

https://codereview.chromium.org/273843003/diff/60001/Source/core/css/resolver...
Source/core/css/resolver/MatchedPropertiesCache.cpp:70: // If we had a GC since
the last find we could have entries that are now invalid.
On 2014/05/14 20:25:27, esprehn wrote:
> This feels very wrong, why are we allowing dead pointers to live after a GC?
> This feels like a use-after-free waiting to happen. We shouldn't switch
> something to oilpan until we can ensure that dead pointers don't live after
the
> GC kills an object.

This is a temporary work around to the issue Haraken is mentioning. Basically we
cannot remove elements from our heap hash maps during the GC weak processing.
The reason being that we don't allow reallocation of the hash map's backing
which doing a GC.
That said there are no dead pointers. The pointers in the HeapVector are cleared
(NULL) during the GC since they are WeakMembers, but due to the above
restriction we cannot remove the containing HashMap entry until we are out of
the GC. Erik is working on enhancing our collections to support this at which
point we will do the entire cleanup during GC.

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/ElementD...
File Source/core/dom/ElementData.cpp (right):

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/ElementD...
Source/core/dom/ElementData.cpp:163:
static_cast<UniqueElementData*>(this)->traceAfterDispatch(visitor);
On 2014/05/14 20:25:27, esprehn wrote:
> What is traceAfterDispatch? what is dispatch? Why does trace() sometimes need
to
> call traceAfterDispatch, and sometimes need to call trace?

This is a common pattern we use when objects don't have a vtable. Ideally we
want to use a virtual dispatch, but that will add an extra word of memory to
each object. To avoid this we do this manual dispatch to the subclass'
traceAfterDispatch method which then does the actual tracing.

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/ElementD...
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/ElementD...
Source/core/dom/ElementData.h:144: void* operator new(std::size_t, void*
location)
On 2014/05/14 20:25:27, esprehn wrote:
> Why is this needed? Can you add a comment?

Sure. I will add a comment. This is needed because ShareableElementData and
UniqueElementData are not allocated with a fixed size. Instead the allocated
memory size is computed based on the number of attributes. This requires us to
call Heap::allocate directly with the size and subsequently call placement new
with the allocated memory address.
We don't normally support this pattern for objects and so haven't defined the
placement new operator on garbage collected objects by default. If we see this
pattern enough we will come up with a generic solution for it.

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/ElementD...
File Source/core/dom/ElementDataCache.cpp (right):

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/ElementD...
Source/core/dom/ElementDataCache.cpp:56: elementData =
m_shareableElementDataCache.set(attributeHash(attributes),
ShareableElementData::createWithAttributes(attributes)).storedValue->value.get();
On 2014/05/14 20:25:27, esprehn wrote:
> You make us do two hash lookups now instead of one, this is wrong.

The reason is that the createWithAttributes call might trigger a GC which could
cause the backing of the hash map to be reallocated (ie. moved). In that case
the pointer to the storedValue would be incorrect.

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/Presenta...
File Source/core/dom/PresentationAttributeStyle.cpp (right):

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/Presenta...
Source/core/dom/PresentationAttributeStyle.cpp:176: if (cacheEntry &&
cacheEntry->key != cacheKey)
On 2014/05/14 20:25:27, esprehn wrote:
> Why did you change this? Why doesn't the Oilpan type using the same API as the
> WTF type?

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.

https://codereview.chromium.org/273843003/diff/60001/Source/core/dom/Presenta...
Source/core/dom/PresentationAttributeStyle.cpp:208:
presentationAttributeCache().set(cacheHash, newEntry.release());
On 2014/05/14 20:25:27, esprehn wrote:
> This is again wrong, you make us do two hash lookups instead of one.

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.

[haraken, 2014-06-10 15:30:41]

Just a reminder: Any update on this CL? :)

[Mads Ager (chromium), 2014-06-10 15:35:37]

Yes, it is waiting for Erik's change to allow more flexible weak processing.
Erik is landing that change now so Gustav should be able to get back to this one
soon! :)

[wibling-chromium, 2014-06-24 15:11:14]

PTAL. I have updated the change to use Erik's new ephemeron infrastructure.

[zerny-chromium, 2014-06-25 06:19:02]

LGTM! This is the way it should be structured. Hurrah for custom weak processing
and ephemerons :-)

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
File Source/core/css/resolver/MatchedPropertiesCache.h (right):

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:39: class
CachedMatchedProperties : public
NoBaseWillBeGarbageCollectedFinalized<CachedMatchedProperties> {
Nit: FINAL

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:67: while (it != end) {
Nit: for(; it != end; ++it)

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:71: isDead = true;
It looks like this might as well be:

  return true;

and the code after the loop specialized to isDead == false.

https://codereview.chromium.org/273843003/diff/100001/Source/core/dom/Present...
File Source/core/dom/PresentationAttributeStyle.cpp (right):

https://codereview.chromium.org/273843003/diff/100001/Source/core/dom/Present...
Source/core/dom/PresentationAttributeStyle.cpp:60: struct
PresentationAttributeCacheEntry : public
NoBaseWillBeGarbageCollectedFinalized<PresentationAttributeCacheEntry> {
Nit: FINAL

https://codereview.chromium.org/273843003/diff/100001/Source/core/dom/Present...
Source/core/dom/PresentationAttributeStyle.cpp:209: cacheValue->value
=newEntry.release();
Nit: unintended edit

[Mads Ager (chromium), 2014-06-25 06:31:48]

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
File Source/core/css/resolver/MatchResult.h (right):

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/MatchResult.h:78: namespace WTF {
You can use
WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS(WebCore::MathcedProperties> here.

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/MatchResult.h:92: WillBeHeapVector<MatchedProperties,
64> matchedProperties;
In MatchResult you are using the normal tracing of the MatchedProperties objects
which means that the WeakMember<StylePropertySet> can be cleared out if it is
the only reference. Is that what we want? I would guess that for the MatchResult
we want the pointer in MatchedProperties to be strong.

I think we can probably use a Member instead of a weak member because the
processing of the table is controlled by the trait and therefore we can do the
custom weak processing even though the field is a Member instead of a
WeakMember?

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
File Source/core/css/resolver/MatchedPropertiesCache.h (right):

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:53: // entries in the
MatchedPropertiesCache.
MatchedPropertiesCache -> matchedProperties vector?

[Erik Corry, 2014-06-25 06:58:25]

lgtm

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
File Source/core/css/resolver/MatchResult.h (right):

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/MatchResult.h:92: WillBeHeapVector<MatchedProperties,
64> matchedProperties;
On 2014/06/25 06:31:48, Mads Ager (chromium) wrote:
> In MatchResult you are using the normal tracing of the MatchedProperties
objects
> which means that the WeakMember<StylePropertySet> can be cleared out if it is
> the only reference. Is that what we want? I would guess that for the
MatchResult
> we want the pointer in MatchedProperties to be strong.

I agree in principle, but it's sort of subtle. MatchResult is on the stack so it
is only ever found conservatively, and the pointer from the MatchResult to the
backing of the vector will look just like an iterator, so the backing of the
heap vector will be scanned with strongify on.

However, MatchedProperties does not have support for special weak processing, so
it will ignore the strongify flag and just zap the weak member while leaving the
entry in the collection. Because it's a vector we don't support the full weak
processing machinery (perhaps we should) so I think just adding special weak
processing support to MatchedProperties will not work right now. So Mads'
suggestion may be better.

There was talk (for something else) of adding support for weakness in vectors
that just compacts the vector, collecting all the surviving entries near the
start. It would support iterator-based iteration over weak vectors, but it would
fail for integer based iteration which would not trigger the strongified weak
processing. (If there was a GC during iteration.)

A side effect of adding that support for weakness in vectors would be that you
could fix this case cleanly just by having the right trait for
MatchedProperties. Though in this case the
compacting-vectors-when-removing-weak-entries support would never compact the
vector because the MatchResult acts like an iterator.

> I think we can probably use a Member instead of a weak member because the
> processing of the table is controlled by the trait and therefore we can do the
> custom weak processing even though the field is a Member instead of a
> WeakMember?

It's a nice documenting of the intent to have it be a WeakMember though.

[Mads Ager (chromium), 2014-06-25 07:17:13]

On 2014/06/25 06:58:25, Erik Corry wrote:
> lgtm
> 
>
https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
> File Source/core/css/resolver/MatchResult.h (right):
> 
>
https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
> Source/core/css/resolver/MatchResult.h:92: WillBeHeapVector<MatchedProperties,
> 64> matchedProperties;
> On 2014/06/25 06:31:48, Mads Ager (chromium) wrote:
> > In MatchResult you are using the normal tracing of the MatchedProperties
> objects
> > which means that the WeakMember<StylePropertySet> can be cleared out if it
is
> > the only reference. Is that what we want? I would guess that for the
> MatchResult
> > we want the pointer in MatchedProperties to be strong.
> 
> I agree in principle, but it's sort of subtle. MatchResult is on the stack so
it
> is only ever found conservatively, and the pointer from the MatchResult to the
> backing of the vector will look just like an iterator, so the backing of the
> heap vector will be scanned with strongify on.
> 
> However, MatchedProperties does not have support for special weak processing,
so
> it will ignore the strongify flag and just zap the weak member while leaving
the
> entry in the collection. Because it's a vector we don't support the full weak
> processing machinery (perhaps we should) so I think just adding special weak
> processing support to MatchedProperties will not work right now. So Mads'
> suggestion may be better.
> 
> There was talk (for something else) of adding support for weakness in vectors
> that just compacts the vector, collecting all the surviving entries near the
> start. It would support iterator-based iteration over weak vectors, but it
would
> fail for integer based iteration which would not trigger the strongified weak
> processing. (If there was a GC during iteration.)
> 
> A side effect of adding that support for weakness in vectors would be that you
> could fix this case cleanly just by having the right trait for
> MatchedProperties. Though in this case the
> compacting-vectors-when-removing-weak-entries support would never compact the
> vector because the MatchResult acts like an iterator.
> 
> > I think we can probably use a Member instead of a weak member because the
> > processing of the table is controlled by the trait and therefore we can do
the
> > custom weak processing even though the field is a Member instead of a
> > WeakMember?
> 
> It's a nice documenting of the intent to have it be a WeakMember though.

Yes, but as you explained above it does not work (not just in principle; in
practice too). Therefore, we cannot use a WeakMember here at this point. We need
a working solution for this before we can commit. Using Member and having
special handling of the cache tracing that actually does not trace the Member in
certain cases where instead we will remove it from the cache is the analogous of
the RefPtr and special handling that is currently used and would allow us to
move forward with this change. I am fine with other suggestions as well, but
keeping the WeakMember with the current handling doesn't work.

[wibling-chromium, 2014-06-25 09:03:28]

Thanks for the reviews! I have updated the diff based on the feedback.

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
File Source/core/css/resolver/MatchResult.h (right):

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/MatchResult.h:78: namespace WTF {
On 2014/06/25 06:31:48, Mads Ager (chromium) wrote:
> You can use
> WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS(WebCore::MathcedProperties> here.

Done.

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/MatchResult.h:92: WillBeHeapVector<MatchedProperties,
64> matchedProperties;
On 2014/06/25 06:31:48, Mads Ager (chromium) wrote:
> In MatchResult you are using the normal tracing of the MatchedProperties
objects
> which means that the WeakMember<StylePropertySet> can be cleared out if it is
> the only reference. Is that what we want? I would guess that for the
MatchResult
> we want the pointer in MatchedProperties to be strong.
> 
> I think we can probably use a Member instead of a weak member because the
> processing of the table is controlled by the trait and therefore we can do the
> custom weak processing even though the field is a Member instead of a
> WeakMember?

Nice catch. I have changed it be a Member instead. As you say the tracing is
conditional anyway in the specialized traceInCollection method.

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
File Source/core/css/resolver/MatchedPropertiesCache.h (right):

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:39: class
CachedMatchedProperties : public
NoBaseWillBeGarbageCollectedFinalized<CachedMatchedProperties> {
On 2014/06/25 06:19:02, zerny-chromium wrote:
> Nit: FINAL

Done.

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:53: // entries in the
MatchedPropertiesCache.
On 2014/06/25 06:31:48, Mads Ager (chromium) wrote:
> MatchedPropertiesCache -> matchedProperties vector?

I was thinking about it being the entries in the cache that I am checking. The
check involves running through the matchedProperties vector, but the trait is
for the value element in the MatchedProperties' m_cache.

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:67: while (it != end) {
On 2014/06/25 06:19:02, zerny-chromium wrote:
> Nit: for(; it != end; ++it)

Done.

https://codereview.chromium.org/273843003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:71: isDead = true;
On 2014/06/25 06:19:02, zerny-chromium wrote:
> It looks like this might as well be:
> 
>   return true;
> 
> and the code after the loop specialized to isDead == false.

Done.

https://codereview.chromium.org/273843003/diff/100001/Source/core/dom/Present...
File Source/core/dom/PresentationAttributeStyle.cpp (right):

https://codereview.chromium.org/273843003/diff/100001/Source/core/dom/Present...
Source/core/dom/PresentationAttributeStyle.cpp:60: struct
PresentationAttributeCacheEntry : public
NoBaseWillBeGarbageCollectedFinalized<PresentationAttributeCacheEntry> {
On 2014/06/25 06:19:02, zerny-chromium wrote:
> Nit: FINAL

Done.

https://codereview.chromium.org/273843003/diff/100001/Source/core/dom/Present...
Source/core/dom/PresentationAttributeStyle.cpp:209: cacheValue->value
=newEntry.release();
On 2014/06/25 06:19:02, zerny-chromium wrote:
> Nit: unintended edit

Done.

[Mads Ager (chromium), 2014-06-25 09:18:33]

Thanks Gustav! LGTM

https://codereview.chromium.org/273843003/diff/140001/Source/core/css/resolve...
File Source/core/css/resolver/MatchedPropertiesCache.h (right):

https://codereview.chromium.org/273843003/diff/140001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:64: for (;it != end; ++it) {
space after ;

https://codereview.chromium.org/273843003/diff/140001/Source/core/dom/Element...
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/273843003/diff/140001/Source/core/dom/Element...
Source/core/dom/ElementData.h:73: // dispatch to the correct subclass'
destructor.
Nit: subclass's destructor or subclass destructor

[wibling-chromium, 2014-06-25 09:30:47]

Thanks for the review!

https://codereview.chromium.org/273843003/diff/140001/Source/core/css/resolve...
File Source/core/css/resolver/MatchedPropertiesCache.h (right):

https://codereview.chromium.org/273843003/diff/140001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:64: for (;it != end; ++it) {
On 2014/06/25 09:18:33, Mads Ager (chromium) wrote:
> space after ;

Done.

https://codereview.chromium.org/273843003/diff/140001/Source/core/dom/Element...
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/273843003/diff/140001/Source/core/dom/Element...
Source/core/dom/ElementData.h:73: // dispatch to the correct subclass'
destructor.
On 2014/06/25 09:18:33, Mads Ager (chromium) wrote:
> Nit: subclass's destructor or subclass destructor

I tend not to add an extra s when the word is ending in s. I have heard some
people have heated debates about this, so if you feel strongly I will add a s:)

[wibling-chromium, 2014-06-25 09:39:20]

Elliott,

Do you have time for a follow-up look at this. We now have support for removing
elements in a hashmap during GC.

[esprehn, 2014-06-25 11:02:58]

On 2014/06/25 at 09:39:20, wibling wrote:
> Elliott,
> 
> Do you have time for a follow-up look at this. We now have support for
removing elements in a hashmap during GC.

Sure, I'll look later today.

[wibling-chromium, 2014-06-25 11:14:33]

On 2014/06/25 11:02:58, esprehn wrote:
> On 2014/06/25 at 09:39:20, wibling wrote:
> > Elliott,
> > 
> > Do you have time for a follow-up look at this. We now have support for
> removing elements in a hashmap during GC.
> 
> Sure, I'll look later today.

Thanks!

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

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
File Source/core/css/resolver/MatchedPropertiesCache.cpp (right):

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.cpp:118:
WillBeHeapVector<unsigned, 16> toRemove;

Shall we revert this back to a off-heap Vector, since we don't yet move vectors
of primitive types to the heap?

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
File Source/core/css/resolver/MatchedPropertiesCache.h (right):

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:60: // is traced without the
strongify flag set to WeakPointersActStrong.

... if the collection is traced with WeakPointersActWeak.

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:63: if (strongify !=
WTF::WeakPointersActStrong) {

if (strongify == WTF::WeakPointersActWeak)

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:72: // be the final result if
in a subsequent call to this

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

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:80: // had a dead "properties"
field so trace CachedMatchedProperties normally.

normally => strongly

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:105: typedef
HeapHashMap<unsigned, Member<CachedMatchedProperties>,
DefaultHash<unsigned>::Hash, HashTraits<unsigned>,
CachedMatchedPropertiesHashTraits > Cache;

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.

https://codereview.chromium.org/273843003/diff/180001/Source/core/dom/Present...
File Source/core/dom/PresentationAttributeStyle.cpp (right):

https://codereview.chromium.org/273843003/diff/180001/Source/core/dom/Present...
Source/core/dom/PresentationAttributeStyle.cpp:78: #endif // ENABLE(OILPAN)

You can write:

  DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<PresentationAttributeCache>, cache,
(new PresentationAttributeCache()));
  return *cache;

https://codereview.chromium.org/273843003/diff/180001/Source/core/dom/Present...
Source/core/dom/PresentationAttributeStyle.cpp:183:
RefPtrWillBeRawPtr<StylePropertySet> style;

= nullptr;

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

Thanks for the review. I have replied below and will update the diff ASAP.

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
File Source/core/css/resolver/MatchedPropertiesCache.cpp (right):

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.cpp:118:
WillBeHeapVector<unsigned, 16> toRemove;
On 2014/06/25 13:37:11, haraken wrote:
> 
> Shall we revert this back to a off-heap Vector, since we don't yet move
vectors
> of primitive types to the heap?

Done.

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
File Source/core/css/resolver/MatchedPropertiesCache.h (right):

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:60: // is traced without the
strongify flag set to WeakPointersActStrong.
On 2014/06/25 13:37:11, haraken wrote:
> 
> ... if the collection is traced with WeakPointersActWeak.

Done.

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:63: if (strongify !=
WTF::WeakPointersActStrong) {
On 2014/06/25 13:37:11, haraken wrote:
> 
> if (strongify == WTF::WeakPointersActWeak)

Done.

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:72: // be the final result if
in a subsequent call to this
On 2014/06/25 13:37:11, haraken wrote:
> 
> in a subsequent call to this the properties... => in a subsequent call the
> properties...

I have rephrased it a bit.

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:80: // had a dead "properties"
field so trace CachedMatchedProperties normally.
On 2014/06/25 13:37:11, haraken wrote:
> 
> normally => strongly

Done.

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:105: typedef
HeapHashMap<unsigned, Member<CachedMatchedProperties>,
DefaultHash<unsigned>::Hash, HashTraits<unsigned>,
CachedMatchedPropertiesHashTraits > Cache;
On 2014/06/25 13:37:11, haraken wrote:
> 
> 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.

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.

https://codereview.chromium.org/273843003/diff/180001/Source/core/dom/Present...
File Source/core/dom/PresentationAttributeStyle.cpp (right):

https://codereview.chromium.org/273843003/diff/180001/Source/core/dom/Present...
Source/core/dom/PresentationAttributeStyle.cpp:78: #endif // ENABLE(OILPAN)
On 2014/06/25 13:37:11, haraken wrote:
> 
> You can write:
> 
>   DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<PresentationAttributeCache>,
cache,
> (new PresentationAttributeCache()));
>   return *cache;

Done.

https://codereview.chromium.org/273843003/diff/180001/Source/core/dom/Present...
Source/core/dom/PresentationAttributeStyle.cpp:183:
RefPtrWillBeRawPtr<StylePropertySet> style;
On 2014/06/25 13:37:11, haraken wrote:
> 
> = nullptr;

Done.

[esprehn, 2014-06-26 09:48:54]

This lgtm, but this Oilpan stuff is making me nervous. All of these caches that
we used to rarely visit and that have thousands of entries are going to mean
thousands of function calls that we didn't need to make before whenever Oilpan
decides it's time to GC.

How's perf testing coming along?

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
File Source/core/css/resolver/MatchedPropertiesCache.h (right):

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:55: struct
CachedMatchedPropertiesHashTraits : HashTraits<Member<CachedMatchedProperties> >
{
Does this really need to be inline? I'd rather it didn't add so much code to
this header.

[wibling-chromium, 2014-06-26 09:58:41]

On 2014/06/26 09:48:54, esprehn wrote:
> This lgtm, but this Oilpan stuff is making me nervous. All of these caches
that
> we used to rarely visit and that have thousands of entries are going to mean
> thousands of function calls that we didn't need to make before whenever Oilpan
> decides it's time to GC.
> 
> How's perf testing coming along?
> 
>
https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
> File Source/core/css/resolver/MatchedPropertiesCache.h (right):
> 
>
https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
> Source/core/css/resolver/MatchedPropertiesCache.h:55: struct
> CachedMatchedPropertiesHashTraits : HashTraits<Member<CachedMatchedProperties>
>
> {
> Does this really need to be inline? I'd rather it didn't add so much code to
> this header.

Hi Elliott,

Thanks for the review.

Regarding the tracing that is something we had to do independent of this change.
Oilpan already traces a lot of large datastructure and yes, that does affect
performance:) Right now the performance tests are mixed, some are better, some
are untouched, and some are definitely worse. The last is our focus for Q3. The
goal being to get performance on par with current ref-counted version.

I will look into if I can move the body of the traceInCollection method to the
.cpp file.

/gustav

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

https://codereview.chromium.org/273843003/diff/180001/Source/core/dom/Present...
File Source/core/dom/PresentationAttributeStyle.cpp (right):

https://codereview.chromium.org/273843003/diff/180001/Source/core/dom/Present...
Source/core/dom/PresentationAttributeStyle.cpp:78: #endif // ENABLE(OILPAN)
On 2014/06/25 13:37:11, haraken wrote:
> 
> You can write:
> 
>   DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<PresentationAttributeCache>,
cache,
> (new PresentationAttributeCache()));
>   return *cache;

Actually this seems to not compile on the non-oilpan build because the OwnPtr is
non-copyable. I will revert back to the previous ifdef.

[Mads Ager (chromium), 2014-06-26 10:03:17]

On 2014/06/26 09:59:41, wibling-chromium wrote:
>
https://codereview.chromium.org/273843003/diff/180001/Source/core/dom/Present...
> File Source/core/dom/PresentationAttributeStyle.cpp (right):
> 
>
https://codereview.chromium.org/273843003/diff/180001/Source/core/dom/Present...
> Source/core/dom/PresentationAttributeStyle.cpp:78: #endif // ENABLE(OILPAN)
> On 2014/06/25 13:37:11, haraken wrote:
> > 
> > You can write:
> > 
> >   DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<PresentationAttributeCache>,
> cache,
> > (new PresentationAttributeCache()));
> >   return *cache;
> 
> Actually this seems to not compile on the non-oilpan build because the OwnPtr
is
> non-copyable. I will revert back to the previous ifdef.

Did you remember to do adoptPtrWillBeNoop?

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

LGTM

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
File Source/core/css/resolver/MatchedPropertiesCache.h (right):

https://codereview.chromium.org/273843003/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/MatchedPropertiesCache.h:105: typedef
HeapHashMap<unsigned, Member<CachedMatchedProperties>,
DefaultHash<unsigned>::Hash, HashTraits<unsigned>,
CachedMatchedPropertiesHashTraits > Cache;
On 2014/06/26 09:31:00, wibling-chromium wrote:
> On 2014/06/25 13:37:11, haraken wrote:
> > 
> > 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.
> 
> 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.

Makes sense, thanks for the clarification!

[wibling-chromium, 2014-06-26 10:36:50]

On 2014/06/26 10:03:17, Mads Ager (chromium) wrote:
> On 2014/06/26 09:59:41, wibling-chromium wrote:
> >
>
https://codereview.chromium.org/273843003/diff/180001/Source/core/dom/Present...
> > File Source/core/dom/PresentationAttributeStyle.cpp (right):
> > 
> >
>
https://codereview.chromium.org/273843003/diff/180001/Source/core/dom/Present...
> > Source/core/dom/PresentationAttributeStyle.cpp:78: #endif // ENABLE(OILPAN)
> > On 2014/06/25 13:37:11, haraken wrote:
> > > 
> > > You can write:
> > > 
> > >   DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<PresentationAttributeCache>,
> > cache,
> > > (new PresentationAttributeCache()));
> > >   return *cache;
> > 
> > Actually this seems to not compile on the non-oilpan build because the
OwnPtr
> is
> > non-copyable. I will revert back to the previous ifdef.
> 
> Did you remember to do adoptPtrWillBeNoop?

Doh, that seems to be the issue, back again to Haraken's suggestion.

[wibling-chromium, 2014-06-26 10:37:30]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-06-26 10:38:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wibling@chromium.org/273843003/240001

[commit-bot: I haz the power, 2014-06-26 11:46:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[commit-bot: I haz the power, 2014-06-26 18:14:55]

Change committed as 177012