Bug 107777 – Split each RuleSet and feature out from StyleResolver into its own class.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Hayato Ito]

To address bug 89879, 'META: StyleResolver does too much and needs refactoring', I would like to factor out each RuleSet and feature from StyleResolver into  its own class to manage them separately.

We cannot do that soon since it requires other refactoring. Let me file them separately.

[Hayato Ito]

Created attachment 184387 [details]
WIP

[Alexey Proskuryakov]

Please measure how this affects WebCore binary size.

[Hayato Ito]

Thank you for the comment.

(In reply to comment #2)
> Please measure how this affects WebCore binary size.

Okay. Let me measure the binary size. I am going to measure the binary size in chromium linux port release build. I am wondering whethe this is enough or not.

If it might be better to measure the binary size in Mac port, please let me know that. I'd like to know the convention for measuring binary size.

[Dominic Cooney]

Comment on attachment 184387 [details]
WIP

I’m not a reviewer, but this looks like a clean way for StyleResolver to lose some weight.

[Dimitri Glazkov (Google)]

Comment on attachment 184387 [details]
WIP

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

This looks nice.

> Source/WebCore/css/DocumentRuleSets.h:47
> +class DefaultStyleSheets {

Should this be a singleton class instead?

> Source/WebCore/css/DocumentRuleSets.h:70
> +class DocumentCSSOMWrapper {

Same naming problem as the patch on bug 107779. Is this class significantly different from the one you have there?

> Source/WebCore/css/MediaQueryEvaluator.h:59
> +    explicit MediaQueryEvaluator(bool mediaFeatureResult = false);

That's a nice easy patch in itself.

> Source/WebCore/css/StyleResolver.h:181
> +    // FIXME: It could be better to call ruleSets().appendAuthorStyleSheets() directly when we factor StyleRsolver further.

Add a bug number here?

[Hayato Ito]

Thank you for the review. Note that some parts of this WIP patch are being under review in bug 107779.

(In reply to comment #5)
> (From update of attachment 184387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184387&action=review
> 
> This looks nice.
> 
> > Source/WebCore/css/DocumentRuleSets.h:47
> > +class DefaultStyleSheets {
> 
> Should this be a singleton class instead?

I am okay to use a singleton pattern here, but I am not sure which is preferred in WebKit.
Let me walk around code base and find which is a widely used convention.

I appreciated a comment about which is preferred.

> 
> > Source/WebCore/css/DocumentRuleSets.h:70
> > +class DocumentCSSOMWrapper {
> 
> Same naming problem as the patch on bug 107779. Is this class significantly different from the one you have there?

This WIP patch is a bit old. The patch is bug 107779 is latest and that was already reviewed in bug 107779. 
Let me rebase this WIP patch after a patch in 107779 is landed.

> 
> > Source/WebCore/css/MediaQueryEvaluator.h:59
> > +    explicit MediaQueryEvaluator(bool mediaFeatureResult = false);
> 
> That's a nice easy patch in itself.

That was already landed in http://trac.webkit.org/changeset/140503.

> 
> > Source/WebCore/css/StyleResolver.h:181
> > +    // FIXME: It could be better to call ruleSets().appendAuthorStyleSheets() directly when we factor StyleRsolver further.
> 
> Add a bug number here?

Let me add a bug number later.

[Hayato Ito]

Created attachment 186537 [details]
factor RuleSets and features

[Early Warning System Bot]

Comment on attachment 186537 [details]
factor RuleSets and features

Attachment 186537 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16372554

[Early Warning System Bot]

Comment on attachment 186537 [details]
factor RuleSets and features

Attachment 186537 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16368516

[Build Bot]

Comment on attachment 186537 [details]
factor RuleSets and features

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

[kov's GTK+ EWS bot]

Comment on attachment 186537 [details]
factor RuleSets and features

Attachment 186537 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16365646

[EFL EWS Bot]

Comment on attachment 186537 [details]
factor RuleSets and features

Attachment 186537 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16371571

[WebKit Review Bot]

Comment on attachment 186537 [details]
factor RuleSets and features

Attachment 186537 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16366602

[Peter Beverloo (cr-android ews)]

Comment on attachment 186537 [details]
factor RuleSets and features

Attachment 186537 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16370513

[Hayato Ito]

Created attachment 186540 [details]
Fix build errors.

[Dimitri Glazkov (Google)]

Comment on attachment 186540 [details]
Fix build errors.

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

> Source/WebCore/css/DocumentRuleSets.cpp:11
> + * Copyright (C) 2012 Google Inc. All rights reserved.

2013? :)

> Source/WebCore/css/DocumentRuleSets.h:54
> +    void initUserStyle(DocumentStyleSheetCollection*, const MediaQueryEvaluator&, StyleResolver&);

Should this just be a constructor with resetAuthorStyle in it?

> Source/WebCore/css/DocumentRuleSets.h:55
> +    void resetAuthorStyle();

This makes me wonder if author styles should sit by themselves, since they are the only ones that are mutable in this bunch.

> Source/WebCore/css/DocumentRuleSets.h:59
> +    void collectRulesFromUserStyleSheets(const Vector<RefPtr<CSSStyleSheet> >&, RuleSet& userStyle, const MediaQueryEvaluator&, StyleResolver&);

This can be private, right?

[Hayato Ito]

Thank you for the review. Let me land this patch after addressing some of the comments since i got r+.
I am happy to address the others in a following patch. See inline comments.

(In reply to comment #16)
> (From update of attachment 186540 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186540&action=review
> 
> > Source/WebCore/css/DocumentRuleSets.cpp:11
> > + * Copyright (C) 2012 Google Inc. All rights reserved.
> 
> 2013? :)

Done.

> 
> > Source/WebCore/css/DocumentRuleSets.h:54
> > +    void initUserStyle(DocumentStyleSheetCollection*, const MediaQueryEvaluator&, StyleResolver&);
> 
> Should this just be a constructor with resetAuthorStyle in it?


That's difficult since initUserStyle requires some parameters which should be calculated in StyleResolver's constructor. If we change m_ruleSets to OwnPtr, like OwnPtr<DocumentRuleSets> m_ruleSets', we can do it. I can address it in a following patch if required.

> 
> > Source/WebCore/css/DocumentRuleSets.h:55
> > +    void resetAuthorStyle();
> 
> This makes me wonder if author styles should sit by themselves, since they are the only ones that are mutable in this bunch.

Hmm. Could you clarify what that mean?

> 
> > Source/WebCore/css/DocumentRuleSets.h:59
> > +    void collectRulesFromUserStyleSheets(const Vector<RefPtr<CSSStyleSheet> >&, RuleSet& userStyle, const MediaQueryEvaluator&, StyleResolver&);
> 
> This can be private, right?

Yes!

[Hayato Ito]

Created attachment 186747 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 186747 [details]
Patch for landing

Clearing flags on attachment: 186747

Committed r141964: <http://trac.webkit.org/changeset/141964>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

Comment on attachment 186540 [details]
Fix build errors.

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

>>> Source/WebCore/css/DocumentRuleSets.h:54
>>> +    void initUserStyle(DocumentStyleSheetCollection*, const MediaQueryEvaluator&, StyleResolver&);
>> 
>> Should this just be a constructor with resetAuthorStyle in it?
> 
> That's difficult since initUserStyle requires some parameters which should be calculated in StyleResolver's constructor. If we change m_ruleSets to OwnPtr, like OwnPtr<DocumentRuleSets> m_ruleSets', we can do it. I can address it in a following patch if required.

No worries, just a suggestion.

>>> Source/WebCore/css/DocumentRuleSets.h:55
>>> +    void resetAuthorStyle();
>> 
>> This makes me wonder if author styles should sit by themselves, since they are the only ones that are mutable in this bunch.
> 
> Hmm. Could you clarify what that mean?

I disagree with myself. The author styles are used to populate features, which means that they can't live outside of this class :)

[Ryosuke Niwa]

This appears to have caused a 2.8% performance regression on Apple's internal test suite.

[Hayato Ito]

Thank you for reporting. That patch itself should be just a factoring and should not cause any regression.
Let me investigate. If you have any further info, let me know that.

(In reply to comment #22)
> This appears to have caused a 2.8% performance regression on Apple's internal test suite.

[Ryosuke Niwa]

It appears to have caused a slight dent in Chromium Win Perf test:
http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/intl2/report.html?history=150&rev=-1

as well as some of Parser/query-selector-* tests:
http://webkit-perf.appspot.com/graph.html#tests=[[3630247,2001,3001]]&sel=1360097136438.8433,1360136808728,328.6474501108647,364.56762749445676&displayrange=7&datatype=running

[WebKit Review Bot]

Re-opened since this is blocked by bug 109014

[Hayato Ito]

I can not reproduce on my machines using Parser/query-selector-* tests.
Hmm.

Let me watch the intl2 results.
It seems that result of intl2 was getting worse at the next cycle (@r141649) after this patch

(In reply to comment #24)
> It appears to have caused a slight dent in Chromium Win Perf test:
> http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/intl2/report.html?history=150&rev=-1
> 
> as well as some of Parser/query-selector-* tests:
> http://webkit-perf.appspot.com/graph.html#tests=[[3630247,2001,3001]]&sel=1360097136438.8433,1360136808728,328.6474501108647,364.56762749445676&displayrange=7&datatype=running

[Hayato Ito]

It seems the result of intl2 came back to the normal score at r141972, which is before reverting this patch at r141973. So this patch seems innocent.

Let me watch the result of intl2 after r141973. If nothing changes, let me re-land this patch.

If there is any concerns, please let me know it.

> http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/intl2/report.html?history=150&rev=-1

(In reply to comment #26)
> I can not reproduce on my machines using Parser/query-selector-* tests.
> Hmm.
> 
> Let me watch the intl2 results.
> It seems that result of intl2 was getting worse at the next cycle (@r141649) after this patch
> 
> (In reply to comment #24)
> > It appears to have caused a slight dent in Chromium Win Perf test:
> > http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/intl2/report.html?history=150&rev=-1
> > 
> > as well as some of Parser/query-selector-* tests:
> > http://webkit-perf.appspot.com/graph.html#tests=[[3630247,2001,3001]]&sel=1360097136438.8433,1360136808728,328.6474501108647,364.56762749445676&displayrange=7&datatype=running

[Hayato Ito]

The next page cycle test (inlcuding r141973) was done and I cannot find any difference in intl2 result as originally pointed out.

So let me re-land this patch. If this patch still causes performance regression, please feel free to revert this. I'll investigate it tomorrow in that case.

(In reply to comment #27)
> It seems the result of intl2 came back to the normal score at r141972, which is before reverting this patch at r141973. So this patch seems innocent.
> 
> Let me watch the result of intl2 after r141973. If nothing changes, let me re-land this patch.
> 
> If there is any concerns, please let me know it.
> 
> > http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/intl2/report.html?history=150&rev=-1
> 
> (In reply to comment #26)
> > I can not reproduce on my machines using Parser/query-selector-* tests.
> > Hmm.
> > 
> > Let me watch the intl2 results.
> > It seems that result of intl2 was getting worse at the next cycle (@r141649) after this patch
> > 
> > (In reply to comment #24)
> > > It appears to have caused a slight dent in Chromium Win Perf test:
> > > http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/intl2/report.html?history=150&rev=-1
> > > 
> > > as well as some of Parser/query-selector-* tests:
> > > http://webkit-perf.appspot.com/graph.html#tests=[[3630247,2001,3001]]&sel=1360097136438.8433,1360136808728,328.6474501108647,364.56762749445676&displayrange=7&datatype=running

[Antti Koivisto]

RuleSets and RuleDatas are implementation details of StyleResolver. The overly generic names weren't a huge problem when they were living in StyleResolver.cpp. As top level files and concepts they are confusing and this patch heads further to that direction. An important part of the refactoring should be understanding what these things are, giving them proper names and ensure they do things their name implies (RuleData is something like "StyleResolverRuleAnalysis" and so on).

I feel the whole "move Foo out of Bar" approach is overly mechanical except as a temporary intermediate step towards the end goal.

[Hayato Ito]

Created attachment 186810 [details]
Patch for landing

[Hayato Ito]

Ops. I've just noticed the comment. Thank you for the comment.

(In reply to comment #29)
> RuleSets and RuleDatas are implementation details of StyleResolver. The overly generic names weren't a huge problem when they were living in StyleResolver.cpp. As top level files and concepts they are confusing and this patch heads further to that direction. An important part of the refactoring should be understanding what these things are, giving them proper names and ensure they do things their name implies (RuleData is something like "StyleResolverRuleAnalysis" and so on).
> 
> I feel the whole "move Foo out of Bar" approach is overly mechanical except as a temporary intermediate step towards the end goal.

Agreed. it's important to have a good name. I've named it StyleResolverRuleSets once. But I've abandoned the idea..

Since DocumentRuleSets is only used from StyleResolver, prefixing it with StyleResolverXXX sounds reasonable. Let me address that later in another patch. Is that okay? I am now going to home.

[Antti Koivisto]

Comment on attachment 186810 [details]
Patch for landing

Lets wait if Ryosuke sees the regression going away as a result of revert before re-landing.

[Antti Koivisto]

Also I would prefer to fixing the existing types first. I see no point in propagating bad names.

[Ryosuke Niwa]

(In reply to comment #32)
> (From update of attachment 186810 [details])
> Lets wait if Ryosuke sees the regression going away as a result of revert before re-landing.

I didn't see any improvement when this patch was rolled out.

[Hayato Ito]

Thank you for the comments.

(In reply to comment #34)
> (In reply to comment #32)
> > (From update of attachment 186810 [details] [details])
> > Lets wait if Ryosuke sees the regression going away as a result of revert before re-landing.
> 
> I didn't see any improvement when this patch was rolled out.

Thank you for letting me know that. I think we can assume this is a false positive.

To move forward, we should have a good name to the factored class. I'd like to hear opinions. Which do you prefer? Any recommendation?

1 .DocumentRuleSets
2. StyleResolverRuleSets
3. (Insert other good names here.)

[Hayato Ito]

If there is no objection to the current naming in a few days, let me land this patch.

(In reply to comment #35)
> Thank you for the comments.
> 
> (In reply to comment #34)
> > (In reply to comment #32)
> > > (From update of attachment 186810 [details] [details] [details])
> > > Lets wait if Ryosuke sees the regression going away as a result of revert before re-landing.
> > 
> > I didn't see any improvement when this patch was rolled out.
> 
> Thank you for letting me know that. I think we can assume this is a false positive.
> 
> To move forward, we should have a good name to the factored class. I'd like to hear opinions. Which do you prefer? Any recommendation?
> 
> 1 .DocumentRuleSets
> 2. StyleResolverRuleSets
> 3. (Insert other good names here.)

[WebKit Review Bot]

Comment on attachment 186810 [details]
Patch for landing

Clearing flags on attachment: 186810

Committed r142573: <http://trac.webkit.org/changeset/142573>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.