Bug 107777

Created attachment 184387 [details] WIP

Please measure how this affects WebCore binary size.

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 on attachment 184387 [details] WIP I’m not a reviewer, but this looks like a clean way for StyleResolver to lose some weight.

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?

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.

Created attachment 186537 [details] factor RuleSets and features

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 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 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 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 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 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 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

Created attachment 186540 [details] Fix build errors.

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?

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!

Created attachment 186747 [details] Patch for landing

Comment on attachment 186747 [details] Patch for landing Clearing flags on attachment: 186747 Committed r141964: <http://trac.webkit.org/changeset/141964>

All reviewed patches have been landed. Closing bug.

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 :)

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

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.

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

Re-opened since this is blocked by bug 109014

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

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

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

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.

Created attachment 186810 [details] Patch for landing

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 on attachment 186810 [details] Patch for landing Lets wait if Ryosuke sees the regression going away as a result of revert before re-landing.

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

(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.

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.)

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 on attachment 186810 [details] Patch for landing Clearing flags on attachment: 186810 Committed r142573: <http://trac.webkit.org/changeset/142573>

All reviewed patches have been landed. Closing bug.