[oilpan]: Avoid refcounting QualifiedName's nullQName when tracing.

[oilpan]: Avoid refcounting QualifiedName's nullQName when tracing.
    
When doing tracing across threads I hit an assert due to tracing the SMILTimeContainer's hashmap from a different thread than the one it was allocated on. Basically in HashTable's trace method we call isEmptyOrDeletedBucket which ends up calling PairHashTraits::emptyValue which makes a pair<WeakMember<SVGElement>, QualifiedName> initialized with the individual empty values. This causes the nullQName to be copied to the new pair which causes a ref() in RefCountedBase hitting ASSERT(m_verifier.isSafeToUse()).

With this change all global or static local QualifiedNames are ignoring ref counting. This is similar to the way StringImpl works.

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=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176307

[haraken, 2014-06-03 14:55:33]

Another idea might be to simplify the data structure.

Currently we're using:

HeapHashMap<pair<WeakMember<SVGElement>, QualifiedName>,
Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>>

Can we change this to something like:

class ElementAttributePair {
  WeakMember<SVGElement> m_element;
  QualifiedName m_name;
};

HeapHashMap<Member<ElementAttributePair>,
Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>>

?

[Mads Ager (chromium), 2014-06-04 06:37:52]

On 2014/06/03 14:55:33, haraken wrote:
> Another idea might be to simplify the data structure.
> 
> Currently we're using:
> 
> HeapHashMap<pair<WeakMember<SVGElement>, QualifiedName>,
> Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>>
> 
> Can we change this to something like:
> 
> class ElementAttributePair {
>   WeakMember<SVGElement> m_element;
>   QualifiedName m_name;
> };
> 
> HeapHashMap<Member<ElementAttributePair>,
> Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>>
> 
> ?

I think we will end up in the same place if we do that. In order to use
ElementAttributePair as the key in a HashMap you have to provide a hash trait
for it...

[Mads Ager (chromium), 2014-06-04 07:01:41]

On 2014/06/04 06:37:52, Mads Ager (chromium) wrote:
> On 2014/06/03 14:55:33, haraken wrote:
> > Another idea might be to simplify the data structure.
> > 
> > Currently we're using:
> > 
> > HeapHashMap<pair<WeakMember<SVGElement>, QualifiedName>,
> > Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>>
> > 
> > Can we change this to something like:
> > 
> > class ElementAttributePair {
> >   WeakMember<SVGElement> m_element;
> >   QualifiedName m_name;
> > };
> > 
> > HeapHashMap<Member<ElementAttributePair>,
> > Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>>
> > 
> > ?
> 
> I think we will end up in the same place if we do that. In order to use
> ElementAttributePair as the key in a HashMap you have to provide a hash trait
> for it...

Ian points out that I misunderstood. ;-)

What you are suggesting is using a Member pointer as the key which hashes just
fine.

I think there are two separate issues that we need to address here:

1) marking crashes because of threading issues. That is what this change is
fixing. I wonder if we can fix that by changing the QualifiedName hash trait to
not use the null qualified name as the empty value?

2) this is a weak hash map but that does not work for pairs. That is, we do not
actually get the values pulled out of a hashmap when the weak part of a
pair<WeakMember<T>, U> dies. We currently have a workaround for that which is
that we collect invalid keys when we iterate the map and pull them out manually
at that point. However, we should make the automatic clearing work for this.

Haraken, your solution will fix 1) but it will make the hash set strong and make
it impossible to make weak processing clear this map automatically. Therefore,
it might be a step in the wrong direction for dealing with 2).

[haraken, 2014-06-04 07:10:35]

