Bug 24785 – Move CSS AtomicStrings into static AtomicStrings

• Component: WebCore Misc.
• Status: NEW

[Mike Belshe]

CSSFontSelector.cpp has some strings which are constantly churning through the AtomicString table.

[Mike Belshe]

Created attachment 28921 [details]
patch

[Darin Adler]

Comment on attachment 28921 [details]
patch

> + * This file is part of the DOM implementation for KDE.

You shouldn't put these in these new files.

> + * Copyright (C) 2009 Apple Computer, Inc.

If you're granting copyright to Apple, the correct format is Copyright (C) 2009 Apple Inc. All rights reserved.

> +#include "config.h"

> +    #define CSS_NAMES_DECLARE(name, str) extern const AtomicString name##Atom;

I think it's a little strange to use the suffix "Atom". We don't do that for event names or HTML tag names.

Does this have a measurable performance effect?

It doesn't make sense to make an AtomicString for the prefix "-webkit-", because atomic strings optimize equality comparisons. They don't optimize prefix checks.

[Mike Belshe]

(In reply to comment #2)
> (From update of attachment 28921 [details] [review])
> > + * This file is part of the DOM implementation for KDE.
> 
> You shouldn't put these in these new files.
> 
> > + * Copyright (C) 2009 Apple Computer, Inc.
> 
> If you're granting copyright to Apple, the correct format is Copyright (C) 2009
> Apple Inc. All rights reserved.

I don't care much about the copyright; I'm just trying to abide by existing style. I'll read the webkit docs and see what looks better.


> 
> > +#include "config.h"
> 
> > +    #define CSS_NAMES_DECLARE(name, str) extern const AtomicString name##Atom;
> 
> I think it's a little strange to use the suffix "Atom". We don't do that for
> event names or HTML tag names.

True; I was just trying to find a good naming scheme.  Do you have an alternate proposal?  I liked calling them Atoms because it is clear what we're doing by having them.

> Does this have a measurable performance effect?

I used a page cycler which is heavy on international pages (churns through lots of fonts) and it did have a benefit; but it is relatively small 0.5-1%.  I do think this change is good, though.


> It doesn't make sense to make an AtomicString for the prefix "-webkit-",
> because atomic strings optimize equality comparisons. They don't optimize
> prefix checks.

Sort of.  In this case, we have an atomic string and we're doing:
   atomstr.startsWith("-webkit-")

The AtomicString::startsWith() method takes an AtomicString as an argument.  So the "-webkit-" gets casted all the way into an AtomicString, only to be torn out as it drops out of scope.  Perhaps a better solution to this problem would be to add a method to AtomicString for startWith which takes a String.  Then we could either create a static String for this or just let it create the String on each invocation.

[Eric Seidel (no email)]

Comment on attachment 28921 [details]
patch

If you went the CSSNames route, the make_names.pl script is what you'd want to use.  It would do all this for you.  Then you'd just add a CSsNames.in file somewhere.

But I think you probably want to use this construct instead:
    DEFINE_STATIC_LOCAL(AtomicString, anyLink, ("-webkit-any-link"));

It's simpler.  It's our hack around the static local problem as far as I understand it.  We used to use static locals until recently that macro was added.

[Mike Belshe]

(In reply to comment #4)
> (From update of attachment 28921 [details] [review])
> If you went the CSSNames route, the make_names.pl script is what you'd want to
> use.  It would do all this for you.  Then you'd just add a CSsNames.in file
> somewhere.

Ok - I will check that out.  I wasn't aware of that magic!

> 
> But I think you probably want to use this construct instead:
>     DEFINE_STATIC_LOCAL(AtomicString, anyLink, ("-webkit-any-link"));

Originally I used this, but I discovered that both CSSFontSelector.cpp and CSSStyleSelector.cpp used these strings.  The second file was less important, but it seemed like a good idea to cover both...


> 
> It's simpler.  It's our hack around the static local problem as far as I
> understand it.  We used to use static locals until recently that macro was
> added.
> 

[Alexey Proskuryakov]

> Perhaps a better solution to this problem would
> be to add a method to AtomicString for startWith which takes a String

Yes, this definitely looks like a better solution to me (I even think that the version taking AtomicString could be removed). Of course, find(), contains() and other AtomicString methods need this treatment, too.

0.5-1% is a significant improvement by our standards.

[Mike Belshe]

Great - I agree on the AtomicString methods.  Breaking that portion out into bug 24979.