It seems we can use AtomicString instead of AtomicStringImpl* in WebCore::RuleSet without any performance regressions.
Created attachment 175948 [details] Use AtomicString
Created attachment 175956 [details] Add isNull() check.
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?
(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
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?
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?
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.
(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.
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.
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.
(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.
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.
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.
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.