On 2014/06/04 07:01:41, Mads Ager (chromium) wrote:
> On 2014/06/04 06:37:52, Mads Ager (chromium) wrote:
> > On 2014/06/03 14:55:33, haraken wrote:
> > > Another idea might be to simplify the data structure.
> > > 
> > > Currently we're using:
> > > 
> > > HeapHashMap<pair<WeakMember<SVGElement>, QualifiedName>,
> > > Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>>
> > > 
> > > Can we change this to something like:
> > > 
> > > class ElementAttributePair {
> > >   WeakMember<SVGElement> m_element;
> > >   QualifiedName m_name;
> > > };
> > > 
> > > HeapHashMap<Member<ElementAttributePair>,
> > > Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>>
> > > 
> > > ?
> > 
> > I think we will end up in the same place if we do that. In order to use
> > ElementAttributePair as the key in a HashMap you have to provide a hash
trait
> > for it...
> 
> Ian points out that I misunderstood. ;-)
> 
> What you are suggesting is using a Member pointer as the key which hashes just
> fine.
> 
> I think there are two separate issues that we need to address here:
> 
> 1) marking crashes because of threading issues. That is what this change is
> fixing. I wonder if we can fix that by changing the QualifiedName hash trait
to
> not use the null qualified name as the empty value?
> 
> 2) this is a weak hash map but that does not work for pairs. That is, we do
not
> actually get the values pulled out of a hashmap when the weak part of a
> pair<WeakMember<T>, U> dies. We currently have a workaround for that which is
> that we collect invalid keys when we iterate the map and pull them out
manually
> at that point. However, we should make the automatic clearing work for this.
> 
> Haraken, your solution will fix 1) but it will make the hash set strong and
make
> it impossible to make weak processing clear this map automatically. Therefore,
> it might be a step in the wrong direction for dealing with 2).

Regarding 2), can we manually clean up empty entries from the hash map before
starting iterating the hash map?

We're already doing the manual clean-up for the value of the hash map (i.e.,
Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>). We have code to remove
the HeapLinkedHashSet if the HeapLinkedHashSet becomes empty:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Mads Ager (chromium), 2014-06-04 07:13:43]

On 2014/06/04 07:10:35, haraken wrote:
> On 2014/06/04 07:01:41, Mads Ager (chromium) wrote:
> > On 2014/06/04 06:37:52, Mads Ager (chromium) wrote:
> > > On 2014/06/03 14:55:33, haraken wrote:
> > > > Another idea might be to simplify the data structure.
> > > > 
> > > > Currently we're using:
> > > > 
> > > > HeapHashMap<pair<WeakMember<SVGElement>, QualifiedName>,
> > > > Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>>
> > > > 
> > > > Can we change this to something like:
> > > > 
> > > > class ElementAttributePair {
> > > >   WeakMember<SVGElement> m_element;
> > > >   QualifiedName m_name;
> > > > };
> > > > 
> > > > HeapHashMap<Member<ElementAttributePair>,
> > > > Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>>
> > > > 
> > > > ?
> > > 
> > > I think we will end up in the same place if we do that. In order to use
> > > ElementAttributePair as the key in a HashMap you have to provide a hash
> trait
> > > for it...
> > 
> > Ian points out that I misunderstood. ;-)
> > 
> > What you are suggesting is using a Member pointer as the key which hashes
just
> > fine.
> > 
> > I think there are two separate issues that we need to address here:
> > 
> > 1) marking crashes because of threading issues. That is what this change is
> > fixing. I wonder if we can fix that by changing the QualifiedName hash trait
> to
> > not use the null qualified name as the empty value?
> > 
> > 2) this is a weak hash map but that does not work for pairs. That is, we do
> not
> > actually get the values pulled out of a hashmap when the weak part of a
> > pair<WeakMember<T>, U> dies. We currently have a workaround for that which
is
> > that we collect invalid keys when we iterate the map and pull them out
> manually
> > at that point. However, we should make the automatic clearing work for this.
> > 
> > Haraken, your solution will fix 1) but it will make the hash set strong and
> make
> > it impossible to make weak processing clear this map automatically.
Therefore,
> > it might be a step in the wrong direction for dealing with 2).
> 
> Regarding 2), can we manually clean up empty entries from the hash map before
> starting iterating the hash map?
> 
> We're already doing the manual clean-up for the value of the hash map (i.e.,
> Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>). We have code to remove
> the HeapLinkedHashSet if the HeapLinkedHashSet becomes empty:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Yes, we are currently doing that manual cleanup. What I meant by "We currently
have a workaround for that which is that we collect invalid keys when we iterate
the map and pull them out manually at that point" was this code:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Whenever we update animations we collect the invalid keys as part of the
iteration and pull them out at the end.

