Bug 165743 - [CSS Parser] Implement deferred parsing of properties, @media, @supports and @keyframes
Summary: [CSS Parser] Implement deferred parsing of properties, @media, @supports and ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords: InRadar
Depends on:
Blocks: 240244
  Show dependency treegraph
 
Reported: 2016-12-11 15:29 PST by Dave Hyatt
Modified: 2022-05-09 11:57 PDT (History)
8 users (show)

See Also:


Attachments
Patch (116.60 KB, patch)
2016-12-11 15:32 PST, Dave Hyatt
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (1.12 MB, application/zip)
2016-12-11 16:44 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.37 MB, application/zip)
2016-12-11 16:49 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (1.85 MB, application/zip)
2016-12-11 16:51 PST, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (12.10 MB, application/zip)
2016-12-11 17:00 PST, Build Bot
no flags Details
Patch (117.65 KB, patch)
2016-12-11 17:33 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (119.47 KB, patch)
2016-12-11 17:53 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (118.81 KB, patch)
2016-12-12 08:42 PST, Dave Hyatt
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.22 MB, application/zip)
2016-12-12 09:34 PST, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-yosemite (1.11 MB, application/zip)
2016-12-12 09:47 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (1.73 MB, application/zip)
2016-12-12 09:55 PST, Build Bot
no flags Details
Patch making the back ptr as WeakPtr (120.51 KB, patch)
2016-12-12 10:14 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (133.01 KB, patch)
2016-12-14 11:30 PST, Dave Hyatt
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2016-12-11 15:29:27 PST
[CSS Parser] Implement deferred parsing of properties, @media, @supports and @keyframes
Comment 1 Dave Hyatt 2016-12-11 15:32:26 PST
Created attachment 296875 [details]
Patch
Comment 2 WebKit Commit Bot 2016-12-11 15:33:41 PST
Attachment 296875 [details] did not pass style-queue:


ERROR: Source/WebCore/css/parser/CSSParserImpl.h:72:  The parameter name "wrapper" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/css/parser/CSSParserImpl.h:106:  The parameter name "deferredParser" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/css/parser/CSSParserImpl.h:107:  The parameter name "deferredParser" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/css/parser/CSSDeferredParser.cpp:45:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/parser/CSSDeferredParser.cpp:50:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/testing/Internals.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 6 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Build Bot 2016-12-11 16:44:34 PST
Comment on attachment 296875 [details]
Patch

Attachment 296875 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2700265

New failing tests:
editing/execCommand/5142012-1.html
editing/inserting/insert-at-end-02.html
editing/pasteboard/4989774.html
editing/execCommand/nsresponder-outdent.html
Comment 4 Build Bot 2016-12-11 16:44:37 PST
Created attachment 296878 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 5 Build Bot 2016-12-11 16:49:35 PST
Comment on attachment 296875 [details]
Patch

Attachment 296875 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2700274

New failing tests:
editing/execCommand/5142012-1.html
editing/inserting/insert-at-end-02.html
editing/pasteboard/4989774.html
editing/execCommand/nsresponder-outdent.html
Comment 6 Build Bot 2016-12-11 16:49:39 PST
Created attachment 296879 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 7 Build Bot 2016-12-11 16:51:35 PST
Comment on attachment 296875 [details]
Patch

Attachment 296875 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2700269

New failing tests:
editing/execCommand/5142012-1.html
editing/inserting/insert-at-end-02.html
editing/pasteboard/4989774.html
editing/execCommand/nsresponder-outdent.html
Comment 8 Build Bot 2016-12-11 16:51:39 PST
Created attachment 296880 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 9 Build Bot 2016-12-11 17:00:18 PST
Comment on attachment 296875 [details]
Patch

Attachment 296875 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2700278

