Bug 103231 – Use AtomicString instead of AtomicStringImpl* in WebCore::RuleSet.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: CSS

[Hayato Ito]

It seems we can use AtomicString instead of AtomicStringImpl* in WebCore::RuleSet without any performance regressions.

[Hayato Ito]

Created attachment 175948 [details]
Use AtomicString

[Hayato Ito]

Created attachment 175956 [details]
Add isNull() check.

[Ojan Vafai]

Comment on attachment 175956 [details]
Add isNull() check.

This seems reasonable to me. Do you know why AtomicStringImpl was used in the first place? What patch was this code added in?

[Hayato Ito]

(In reply to comment #3)
> (From update of attachment 175956 [details])
> This seems reasonable to me. Do you know why AtomicStringImpl was used in the first place? What patch was this code added in?

It's hard to find the first place. They were used at 94656 at least around CSS code base.
http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSStyleSelector.h?annotate=blame&rev=94656

[Hayato Ito]

It turned out that this replacement is not specific to WebCore::RuleSet. I think we can replace most of usage of AtomicStringImpl* with AtomicString in WebCore.

Let me use this patch as a pilot case. If things go well, I'll replace AtomicStringImpl* with AtomicString in other places in WebCore.

(In reply to comment #3)
> (From update of attachment 175956 [details])
> This seems reasonable to me. Do you know why AtomicStringImpl was used in the first place? What patch was this code added in?

[Ojan Vafai]

Now that I think about this more, the AtomicStringImpl* is the thing that's guaranteed to be unique for a given string, not the AtomicString*. So, if we're putting it in a map, we probably want to be putting in the impl, no?

[Antti Koivisto]

Comment on attachment 175956 [details]
Add isNull() check.

View in context: https://bugs.webkit.org/attachment.cgi?id=175956&action=review

> Source/WebCore/ChangeLog:8
> +        Using AtomicString is preferred to using *AtomicStringImpl directly.  We

Rules like that exist to help people avoid mistakes with things they don't understand. This code is safe, the change just increases ref churn.

[Ojan Vafai]

(In reply to comment #7)
> (From update of attachment 175956 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175956&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Using AtomicString is preferred to using *AtomicStringImpl directly.  We
> 
> Rules like that exist to help people avoid mistakes with things they don't understand. This code is safe, the change just increases ref churn.

In fairness, code *should* generally use AtomicString. Anywhere we directly use AtomicStringImpl deserves a comment explaining why IMO.

[Antti Koivisto]

Fair enough. However should not run around the code base changing things that break some supposed rule unless they can show the change fixes a concrete bug. This has caused problems (and bugs!) in the past and has been extensively discussed on webkit-dev.

[Hayato Ito]

Okay. I'm okay to mark this WONTFIX.
It might not be worth doing this kind of refactoring.

(In reply to comment #9)
> Fair enough. However should not run around the code base changing things that break some supposed rule unless they can show the change fixes a concrete bug. This has caused problems (and bugs!) in the past and has been extensively discussed on webkit-dev.

Can I assume that we should use HashSet<AtomicString> instead of HashSet<AtomicStringImpl*> for *new* code? I could not find the discussion on webkit-dev.

[Benjamin Poulain]

(In reply to comment #10)
> Can I assume that we should use HashSet<AtomicString> instead of HashSet<AtomicStringImpl*> for *new* code?

No, not necessarily. HashSet<AtomicStringImpl*> is a better solution in some cases. It is not a case of an old code style, AtomicStringImpl* is a valid tool in our engineering toolbox.

[Hayato Ito]

Thank you.

I've just come up with the merit of using AtomicStringImpl* as a key in this case.
Since AtomicString defines implicit conversion constructors from String unfortunately, callers might pass String unintentionally without any warnings.
The pointer has a merit since String can not be a valid key if we use AtomicStringImpl* as a key.

(In reply to comment #11)
> (In reply to comment #10)
> > Can I assume that we should use HashSet<AtomicString> instead of HashSet<AtomicStringImpl*> for *new* code?
> 
> No, not necessarily. HashSet<AtomicStringImpl*> is a better solution in some cases. It is not a case of an old code style, AtomicStringImpl* is a valid tool in our engineering toolbox.

[Hayato Ito]

For the record, I've wrongly assumed that AtomicString has only a pointer to AtomicStringImpl as a member variable. So I thought there won't be any performance regression. That's wrong. AtomicString has a String as a member variable in fact.

(In reply to comment #12)
> Thank you.
> 
> I've just come up with the merit of using AtomicStringImpl* as a key in this case.
> Since AtomicString defines implicit conversion constructors from String unfortunately, callers might pass String unintentionally without any warnings.
> The pointer has a merit since String can not be a valid key if we use AtomicStringImpl* as a key.
> 
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Can I assume that we should use HashSet<AtomicString> instead of HashSet<AtomicStringImpl*> for *new* code?
> > 
> > No, not necessarily. HashSet<AtomicStringImpl*> is a better solution in some cases. It is not a case of an old code style, AtomicStringImpl* is a valid tool in our engineering toolbox.

[Hayato Ito]

I recalled that my assumption came from the comment here:
http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSSelector.h#L198

        // AtomicString is really just an AtomicStringImpl* so the cast below is safe.
        // FIXME: Perhaps call sites could be changed to accept AtomicStringImpl?
        const AtomicString& value() const { return *reinterpret_cast<const AtomicString*>(m_hasRareData ? &m_data.m_rareData->m_value : &m_data.m_value); }


(In reply to comment #13)
> For the record, I've wrongly assumed that AtomicString has only a pointer to AtomicStringImpl as a member variable. So I thought there won't be any performance regression. That's wrong. AtomicString has a String as a member variable in fact.
> 
> (In reply to comment #12)
> > Thank you.
> > 
> > I've just come up with the merit of using AtomicStringImpl* as a key in this case.
> > Since AtomicString defines implicit conversion constructors from String unfortunately, callers might pass String unintentionally without any warnings.
> > The pointer has a merit since String can not be a valid key if we use AtomicStringImpl* as a key.
> > 
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > Can I assume that we should use HashSet<AtomicString> instead of HashSet<AtomicStringImpl*> for *new* code?
> > > 
> > > No, not necessarily. HashSet<AtomicStringImpl*> is a better solution in some cases. It is not a case of an old code style, AtomicStringImpl* is a valid tool in our engineering toolbox.