[haraken, 2014-06-04 07:20:59]

On 2014/06/04 07:13:43, Mads Ager (chromium) wrote:
> On 2014/06/04 07:10:35, haraken wrote:
> > On 2014/06/04 07:01:41, Mads Ager (chromium) wrote:
> > > On 2014/06/04 06:37:52, Mads Ager (chromium) wrote:
> > > > On 2014/06/03 14:55:33, haraken wrote:
> > > > > Another idea might be to simplify the data structure.
> > > > > 
> > > > > Currently we're using:
> > > > > 
> > > > > HeapHashMap<pair<WeakMember<SVGElement>, QualifiedName>,
> > > > > Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>>
> > > > > 
> > > > > Can we change this to something like:
> > > > > 
> > > > > class ElementAttributePair {
> > > > >   WeakMember<SVGElement> m_element;
> > > > >   QualifiedName m_name;
> > > > > };
> > > > > 
> > > > > HeapHashMap<Member<ElementAttributePair>,
> > > > > Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>>
> > > > > 
> > > > > ?
> > > > 
> > > > I think we will end up in the same place if we do that. In order to use
> > > > ElementAttributePair as the key in a HashMap you have to provide a hash
> > trait
> > > > for it...
> > > 
> > > Ian points out that I misunderstood. ;-)
> > > 
> > > What you are suggesting is using a Member pointer as the key which hashes
> just
> > > fine.
> > > 
> > > I think there are two separate issues that we need to address here:
> > > 
> > > 1) marking crashes because of threading issues. That is what this change
is
> > > fixing. I wonder if we can fix that by changing the QualifiedName hash
trait
> > to
> > > not use the null qualified name as the empty value?
> > > 
> > > 2) this is a weak hash map but that does not work for pairs. That is, we
do
> > not
> > > actually get the values pulled out of a hashmap when the weak part of a
> > > pair<WeakMember<T>, U> dies. We currently have a workaround for that which
> is
> > > that we collect invalid keys when we iterate the map and pull them out
> > manually
> > > at that point. However, we should make the automatic clearing work for
this.
> > > 
> > > Haraken, your solution will fix 1) but it will make the hash set strong
and
> > make
> > > it impossible to make weak processing clear this map automatically.
> Therefore,
> > > it might be a step in the wrong direction for dealing with 2).
> > 
> > Regarding 2), can we manually clean up empty entries from the hash map
before
> > starting iterating the hash map?
> > 
> > We're already doing the manual clean-up for the value of the hash map (i.e.,
> > Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>). We have code to
remove
> > the HeapLinkedHashSet if the HeapLinkedHashSet becomes empty:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Yes, we are currently doing that manual cleanup. What I meant by "We currently
> have a workaround for that which is that we collect invalid keys when we
iterate
> the map and pull them out manually at that point" was this code:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Whenever we update animations we collect the invalid keys as part of the
> iteration and pull them out at the end.

ah, understood.

[haraken, 2014-06-04 07:21:49]

As far as I understand, this hash map is a rare, crazy case, so probably it
might be OK to live with a manual clean-up in this particular case.

[Mads Ager (chromium), 2014-06-04 07:32:53]

On 2014/06/04 07:21:49, haraken wrote:
> As far as I understand, this hash map is a rare, crazy case, so probably it
> might be OK to live with a manual clean-up in this particular case.

Yes, it is probably OK in this case, I agree. However, we need to have a general
fix for marking of hashmaps of QualifiedNames anyway. Here is map from
QualifiedName to something else. That will have a similar crash during marking
because of threading issues:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The other option here is to find a way of disabling that ref counting assert
during marking. As far as I can tell the ref/deref is safe during marking in
this case. Since all threads are stopped we cannot get race conditions and
churning on the null qualified name ref count should be safe and the ref count
will end up in the same place as it was before marking. Not sure how to do the
disabling in a nice way though, so I think we should spent some time considering
if we can make the QualifiedName empty value avoid this issue. We can use
disabling of the assert during marking as a last resort.

