Bug 107777 - Split each RuleSet and feature out from StyleResolver into its own class.
Summary: Split each RuleSet and feature out from StyleResolver into its own class.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on: 107779 107780 108797 109014
Blocks: 89879 108890
  Show dependency treegraph
 
Reported: 2013-01-23 20:06 PST by Hayato Ito
Modified: 2013-02-11 20:33 PST (History)
20 users (show)

See Also:


Attachments
WIP (69.00 KB, patch)
2013-01-23 20:13 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
factor RuleSets and features (38.81 KB, patch)
2013-02-04 21:06 PST, Hayato Ito
peter+ews: commit-queue-
Details | Formatted Diff | Diff
Fix build errors. (39.26 KB, patch)
2013-02-04 21:25 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch for landing (39.31 KB, patch)
2013-02-05 19:56 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch for landing (38.89 KB, patch)
2013-02-06 03:09 PST, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2013-01-23 20:06:52 PST
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.
Comment 1 Hayato Ito 2013-01-23 20:13:59 PST
Created attachment 184387 [details]
WIP
Comment 2 Alexey Proskuryakov 2013-01-24 09:58:06 PST
Please measure how this affects WebCore binary size.
Comment 3 Hayato Ito 2013-01-28 00:26:27 PST
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.
Comment 4 Dominic Cooney 2013-01-28 05:59:55 PST
Comment on attachment 184387 [details]
WIP

Iā€™m not a reviewer, but this looks like a clean way for StyleResolver to lose some weight.
Comment 5 Dimitri Glazkov (Google) 2013-01-28 10:10:18 PST
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?
Comment 6 Hayato Ito 2013-01-29 00:11:48 PST
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.
Comment 7 Hayato Ito 2013-02-04 21:06:48 PST
Created attachment 186537 [details]
factor RuleSets and features
Comment 8 Early Warning System Bot 2013-02-04 21:12:23 PST
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
Comment 9 Early Warning System Bot 2013-02-04 21:13:04 PST
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
Comment 10 Build Bot 2013-02-04 21:13:25 PST
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
Comment 11 kov's GTK+ EWS bot 2013-02-04 21:14:31 PST
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
Comment 12 EFL EWS Bot 2013-02-04 21:15:32 PST
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
Comment 13 WebKit Review Bot 2013-02-04 21:16:31 PST
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
Comment 14 Peter Beverloo (cr-android ews) 2013-02-04 21:25:12 PST
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
Comment 15 Hayato Ito 2013-02-04 21:25:16 PST
Created attachment 186540 [details]
Fix build errors.
Comment 16 Dimitri Glazkov (Google) 2013-02-05 10:41:03 PST
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?
Comment 17 Hayato Ito 2013-02-05 19:52:31 PST
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!
Comment 18 Hayato Ito 2013-02-05 19:56:30 PST
Created attachment 186747 [details]
Patch for landing
Comment 19 WebKit Review Bot 2013-02-05 20:30:33 PST
Comment on attachment 186747 [details]
Patch for landing

Clearing flags on attachment: 186747

Committed r141964: <http://trac.webkit.org/changeset/141964>
Comment 20 WebKit Review Bot 2013-02-05 20:30:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Dimitri Glazkov (Google) 2013-02-05 20:53:59 PST
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 :)
Comment 22 Ryosuke Niwa 2013-02-05 23:36:32 PST
This appears to have caused a 2.8% performance regression on Apple's internal test suite.
Comment 23 Hayato Ito 2013-02-05 23:45:28 PST
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.
Comment 25 WebKit Review Bot 2013-02-05 23:57:56 PST
Re-opened since this is blocked by bug 109014
Comment 26 Hayato Ito 2013-02-06 01:31:36 PST
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
Comment 27 Hayato Ito 2013-02-06 01:41:10 PST
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
Comment 28 Hayato Ito 2013-02-06 02:59:30 PST
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
Comment 29 Antti Koivisto 2013-02-06 03:08:57 PST
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.
Comment 30 Hayato Ito 2013-02-06 03:09:25 PST
Created attachment 186810 [details]
Patch for landing
Comment 31 Hayato Ito 2013-02-06 03:15:46 PST
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.
Comment 32 Antti Koivisto 2013-02-06 03:40:06 PST
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.
Comment 33 Antti Koivisto 2013-02-06 03:42:36 PST
Also I would prefer to fixing the existing types first. I see no point in propagating bad names.
Comment 34 Ryosuke Niwa 2013-02-06 13:58:21 PST
(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.
Comment 35 Hayato Ito 2013-02-06 17:53:48 PST
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.)
Comment 36 Hayato Ito 2013-02-08 01:06:13 PST
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.)
Comment 37 WebKit Review Bot 2013-02-11 20:33:43 PST
Comment on attachment 186810 [details]
Patch for landing

Clearing flags on attachment: 186810

Committed r142573: <http://trac.webkit.org/changeset/142573>
Comment 38 WebKit Review Bot 2013-02-11 20:33:52 PST
All reviewed patches have been landed.  Closing bug.