Bug 100753 - Pack immutable StylePropertySets harder on 64-bit.
Summary: Pack immutable StylePropertySets harder on 64-bit.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-10-30 06:37 PDT by Andreas Kling
Modified: 2012-11-01 07:10 PDT (History)
9 users (show)

See Also:


Attachments
For ews (27.75 KB, patch)
2012-10-30 06:43 PDT, Andreas Kling
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
For ews 2: The CSS_VARIABLES strike back (27.69 KB, patch)
2012-10-30 07:24 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Oh yes, it is a patch. (17.50 KB, patch)
2012-10-31 07:34 PDT, Andreas Kling
koivisto: review+
Details | Formatted Diff | Diff
valgrind log (16.26 KB, text/plain)
2012-11-01 06:13 PDT, Sudarsana Nagineni (babu)
no flags Details
Ninja fix for the debug bots (3.45 KB, patch)
2012-11-01 06:51 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2012-10-30 06:37:11 PDT
We currently waste 4 bytes per property on 64-bit due to the padding between CSSProperty's metadata and CSSValue pointer. Let's not do that.
Comment 1 Radar WebKit Bug Importer 2012-10-30 06:37:58 PDT
<rdar://problem/12599155>
Comment 2 Andreas Kling 2012-10-30 06:43:14 PDT
Created attachment 171435 [details]
For ews
Comment 3 WebKit Review Bot 2012-10-30 06:45:01 PDT
Attachment 171435 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/css/StylePropertySet.cpp:927:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 WebKit Review Bot 2012-10-30 07:08:01 PDT
Comment on attachment 171435 [details]
For ews

Attachment 171435 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14631518
Comment 5 EFL EWS Bot 2012-10-30 07:08:31 PDT
Comment on attachment 171435 [details]
For ews

Attachment 171435 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14639532
Comment 6 Peter Beverloo (cr-android ews) 2012-10-30 07:16:19 PDT
Comment on attachment 171435 [details]
For ews

Attachment 171435 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14643128
Comment 7 Andreas Kling 2012-10-30 07:24:42 PDT
Created attachment 171447 [details]
For ews 2: The CSS_VARIABLES strike back
Comment 8 Andreas Kling 2012-10-31 07:34:35 PDT
Created attachment 171656 [details]
Oh yes, it is a patch.
Comment 9 Antti Koivisto 2012-10-31 10:17:52 PDT
Comment on attachment 171656 [details]
Oh yes, it is a patch.

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

r=me, good idea!

> Source/WebCore/css/CSSProperty.h:58
> -    {
> -    }
> +    { }

I think the original was the correct style.

> Source/WebCore/css/StylePropertySet.cpp:1195
> +String StylePropertySet::PropertyReference::cssName() const
> +{
> +#if ENABLE(CSS_VARIABLES)
> +    if (id() == CSSPropertyVariable) {
> +        ASSERT(propertyValue()->isVariableValue());
> +        return "-webkit-var-" + static_cast<const CSSVariableValue*>(propertyValue())->name();
> +    }
> +#endif
> +    return getPropertyNameString(id());
> +}
> +
> +String StylePropertySet::PropertyReference::cssText() const
> +{
> +    StringBuilder result;
> +    result.append(cssName());
> +    result.appendLiteral(": ");
> +    result.append(propertyValue()->cssText());
> +    if (isImportant())
> +        result.appendLiteral(" !important");
> +    result.append(';');
> +    return result.toString();
> +}

I think it would be slightly better if PropertyReference would focus on being a reference and would not have complex functions with CSS semantics hanging from it. Maybe these could just be free standing?

> Source/WebCore/css/StylePropertySet.h:72
> +        String cssName() const;
> +        String cssText() const;

It might be nicer if S
Comment 10 Antti Koivisto 2012-10-31 10:20:33 PDT
the cut off comment was to be the same as the previous one
Comment 11 Andreas Kling 2012-11-01 02:13:08 PDT
(In reply to comment #9)
> r=me, good idea!

Committed r133138: <http://trac.webkit.org/changeset/133138>

> I think it would be slightly better if PropertyReference would focus on being a reference and would not have complex functions with CSS semantics hanging from it. Maybe these could just be free standing?

Will take a separate look at this.
Comment 12 Mikhail Pozdnyakov 2012-11-01 05:27:31 PDT
Leads to memory corruption seen on EFL WK1 and EFL WK2 bots, 
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/5330/steps/API%20tests/logs/stdio

reproduced locally, here is gdb back trace

#17 0x00007ffff54cc7fb in WTF::fastMalloc (n=24) at /media/ssd/WebKit/Source/WTF/wtf/FastMalloc.cpp:269
#18 0x00007ffff421e8f2 in WTF::RefCounted<WebCore::CSSValue>::operator new (size=24)
    at /media/ssd/WebKit/Source/WTF/wtf/RefCounted.h:197
#19 0x00007ffff424083a in WebCore::CSSPrimitiveValue::createIdentifier (identifier=3)
    at /media/ssd/WebKit/Source/WebCore/css/CSSPrimitiveValue.h:193
#20 0x00007ffff4301fde in WebCore::CSSValuePool::createIdentifierValue (this=0x4bb330, ident=3)
    at /media/ssd/WebKit/Source/WebCore/css/CSSValuePool.cpp:58
#21 0x00007ffff4294c93 in WebCore::CSSParser::parseValue (this=0x7fffffffbd10, propId=WebCore::CSSPropertyDisplay, 
    important=false) at /media/ssd/WebKit/Source/WebCore/css/CSSParser.cpp:1729
#22 0x00007ffff547adf9 in cssyyparse (parser=0x7fffffffbd10)
    at /media/ssd/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/CSSGrammar.y:1269
#23 0x00007ffff4290ed7 in WebCore::CSSParser::parseSheet (this=0x7fffffffbd10, sheet=0x4b9ca0, string=..., 
    startLineNumber=0, ruleSourceDataResult=0x0) at /media/ssd/WebKit/Source/WebCore/css/CSSParser.cpp:402
#24 0x00007ffff43cd327 in WebCore::StyleSheetContents::parseStringAtLine (this=0x4b9ca0, sheetText=..., 
    startLineNumber=0) at /media/ssd/WebKit/Source/WebCore/css/StyleSheetContents.cpp:311
#25 0x00007ffff43cd2b8 in WebCore::StyleSheetContents::parseString (this=0x4b9ca0, sheetText=...)
Comment 13 Sudarsana Nagineni (babu) 2012-11-01 06:13:15 PDT
Created attachment 171826 [details]
valgrind log

Attached the valgrind output.
Comment 14 Andreas Kling 2012-11-01 06:17:02 PDT
(In reply to comment #13)
> Created an attachment (id=171826) [details]
> valgrind log
> 
> Attached the valgrind output.

Thanks!
Comment 15 Andreas Kling 2012-11-01 06:51:24 PDT
Created attachment 171836 [details]
Ninja fix for the debug bots
Comment 16 Raphael Kubo da Costa (:rakuco) 2012-11-01 07:10:50 PDT
Comment on attachment 171836 [details]
Ninja fix for the debug bots

Clearing flags on attachment: 171836

Committed r133160: <http://trac.webkit.org/changeset/133160>