NEW 24785
Move CSS AtomicStrings into static AtomicStrings
https://bugs.webkit.org/show_bug.cgi?id=24785
Summary Move CSS AtomicStrings into static AtomicStrings
Mike Belshe
Reported 2009-03-24 14:31:14 PDT
CSSFontSelector.cpp has some strings which are constantly churning through the AtomicString table.
Attachments
patch (16.11 KB, patch)
2009-03-24 18:50 PDT, Mike Belshe
eric: review-
Mike Belshe
Comment 1 2009-03-24 18:50:32 PDT
Darin Adler
Comment 2 2009-03-25 06:34:17 PDT
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
Comment 3 2009-03-25 10:12:01 PDT
(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 4 2009-03-25 18:11:53 PDT
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
Comment 5 2009-03-26 08:41:42 PDT
(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
Comment 6 2009-03-26 12:57:33 PDT
> 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
Comment 7 2009-03-31 20:05:39 PDT
Great - I agree on the AtomicString methods. Breaking that portion out into bug 24979.
Note You need to log in before you can comment on or make changes to this bug.