Bug 86948 - CSS: Move duplicate property elimination to parser.
: CSS: Move duplicate property elimination to parser.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Andreas Kling
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-19 13:42 PDT by Andreas Kling
Modified: 2012-05-21 12:36 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (12.37 KB, patch)
2012-05-19 14:10 PDT, Andreas Kling
koivisto: review-
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (537.29 KB, application/zip)
2012-05-19 15:08 PDT, WebKit Review Bot
no flags Details
Patch v2 (12.32 KB, patch)
2012-05-21 08:28 PDT, Andreas Kling
koivisto: review+
Details | Formatted Diff | Diff
Landalicious patch (18.60 KB, patch)
2012-05-21 10:56 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-05-19 13:42:24 PDT
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 Andreas Kling 2012-05-19 14:10:48 PDT
Created attachment 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 WebKit Review Bot 2012-05-19 15:08:15 PDT
Comment on attachment 142881 [details]
Proposed patch

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 WebKit Review Bot 2012-05-19 15:08:19 PDT
Created attachment 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 Antti Koivisto 2012-05-20 08:52:47 PDT
Comment on attachment 142881 [details]
Proposed patch

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 Luke Macpherson 2012-05-20 17:08:53 PDT
Cool, I think this will make the variables patch cleaner too by moving the variable de-duping logic into the parser.
Comment 6 Andreas Kling 2012-05-20 17:19:26 PDT
(In reply to comment #4)
> (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.

Yeah, I kinda got tunnel-visioned on finding a use for this rad set implementation trick.. Will replace it by BitVectors.
Comment 7 Antti Koivisto 2012-05-20 17:35:36 PDT
Hmm, BitVector could really use templated inline capacity. Otherwise it is probably better to do without to avoid the mallocs.
Comment 8 Andreas Kling 2012-05-21 08:28:19 PDT
Created attachment 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 WebKit Review Bot 2012-05-21 08:31:06 PDT
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 Antti Koivisto 2012-05-21 09:18:15 PDT
Comment on attachment 143040 [details]
Patch v2

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 Antti Koivisto 2012-05-21 09:30:40 PDT
Comment on attachment 143040 [details]
Patch v2

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 Andreas Kling 2012-05-21 10:56:27 PDT
Created attachment 143060 [details]
Landalicious patch
Comment 13 WebKit Review Bot 2012-05-21 12:35:56 PDT
Comment on attachment 143060 [details]
Landalicious patch

Clearing flags on attachment: 143060

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