Bug 94187

Summary: Share the StringImpl the CSS property names
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: CSSAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cmarcelo, kling, macpherson, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Benjamin Poulain
Reported 2012-08-15 22:19:21 PDT
We allocate one new StringImpl every time we need a CSS property name.
Attachments
Patch (10.93 KB, patch)
2012-08-15 22:41 PDT, Benjamin Poulain 		no flags
DetailsFormatted DiffDiff
Patch (11.07 KB, patch)
2012-08-16 17:03 PDT, Benjamin Poulain 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Benjamin Poulain
Comment 1 2012-08-15 22:41:15 PDT
Created attachment 158713 [details] Patch
Alexey Proskuryakov
Comment 2 2012-08-16 14:11:02 PDT
Comment on attachment 158713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158713&action=review > Source/WebCore/css/makeprop.pl:141 > + static AtomicString* propertyStrings = new AtomicString[numCSSProperties]; // Leaked intentionally. It's not "leaked", it's just never destroyed. > Source/WebCore/css/makeprop.pl:153 > + return String(getPropertyNameAtomicString(id).impl()); Why not AtomicString::string()?
Benjamin Poulain
Comment 3 2012-08-16 17:03:08 PDT
Created attachment 158950 [details] Patch
Benjamin Poulain
Comment 4 2012-08-16 17:03:58 PDT
Addressed Alexey's comments + added perf numbers. My microbenchmark: http://jsperf.com/getpropertyname-memory-wasting
Alexey Proskuryakov
Comment 5 2012-08-16 17:08:05 PDT
Comment on attachment 158950 [details] Patch ok
Benjamin Poulain
Comment 6 2012-08-17 14:24:21 PDT
Comment on attachment 158950 [details] Patch Clearing flags on attachment: 158950 Committed r125934: <http://trac.webkit.org/changeset/125934>
Benjamin Poulain
Comment 7 2012-08-17 14:24:25 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.