Bug 95282 – Deploy ASCIILiteral hotness throughout WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Deploy ASCIILiteral hotness throughout WebCore

[Adam Barth]

Created attachment 161132 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 161132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161132&action=review

> Source/WebCore/css/CSSPropertySourceData.cpp:91
> +    DEFINE_STATIC_LOCAL(String, emptyValue, (ASCIILiteral("e")));

Maw?

> Source/WebCore/dom/MicroDataItemList.cpp:45
>      DEFINE_STATIC_LOCAL(String, undefinedItemTypeString, (""));
> +    // FIXME: Why not just return emptyString(); ?
>      return undefinedItemTypeString;

Someone hit me with a spoon.

> Source/WebCore/editing/MarkupAccumulator.cpp:238
> -    DEFINE_STATIC_LOCAL(String, xmlnsWithColon, ("xmlns:"));
> +    DEFINE_STATIC_LOCAL(String, xmlnsWithColon, (ASCIILiteral("xmlns:")));

This seems odd.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:638
> +    DEFINE_STATIC_LOCAL(String, defaultPrefix, (ASCIILiteral("    ")));

!?!

> Source/WebCore/loader/MainResourceLoader.cpp:374
> +            DEFINE_STATIC_LOCAL(String, consoleMessage, (ASCIILiteral("Refused to display document because display forbidden by X-Frame-Options.\n")));

Why the \n?

> Source/WebCore/page/Page.cpp:395
>      DEFINE_STATIC_LOCAL(String, nullString, ());
> +    // FIXME: Why not just return String()?

Oh please.

> Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp:126
> -    DEFINE_STATIC_LOCAL(String, fontFace, ("Arial"));
> +    DEFINE_STATIC_LOCAL(String, fontFace, (ASCIILiteral("Arial")));

WTF?

> Source/WebCore/xml/parser/XMLTokenizer.cpp:485
> -        DEFINE_STATIC_LOCAL(String, xmlString, ("xml"));
> +        DEFINE_STATIC_LOCAL(String, xmlString, (ASCIILiteral("xml")));

XMLNames bro.

> Source/WebCore/xml/parser/XMLTreeBuilder.cpp:341
> +    DEFINE_STATIC_LOCAL(String, ampS, (ASCIILiteral("&")));
> +    DEFINE_STATIC_LOCAL(String, aposS, (ASCIILiteral("'")));
> +    DEFINE_STATIC_LOCAL(String, gtS, (ASCIILiteral(">")));
> +    DEFINE_STATIC_LOCAL(String, ltS, (ASCIILiteral("<")));
> +    DEFINE_STATIC_LOCAL(String, quotS, (ASCIILiteral("\"")));

We have the Entity technology to help you. :)

[Adam Barth]

Comment on attachment 161132 [details]
Patch

I'll fix some of these in a followup patch.  I'm not that inclined to improve the NEWXML at the moment.

[Benjamin Poulain]

Adam, thank you for deploying this widely, it is gonna help me a lot to track our memory.

All those static strings make me wonder if that is causing some of the memory behaviors we have. Once the StringImpl is created in a page, we can no longer give that page back to the system. :(

Maybe we should have some dense memory area just for things like these. It's something we should measure...

> > Source/WebCore/dom/MicroDataItemList.cpp:45
> >      DEFINE_STATIC_LOCAL(String, undefinedItemTypeString, (""));
> > +    // FIXME: Why not just return emptyString(); ?
> >      return undefinedItemTypeString;
> 
> Someone hit me with a spoon.

Brilliant :-D

[Adam Barth]

> All those static strings make me wonder if that is causing some of the memory behaviors we have. Once the StringImpl is created in a page, we can no longer give that page back to the system. :(
> 
> Maybe we should have some dense memory area just for things like these. It's something we should measure...

Yeah, that makes a lot of sense.

[Adam Barth]

Comment on attachment 161132 [details]
Patch

Clearing flags on attachment: 161132

Committed r126968: <http://trac.webkit.org/changeset/126968>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.