Bug 24785 - Move CSS AtomicStrings into static AtomicStrings
Summary: Move CSS AtomicStrings into static AtomicStrings
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Mike Belshe
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-24 14:31 PDT by Mike Belshe
Modified: 2010-06-10 16:56 PDT (History)
1 user (show)

See Also:


Attachments
patch (16.11 KB, patch)
2009-03-24 18:50 PDT, Mike Belshe
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Belshe 2009-03-24 14:31:14 PDT
CSSFontSelector.cpp has some strings which are constantly churning through the AtomicString table.
Comment 1 Mike Belshe 2009-03-24 18:50:32 PDT
Created attachment 28921 [details]
patch
Comment 2 Darin Adler 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.
Comment 3 Mike Belshe 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.

Comment 4 Eric Seidel (no email) 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.
Comment 5 Mike Belshe 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.
> 

Comment 6 Alexey Proskuryakov 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.
Comment 7 Mike Belshe 2009-03-31 20:05:39 PDT
Great - I agree on the AtomicString methods.  Breaking that portion out into bug 24979.