WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Mike Belshe
Comment 1
2009-03-24 18:50:32 PDT
Created
attachment 28921
[details]
patch
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.
Top of Page
Format For Printing
XML
Clone This Bug