[blink-reviews, 2014-06-04 07:41:32]

FYI. I am looking into changing the QualifiedName's hash trait to work
without the nullQName which is what is being copied and hence ref'ed during
marking.


On Wed, Jun 4, 2014 at 9:32 AM, <ager@chromium.org> wrote:

> On 2014/06/04 07:21:49, haraken wrote:
>
>> As far as I understand, this hash map is a rare, crazy case, so probably
>> it
>> might be OK to live with a manual clean-up in this particular case.
>>
>
> Yes, it is probably OK in this case, I agree. However, we need to have a
> general
> fix for marking of hashmaps of QualifiedNames anyway. Here is map from
> QualifiedName to something else. That will have a similar crash during
> marking
> because of threading issues:
>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/third_party/WebKit/Source/core/dom/NodeRareData.h&l=105
>
> The other option here is to find a way of disabling that ref counting
> assert
> during marking. As far as I can tell the ref/deref is safe during marking
> in
> this case. Since all threads are stopped we cannot get race conditions and
> churning on the null qualified name ref count should be safe and the ref
> count
> will end up in the same place as it was before marking. Not sure how to do
> the
> disabling in a nice way though, so I think we should spent some time
> considering
> if we can make the QualifiedName empty value avoid this issue. We can use
> disabling of the assert during marking as a last resort.
>
> https://codereview.chromium.org/311803003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[wibling-chromium, 2014-06-04 07:49:33]

"I" in the above reply happens to be me:)

[haraken, 2014-06-04 07:52:59]

> > Yes, it is probably OK in this case, I agree. However, we need to have a
> > general
> > fix for marking of hashmaps of QualifiedNames anyway. Here is map from
> > QualifiedName to something else. That will have a similar crash during
> > marking
> > because of threading issues:
> >
> > https://code.google.com/p/chromium/codesearch#chromium/
> > src/third_party/WebKit/Source/core/dom/NodeRareData.h&l=105

I'm a bit confused. I was thinking that this is a problem that happens only if
we have "a pair that contains an object that calls ref()" in a hash map.