New failing tests:
editing/execCommand/5142012-1.html
fast/css/deferred-parsing/hover-test.html
Comment 10 Build Bot 2016-12-11 17:00:21 PST
Created attachment 296881 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 11 Dave Hyatt 2016-12-11 17:33:38 PST
Created attachment 296883 [details]
Patch
Comment 12 Dave Hyatt 2016-12-11 17:53:17 PST
Created attachment 296885 [details]
Patch
Comment 13 Darin Adler 2016-12-11 18:22:33 PST
Comment on attachment 296885 [details]
Patch

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

Looks like a good optimization. I have some style suggestions.

I wonder about some of these classes, why they are reference counted and why are they designed to be heap allocated at all. Can’t some of them just be objects that we move around and not be heap allocated at all? Or use unique_ptr instead of RefPtr.

> Source/WebCore/css/StyleProperties.cpp:1376
> +{ }

These should be on two subsequent lines.

> Source/WebCore/css/StyleProperties.h:266
> +    static Ref<DeferredStyleProperties> create(const CSSParserTokenRange&, RefPtr<CSSDeferredParser>&);

The argument here should just be CSSDeferredParser* or CSSDeferredParser& if it can never be null; I see no reason to pass in a RefPtr&.

> Source/WebCore/css/StyleProperties.h:274
> +    RefPtr<CSSDeferredParser> m_parser;

Can this be Ref instead of RefPtr?

> Source/WebCore/css/StyleRule.cpp:335
> +{ }

These should not be on the same line like this.

> Source/WebCore/css/StyleRule.h:194
> +    static Ref<DeferredStyleGroupRuleList> create(const CSSParserTokenRange&, RefPtr<CSSDeferredParser>&);

Same comment as above about RefPtr&.

> Source/WebCore/css/StyleRule.h:197
> +    void parseDeferredRules(Vector<RefPtr<StyleRuleBase>>&);
> +    void parseDeferredKeyframes(StyleRuleKeyframes&);

These functions should use return values instead of out arguments.

> Source/WebCore/css/StyleRule.h:200
> +    DeferredStyleGroupRuleList(const CSSParserTokenRange&, RefPtr<CSSDeferredParser>&);

Same comment as above about RefPtr&.

> Source/WebCore/css/StyleRule.h:203
> +    RefPtr<CSSDeferredParser> m_parser;

Ref instead of RefPtr?

> Source/WebCore/css/StyleRule.h:215
> +    StyleRuleGroup(Type, Vector<RefPtr<StyleRuleBase>>&);

Since this adopts the Vector, it really should be Vector&& instead of Vector&. The old way was the way we did this kind of thing before we had move semantics. Not new to the patch, though.

> Source/WebCore/css/StyleSheetContents.cpp:432
> +            const StyleProperties* properties = downcast<StyleRule>(*rule).propertiesWithoutDeferredParsing();

auto* would work here.

> Source/WebCore/css/StyleSheetContents.cpp:442
> +            const Vector<RefPtr<StyleRuleBase>>* childRules = downcast<StyleRuleMedia>(*rule).childRulesWithoutDeferredParsing();

I would use auto* on a line like this so it’s not so crazy long.

> Source/WebCore/css/parser/CSSDeferredParser.h:53
> +    void parseRuleList(const CSSParserTokenRange&, Vector<RefPtr<StyleRuleBase>>&);
> +    void parseKeyframeList(const CSSParserTokenRange&, StyleRuleKeyframes&);

These functions should have return values instead of out arguments.

> Source/WebCore/css/parser/CSSDeferredParser.h:60
> +    StyleSheetContents* m_styleSheet; // Safe, just a back pointer.

I don’t buy the "safe" comment. What makes it safe? What guarantees this object never outlasts the StyleSheetContents? Do we clear this out or something? This is reference counted, so clearly someone could keep it around longer than they should.

> Source/WebCore/css/parser/CSSParser.h:53
> +    enum class RuleParsing {
> +        NormalRuleParsing,
> +        DeferredRuleParsing
> +    };

Shouldn’t repeat RuleParsing in the names. Also, I would write this on one line just before it’s used:

    enum class RuleParsing { Normal, Deferred };
    void parseSheet ...

> Source/WebCore/css/parser/CSSParser.h:55
>      WEBCORE_EXPORT CSSParser(const CSSParserContext&);

