CSSFontSelector.cpp has some strings which are constantly churning through the AtomicString table.
Created attachment 28921 [details] patch
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.
(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.
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.
(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. >
> 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.
Great - I agree on the AtomicString methods. Breaking that portion out into bug 24979.