If HeapHashMap<QualifiedName, Member<X>> is problematic, isn't
HeapHashMap<AtomicString, Member<X>> problematic as well? AtomicString can call
ref() for the underlying StringImpl objects. (However, AtomicString might not be
a problem since it doesn't call ref() when an empty AtomicString is created.)

[blink-reviews, 2014-06-04 08:04:41]

The difference between AtomicString/String and QualifiedName is that
AtomicString/String can have a m_impl which is null. There is a
HashTraits<String>::isEmptyValue() method which uses !!m_impl as the empty
value check. QualifiedName doesn't currently allow it's m_impl to be null
and instead allocates a nullQName instance representing the empty value.
This emptyValue is copied around and ref'ed/deref'ed during marking which
is why QualifiedName causes an issue.


On Wed, Jun 4, 2014 at 9:52 AM, <haraken@chromium.org> wrote:

> > Yes, it is probably OK in this case, I agree. However, we need to have a
>> > general
>> > fix for marking of hashmaps of QualifiedNames anyway. Here is map from
>> > QualifiedName to something else. That will have a similar crash during
>> > marking
>> > because of threading issues:
>> >
>> > https://code.google.com/p/chromium/codesearch#chromium/
>> > src/third_party/WebKit/Source/core/dom/NodeRareData.h&l=105
>>
>
> I'm a bit confused. I was thinking that this is a problem that happens
> only if
> we have "a pair that contains an object that calls ref()" in a hash map.
>
> If HeapHashMap<QualifiedName, Member<X>> is problematic, isn't
> HeapHashMap<AtomicString, Member<X>> problematic as well? AtomicString can
> call
> ref() for the underlying StringImpl objects. (However, AtomicString might
> not be
> a problem since it doesn't call ref() when an empty AtomicString is
> created.)
>
>
> https://codereview.chromium.org/311803003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[wibling-chromium, 2014-06-04 10:36:40]

PTAL.

[zerny-chromium, 2014-06-04 10:51:06]

It is really a shame that this "empty" value is forced on collection elements.
lgtm.

[Mads Ager (chromium), 2014-06-04 10:58:10]

LGTM, but we let's wait and see if eseidel@ has comments based on experience
with this code. 

It is nice that this removes the constant ref/deref of a null qname object on
iteration of collections of QualifiedNames.

[haraken, 2014-06-05 00:53:10]

LGTM, but experts should confirm. Given comment #13, I like the idea of
addressing this issue in the QualifiedName layer.

[esprehn, 2014-06-05 00:58:43]

lgtm, but please fix the style before landing. I'm also somewhat concerned that
adding all those branches will be bad, but lets see what happens.

https://codereview.chromium.org/311803003/diff/20001/Source/core/dom/Qualifie...
File Source/core/dom/QualifiedName.h (right):

https://codereview.chromium.org/311803003/diff/20001/Source/core/dom/Qualifie...
Source/core/dom/QualifiedName.h:86: bool hasPrefix() const { return m_impl ?
(m_impl->m_prefix != nullAtom) : false; }
Don't use a ternary, use &&.

return m_impl && m_impl->m_prefix != nullAtom;

https://codereview.chromium.org/311803003/diff/20001/Source/core/dom/Qualifie...
Source/core/dom/QualifiedName.h:91: const AtomicString& namespaceURI() const {
return m_impl ? m_impl->m_namespace : nullAtom; }
This code can all be really hot, we call into it all over selector checking and
other things. Are we sure we want branches inside it?

https://codereview.chromium.org/311803003/diff/20001/Source/core/dom/Qualifie...
Source/core/dom/QualifiedName.h:98: QualifiedNameImpl* impl() const { return
m_impl ? m_impl.get() : 0; }
The ternary is not needed here.

[esprehn, 2014-06-05 01:01:08]

Actually, what if we the same thing for the nullQName that we do for
AtomicStrings where we totally ignore the ref counting? That fixes the ASSERT.
See the isStatic() bit and the destroyIfNotStatic() logic.

[wibling-chromium, 2014-06-10 10:59:26]

On 2014/06/05 01:01:08, esprehn wrote:
> Actually, what if we the same thing for the nullQName that we do for
> AtomicStrings where we totally ignore the ref counting? That fixes the ASSERT.
> See the isStatic() bit and the destroyIfNotStatic() logic.

Thanks for the review! I will see if I can get rid of the ref counting instead.

[wibling-chromium, 2014-06-12 10:22:42]

PTAL

[haraken, 2014-06-12 10:57:15]

LGTM. Elliott might want to take another look.

https://codereview.chromium.org/311803003/diff/40001/Source/core/dom/Qualifie...
File Source/core/dom/QualifiedName.h (right):

https://codereview.chromium.org/311803003/diff/40001/Source/core/dom/Qualifie...
Source/core/dom/QualifiedName.h:37: // This struct is used pass data between
QualifiedName and the QNameTranslator.

is used to pass

[wibling-chromium, 2014-06-12 11:02:05]

Thanks for the review! I will wait for Elliott to take a look as well.

https://codereview.chromium.org/311803003/diff/40001/Source/core/dom/Qualifie...
File Source/core/dom/QualifiedName.h (right):

https://codereview.chromium.org/311803003/diff/40001/Source/core/dom/Qualifie...
Source/core/dom/QualifiedName.h:37: // This struct is used pass data between
QualifiedName and the QNameTranslator.
On 2014/06/12 10:57:15, haraken wrote:
> 
> is used to pass

Done.

[wibling-chromium, 2014-06-17 10:51:41]

Elliott, do you have time to take a second look at this?

[esprehn, 2014-06-17 11:09:24]

lgtm, awesome!

[wibling-chromium, 2014-06-17 11:10:27]

On 2014/06/17 11:09:24, esprehn wrote:
> lgtm, awesome!

Thanks!

[wibling-chromium, 2014-06-17 11:10:32]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-06-17 11:10:50]

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

[commit-bot: I haz the power, 2014-06-17 11:44:21]

Change committed as 176307