Bug 86948 - CSS: Move duplicate property elimination to parser.
: CSS: Move duplicate property elimination to parser.
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-05-19 13:42 PST by
Modified: 2012-05-21 12:36 PST (History)


Attachments
Proposed patch (12.37 KB, patch)
2012-05-19 14:10 PST, Andreas Kling
koivisto: review-
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (537.29 KB, application/zip)
2012-05-19 15:08 PST, WebKit Review Bot
no flags Details
Patch v2 (12.32 KB, patch)
2012-05-21 08:28 PST, Andreas Kling
koivisto: review+
Review Patch | Details | Formatted Diff | Diff
Landalicious patch (18.60 KB, patch)
2012-05-21 10:56 PST, Andreas Kling
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-19 13:42:24 PST
We currently have a special StylePropertySet constructor that deals with eliminating duplicate properties (with !important taken into account.)
This logic should really live in the parser. And let's make it more efficient while we're at it.
------- Comment #1 From 2012-05-19 14:10:48 PST -------
Created an attachment (id=142881) [details]
Proposed patch

Note: The PrimitiveSet class could be generalized into a fabulous container class if we figure out some great use-case for it.
------- Comment #2 From 2012-05-19 15:08:15 PST -------
(From update of attachment 142881 [details])
Attachment 142881 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12739027

New failing tests:
inspector/styles/styles-new-API.html
------- Comment #3 From 2012-05-19 15:08:19 PST -------
Created an attachment (id=142882) [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
------- Comment #4 From 2012-05-20 08:52:47 PST -------
(From update of attachment 142881 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=142881&action=review

Nice, except...

> Source/WebCore/css/CSSParser.cpp:1113
> +template<unsigned size>
> +class PrimitiveSet {

I don't quite understand the purpose of this type. Can't it be replaced with a BitVector of size numCSSProperties? The only benefit I see is that you avoid zeroing the memory. However zeroing 38 bytes is not that costly. With this the lookup and setting is slower (two reads/writes). Valgrind and pals are also likely to go crazy.
------- Comment #5 From 2012-05-20 17:08:53 PST -------
Cool, I think this will make the variables patch cleaner too by moving the variable de-duping logic into the parser.
------- Comment #6 From 2012-05-20 17:19:26 PST -------
(In reply to comment #4)
> (From update of attachment 142881 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142881&action=review
> 
> Nice, except...
> 
> > Source/WebCore/css/CSSParser.cpp:1113
> > +template<unsigned size>
> > +class PrimitiveSet {
> 
> I don't quite understand the purpose of this type. Can't it be replaced with a BitVector of size numCSSProperties? The only benefit I see is that you avoid zeroing the memory. However zeroing 38 bytes is not that costly. With this the lookup and setting is slower (two reads/writes). Valgrind and pals are also likely to go crazy.

Yeah, I kinda got tunnel-visioned on finding a use for this rad set implementation trick.. Will replace it by BitVectors.
------- Comment #7 From 2012-05-20 17:35:36 PST -------
Hmm, BitVector could really use templated inline capacity. Otherwise it is probably better to do without to avoid the mallocs.
------- Comment #8 From 2012-05-21 08:28:19 PST -------
Created an attachment (id=143040) [details]
Patch v2

Same thing with a fixed-size bit array instead. Again, this could be generalized for WTF but I'm not sure that's useful just yet.
------- Comment #9 From 2012-05-21 08:31:06 PST -------
Attachment 143040 [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/CSSParser.cpp:1114:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #10 From 2012-05-21 09:18:15 PST -------
(From update of attachment 143040 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=143040&action=review

> Source/WebCore/css/CSSParser.cpp:1113
> +template<unsigned fixedSize>
> +class FixedBitArray

I would use it if it was in WTF.

>> Source/WebCore/css/CSSParser.cpp:1114
>> +{
> 
> This { should be at the end of the previous line  [whitespace/braces] [4]

By Toutatis, the bot is right!

> Source/WebCore/css/CSSParser.cpp:1121
> +    void quickSet(unsigned index)

The quickFoo naming makes little sense with this type as there are no slow versions. For compatibility I guess?

> Source/WebCore/css/CSSParser.cpp:1124
> +        m_data[index / 8] |= 1 << (index & 7);

It would look more consistent if you used >> instead of / (here and elsewhere).
------- Comment #11 From 2012-05-21 09:30:40 PST -------
(From update of attachment 143040 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=143040&action=review

> Source/WebCore/css/CSSParser.cpp:1116
> +    FixedBitArray()

"Array" implies "Fixed"
------- Comment #12 From 2012-05-21 10:56:27 PST -------
Created an attachment (id=143060) [details]
Landalicious patch
------- Comment #13 From 2012-05-21 12:35:56 PST -------
(From update of attachment 143060 [details])
Clearing flags on attachment: 143060

Committed r117809: <http://trac.webkit.org/changeset/117809>
------- Comment #14 From 2012-05-21 12:36:01 PST -------
All reviewed patches have been landed.  Closing bug.