Should mark this explicit.

> Source/WebCore/css/parser/CSSParserImpl.cpp:62
>      , m_observerWrapper(nullptr)

This should be initialized in the class definition instead of here.

> Source/WebCore/css/parser/CSSParserImpl.cpp:65
>  {
> +    
> +}

Left a stray blank line.

> Source/WebCore/css/parser/CSSParserImpl.cpp:71
> +    , m_observerWrapper(nullptr)

This should be initialized in the class definition instead of here.

> Source/WebCore/css/parser/CSSParserImpl.cpp:185
> +    parser.consumeRuleList(tokenRange, RegularRuleList, [&childRules](RefPtr<StyleRuleBase> rule) {

I think this should be "const RefPtr<StyleRuleBase>&" instead of RefPtr, unless it can’t be.

> Source/WebCore/css/parser/CSSParserImpl.cpp:193
> +    parser.consumeRuleList(tokenRange, KeyframesRuleList, [&keyframeRule](RefPtr<StyleRuleBase> keyframe) {

I think this should be "const RefPtr<StyleRuleBase>&" instead of RefPtr, unless it can’t be.

> Source/WebCore/css/parser/CSSParserImpl.cpp:195
> +        RefPtr<StyleKeyframe> key = static_cast<StyleKeyframe*>(keyframe.get());
> +        keyframeRule.parserAppendKeyframe(key.releaseNonNull());

This doesn’t really work. We end up doing all this releaseNonNull stuff and we still churn the reference count. I would write this:

    keyframeRule.parserAppendKeyframe(*static_cast<StyleKeyframe*>(keyframe.get()));

But then it makes me wonder two things:

1) Why is the argument to the function in the consumeRuleList thing a RefPtr<StyleRuleBase>; it should either be StyleRuleBase& or Ref<StyleRuleBase>&&.
2) Why don’t we make a safe way to do these casts, like the downcast we made for the DOM? It seems risky to have to write out static_cast.

> Source/WebCore/css/parser/CSSParserImpl.cpp:536
> +        return StyleRuleMedia::create(MediaQueryParser::parseMediaQuerySet(prelude).releaseNonNull(), DeferredStyleGroupRuleList::create(block, m_deferredParser));

What guarantees parseMediaQuerySet won’t return null? It’s usually a danger sign to be calling releaseNonNull without a null check. Means the function should have been returning a Ref all along, there is a missing null check, or there is some secret erase we know it won’t be null that the type system doesn’t know.

> Source/WebCore/css/parser/CSSParserImpl.cpp:643
> +    RefPtr<StyleRuleKeyframes> keyframeRule = StyleRuleKeyframes::create(name);

Should use auto here.

> Source/WebCore/css/parser/CSSParserImpl.cpp:647
>      consumeRuleList(block, KeyframesRuleList, [keyframeRule](RefPtr<StyleRuleBase> keyframe) {
>          RefPtr<StyleKeyframe> key = static_cast<StyleKeyframe*>(keyframe.get());
>          keyframeRule->parserAppendKeyframe(key.releaseNonNull());
>      });

Same bad pattern here that I mentioned above, all that RefPtr and releaseNonNull is not really working the way we want it to. Maybe I should have read more of this code before. Didn’t see this before.

> Source/WebCore/testing/Internals.cpp:887
> +    StyleSheetContents& contents = downcast<CSSStyleSheet>(styleSheet).contents();
> +    return deferredStyleRulesCountForList(contents.childRules());

I would use auto& or write this all on one line without the local variable.

> Source/WebCore/testing/Internals.cpp:914
> +    StyleSheetContents& contents = downcast<CSSStyleSheet>(styleSheet).contents();
> +    return deferredGroupRulesCountForList(contents.childRules());

Ditto.

> Source/WebCore/testing/Internals.h:44
> +class CSSRuleList;

Not sure why we’re adding this. Don’t see it being used.

> Source/WebCore/testing/Internals.h:125
> +    int deferredStyleRulesCount(StyleSheet&);
> +    int deferredGroupRulesCount(StyleSheet&);
> +    int deferredKeyframesRulesCount(StyleSheet&);

Would have expected unsigned here instead of int. Obviously doesn’t matter, though.

> Source/WebCore/testing/Internals.idl:114
> +    long deferredStyleRulesCount(StyleSheet sheet);
> +    long deferredGroupRulesCount(StyleSheet sheet);
> +    long deferredKeyframesRulesCount(StyleSheet sheet);

unsigned long

> LayoutTests/platform/ios-simulator/TestExpectations:2753
> +# Hover test does not apply

Isn’t there a single place we can put this to cover all variations of iOS, rather than repeating it three times?
Comment 14 Dave Hyatt 2016-12-12 02:01:52 PST
(In reply to comment #13)
> Comment on attachment 296885 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296885&action=review
> 
> Looks like a good optimization. I have some style suggestions.
> 
> I wonder about some of these classes, why they are reference counted and why
> are they designed to be heap allocated at all. Can’t some of them just be
> objects that we move around and not be heap allocated at all? Or use
> unique_ptr instead of RefPtr.

Yeah, you are right. I am going to change DeferredStyleGroupRuleList to be held in a std::unique_ptr. It doesn't have to be reference counted.

Technically DeferredStyleProperties is only refcounted because it's acting as a "stand-in" for the real properties on StyleRule. I did that to avoid adding an extra member to all StyleRules (like I ended up doing with StyleRuleGroup).

If I did just add an extra member to StyleRule, then DeferredStyleProperties could be a std::unique_ptr also, but I liked the stand-in idea as a way of saving that extra pointer per rule once the rule's properties get parsed and all the deferred stuff goes away.


> > Source/WebCore/css/StyleProperties.h:274
> > +    RefPtr<CSSDeferredParser> m_parser;
> 
> Can this be Ref instead of RefPtr?
> 

It can yeah. Changing.


> > Source/WebCore/css/StyleRule.h:203
> > +    RefPtr<CSSDeferredParser> m_parser;
> 
> Ref instead of RefPtr?
>

Yeah, same as above. Will change.
 

> > Source/WebCore/css/parser/CSSDeferredParser.h:60
> > +    StyleSheetContents* m_styleSheet; // Safe, just a back pointer.
> 
> I don’t buy the "safe" comment. What makes it safe? What guarantees this
> object never outlasts the StyleSheetContents? Do we clear this out or
> something? This is reference counted, so clearly someone could keep it
> around longer than they should.
>

I'm going to try to eliminate this back pointer completely and just make it a non-issue.


> > Source/WebCore/css/parser/CSSParserImpl.cpp:536
> > +        return StyleRuleMedia::create(MediaQueryParser::parseMediaQuerySet(prelude).releaseNonNull(), DeferredStyleGroupRuleList::create(block, m_deferredParser));
> 
> What guarantees parseMediaQuerySet won’t return null? It’s usually a danger
> sign to be calling releaseNonNull without a null check. Means the function
> should have been returning a Ref all along, there is a missing null check,
> or there is some secret erase we know it won’t be null that the type system
> doesn’t know.
> 

I screwed this up when I converted Blink's MediaQueryParser to use our memory management. I had to go through a zillion files doing this conversion to Ref/RefPtr by hand, and I seem to have made a mistake here. The query returned cannot be null, but I used RefPtrs when I should have used Refs. That means this code is ok, but I will file a follow-up bug to patch MediaQueryParser to use Refs instead of RefPtrs.

> > Source/WebCore/css/parser/CSSParserImpl.cpp:643
> > +    RefPtr<StyleRuleKeyframes> keyframeRule = StyleRuleKeyframes::create(name);
> 
> Should use auto here.
> 
> > Source/WebCore/css/parser/CSSParserImpl.cpp:647
> >      consumeRuleList(block, KeyframesRuleList, [keyframeRule](RefPtr<StyleRuleBase> keyframe) {
> >          RefPtr<StyleKeyframe> key = static_cast<StyleKeyframe*>(keyframe.get());
> >          keyframeRule->parserAppendKeyframe(key.releaseNonNull());
> >      });
> 
> Same bad pattern here that I mentioned above, all that RefPtr and
> releaseNonNull is not really working the way we want it to. Maybe I should
> have read more of this code before. Didn’t see this before.
> 

The downcast does exist for this, so it can be written like this:

parser.consumeRuleList(tokenRange, KeyframesRuleList, [&keyframeRule](const RefPtr<StyleRuleBase>& keyframe) {
        keyframeRule.parserAppendKeyframe(downcast<const StyleKeyframe>(keyframe.get()));
    });

Better?


> > Source/WebCore/testing/Internals.h:44
> > +class CSSRuleList;
> 
> Not sure why we’re adding this. Don’t see it being used.
> 

Just clutter. I was originally using CSSRuleList as the API argument and then changed to CSSStyleSheet.


> > LayoutTests/platform/ios-simulator/TestExpectations:2753
> > +# Hover test does not apply
> 
> Isn’t there a single place we can put this to cover all variations of iOS,
> rather than repeating it three times?

Maybe? I'm not sure. I'll ask tomorrow.

Thanks for the thorough review. I think there's enough here to warrant a re-review, even though you gave it r+, so I'm going to post another patch once I get all this addressed.
Comment 15 Dave Hyatt 2016-12-12 08:42:17 PST
Created attachment 296920 [details]
Patch
Comment 16 Build Bot 2016-12-12 09:34:53 PST
Comment on attachment 296920 [details]
Patch

Attachment 296920 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2705707

New failing tests:
printing/page-rule-css-text.html
Comment 17 Build Bot 2016-12-12 09:34:56 PST
Created attachment 296925 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 18 Build Bot 2016-12-12 09:47:29 PST
Comment on attachment 296920 [details]
Patch

Attachment 296920 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2705809

New failing tests:
printing/page-rule-css-text.html
Comment 19 Build Bot 2016-12-12 09:47:33 PST
Created attachment 296928 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 20 Build Bot 2016-12-12 09:55:31 PST
Comment on attachment 296920 [details]
Patch

Attachment 296920 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2705821

New failing tests:
printing/page-rule-css-text.html
Comment 21 Build Bot 2016-12-12 09:55:35 PST
Created attachment 296930 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 22 Dave Hyatt 2016-12-12 10:14:37 PST
Created attachment 296932 [details]
Patch making the back ptr as WeakPtr
Comment 23 WebKit Commit Bot 2016-12-12 10:18:10 PST
Attachment 296932 [details] did not pass style-queue:


ERROR: Source/WebCore/css/parser/CSSDeferredParser.h:57:  The parameter name "styleSheet" adds no information, so it should be removed.  [readability/parameter_name] [5]
WARNING: Not running on native Windows.
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Dave Hyatt 2016-12-12 11:55:56 PST
Landed in r209718.
Comment 25 Darin Adler 2016-12-12 13:42:17 PST
Sorry, did not get a chance to re-review. But I will look again once I get a chance.
Comment 26 Gavin Barraclough 2016-12-14 00:26:03 PST
Rolled out in r209794 due to memory regression.
Comment 27 Dave Hyatt 2016-12-14 08:38:56 PST
(In reply to comment #26)
> Rolled out in r209794 due to memory regression.

Maybe next time you could mention that in your changelog so I don't sit there wondering why the hell this was rolled out.
Comment 28 Dave Hyatt 2016-12-14 11:30:07 PST
Created attachment 297105 [details]
Patch
Comment 29 Dave Hyatt 2016-12-14 13:02:58 PST
Landed in r209826.
Comment 30 Csaba Osztrogonác 2016-12-14 13:23:26 PST
(In reply to comment #29)
> Landed in r209826.

It broke the Apple Mac build everywhere, see build.webkit.org for details.
Comment 31 Radar WebKit Bug Importer 2016-12-21 15:36:51 PST
<rdar://problem/29779237>