Summary: | Deploy ASCIILiteral hotness throughout WebCore | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||
Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | benjamin, cmarcelo, eric.carlson, eric, feature-media-reviews, gyuyoung.kim, japhet, macpherson, menard, mifenton, rakuco, tkent, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Adam Barth
2012-08-28 21:24:34 PDT
Created attachment 161132 [details]
Patch
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. :) 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.
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
> 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.
Comment on attachment 161132 [details] Patch Clearing flags on attachment: 161132 Committed r126968: <http://trac.webkit.org/changeset/126968> All reviewed patches have been landed. Closing bug. |