Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2610513003:
Avoid unnecessary updateActiveStyle comparing shadow styles. (Closed)
|
Created:
3 years, 11 months ago by rune Modified:
3 years, 11 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-dom_chromium.org, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, webcomponents-bugzilla_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid unnecessary updateActiveStyle comparing shadow styles.
Found while checking performance for issue 603621.
Style sharing is done during style recalc at which point we know that
the active style is up-to-date. Instead of using the API for
document.styleSheets, compare active stylesheets in ScopedStyleResolver
directly.
BUG=603621
Committed: https://crrev.com/f831fc79824c0281cdd4f6a90d7bc92dbadd44b0
Cr-Commit-Position: refs/heads/master@{#441357}
Patch Set 1 #Patch Set 2 : Added tests. #
Total comments: 7
Patch Set 3 : Fixed review issues. #
Total comments: 4
Patch Set 4 : Added documentation. #
Messages
Total messages: 33 (15 generated)
The CQ bit was checked by rune@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rune@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rune@opera.com changed reviewers: + sashab@chromium.org
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp (right): https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp:304: unsigned secondCount = second ? second->m_authorStyleSheets.size() : 0; Using size_t would avoid the (unlikely) problem of the two size_t conversions end up with the same unsigned values, despite being different sizes. And prevent some compilers from issuing warnings. https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h (right): https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h:79: CORE_EXPORT static bool hasSameStyles(const ScopedStyleResolver*, s/has/have/, but why not an equality method rather than a static binary predicate?
https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h (right): https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h:79: CORE_EXPORT static bool hasSameStyles(const ScopedStyleResolver*, On 2017/01/03 18:48:34, sof wrote: > s/has/have/, but why not an equality method rather than a static binary > predicate? You mean non-static ScopedStyleResolver::hasSameStyles(const ScopedStyleResolver*) ?
On 2017/01/03 18:52:31, rune wrote: > https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h (right): > > https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h:79: > CORE_EXPORT static bool hasSameStyles(const ScopedStyleResolver*, > On 2017/01/03 18:48:34, sof wrote: > > s/has/have/, but why not an equality method rather than a static binary > > predicate? > > You mean non-static ScopedStyleResolver::hasSameStyles(const > ScopedStyleResolver*) ? Oh, it's because I wanted to treat null resolvers and zero-sized author sheet vectors the same. A static method taking null pointers seemed most elegant.
https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h (right): https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h:79: CORE_EXPORT static bool hasSameStyles(const ScopedStyleResolver*, On 2017/01/03 18:52:31, rune wrote: > On 2017/01/03 18:48:34, sof wrote: > > s/has/have/, but why not an equality method rather than a static binary > > predicate? > > You mean non-static ScopedStyleResolver::hasSameStyles(const > ScopedStyleResolver*) ? ok (wrt static.) Basic question -- are these author stylesheets ordered somehow that this basic pointer equality checking is sufficiently precise to determine "same"ness?
https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h (right): https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h:79: CORE_EXPORT static bool hasSameStyles(const ScopedStyleResolver*, On 2017/01/03 19:11:17, sof wrote: > On 2017/01/03 18:52:31, rune wrote: > > On 2017/01/03 18:48:34, sof wrote: > > > s/has/have/, but why not an equality method rather than a static binary > > > predicate? > > > > You mean non-static ScopedStyleResolver::hasSameStyles(const > > ScopedStyleResolver*) ? > > ok (wrt static.) Basic question -- are these author stylesheets ordered somehow > that this basic pointer equality checking is sufficiently precise to determine > "same"ness? The stylesheets are in DOM tree order for the given TreeScope. Quite often web components only contain a single style element whose StyleSheetContents will be shared between multiple instances of the same component because of the String <-> StyleSheetContents cache in StyleEngine. Even if there are more than one style element, the ordering will be the same in multiple instances of the same component. The sameness is used to decide if we can share ComputedStyle objects between different shadow hosts.
https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp (right): https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp:304: unsigned secondCount = second ? second->m_authorStyleSheets.size() : 0; On 2017/01/03 18:48:34, sof wrote: > Using size_t would avoid the (unlikely) problem of the two size_t conversions > end up with the same unsigned values, despite being different sizes. And prevent > some compilers from issuing warnings. Done. https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h (right): https://codereview.chromium.org/2610513003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h:79: CORE_EXPORT static bool hasSameStyles(const ScopedStyleResolver*, On 2017/01/03 18:48:34, sof wrote: > s/has/have/, but why not an equality method rather than a static binary > predicate? Changed has -> have.
Nice patch overall! :) https://codereview.chromium.org/2610513003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp (right): https://codereview.chromium.org/2610513003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp:308: if (first->m_authorStyleSheets[firstCount]->contents() != I know this is what the old code did, but is there a builtin for this? If m_authorStyleSheets return iterators, maybe there's a C++ builtin we can use. Just feels like a lot of code for a pretty standard comparator function. Also I think we prefer iterator for-loops to while loops.
https://codereview.chromium.org/2610513003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp (right): https://codereview.chromium.org/2610513003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp:308: if (first->m_authorStyleSheets[firstCount]->contents() != On 2017/01/03 22:35:19, sashab wrote: > I know this is what the old code did, but is there a builtin for this? If > m_authorStyleSheets return iterators, maybe there's a C++ builtin we can use. > > Just feels like a lot of code for a pretty standard comparator function. Also I > think we prefer iterator for-loops to while loops. I don't think we can compare these vectors with operator== because we need to compare sheet->contents(), not sheet. To make this code simpler to read, I guess you'd need a vector comparator which takes a custom comparator for the elements. I don't know if there's such a thing. Here's an alternative: bool ScopedStyleResolver::haveSameStyles(const ScopedStyleResolver* first, const ScopedStyleResolver* second) { if (!first) return !second || second->m_authorStyleSheets.isEmpty(); if (!second) return first->m_authorStyleSheets.isEmpty(); if (first->m_authorStyleSheets.size() != second->m_authorStyleSheets.size()) return false; auto it = second->m_authorStyleSheets.begin(); for (const auto& sheet : first->m_authorStyleSheets) { if (sheet->contents() != (*it++)->contents()) return false; } return true; } We could skip the no resolver, no active sheet equality to make the code simpler without losing much in practice.
LGTM excellent tests https://codereview.chromium.org/2610513003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp (right): https://codereview.chromium.org/2610513003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp:308: if (first->m_authorStyleSheets[firstCount]->contents() != On 2017/01/03 at 23:35:42, rune wrote: > On 2017/01/03 22:35:19, sashab wrote: > > I know this is what the old code did, but is there a builtin for this? If > > m_authorStyleSheets return iterators, maybe there's a C++ builtin we can use. > > > > Just feels like a lot of code for a pretty standard comparator function. Also I > > think we prefer iterator for-loops to while loops. > > I don't think we can compare these vectors with operator== because we need to compare sheet->contents(), not sheet. To make this code simpler to read, I guess you'd need a vector comparator which takes a custom comparator for the elements. I don't know if there's such a thing. > > Here's an alternative: > > bool ScopedStyleResolver::haveSameStyles(const ScopedStyleResolver* first, > const ScopedStyleResolver* second) { > if (!first) > return !second || second->m_authorStyleSheets.isEmpty(); > if (!second) > return first->m_authorStyleSheets.isEmpty(); > if (first->m_authorStyleSheets.size() != second->m_authorStyleSheets.size()) > return false; > auto it = second->m_authorStyleSheets.begin(); > for (const auto& sheet : first->m_authorStyleSheets) { > if (sheet->contents() != (*it++)->contents()) > return false; > } > return true; > } > > > We could skip the no resolver, no active sheet equality to make the code simpler without losing much in practice. Eh, that's not much nicer. Ok, I'm happy with you leaving this as they are haha, thanks for investigating though. https://codereview.chromium.org/2610513003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h (right): https://codereview.chromium.org/2610513003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h:79: CORE_EXPORT static bool haveSameStyles(const ScopedStyleResolver*, I sort of agree with sigbjornf that this could be a bool operator==, but it sounds like you're not actually comparing equality, just that the two styles are equal in the same order. Also either could be null.
Added documentation upon request from sof.
The CQ bit was checked by rune@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org Link to the patchset: https://codereview.chromium.org/2610513003/#ps60001 (title: "Added documentation.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483522901927270,
"parent_rev": "1b2939c1ef6de4981e0e88808b6f4dd156d3e78f", "commit_rev":
"5ec5d5d3a85ffa3f7eec931f9529487139688cb5"}
Message was sent while issue was closed.
Description was changed from ========== Avoid unnecessary updateActiveStyle comparing shadow styles. Found while checking performance for issue 603621. Style sharing is done during style recalc at which point we know that the active style is up-to-date. Instead of using the API for document.styleSheets, compare active stylesheets in ScopedStyleResolver directly. BUG=603621 ========== to ========== Avoid unnecessary updateActiveStyle comparing shadow styles. Found while checking performance for issue 603621. Style sharing is done during style recalc at which point we know that the active style is up-to-date. Instead of using the API for document.styleSheets, compare active stylesheets in ScopedStyleResolver directly. BUG=603621 Review-Url: https://codereview.chromium.org/2610513003 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Avoid unnecessary updateActiveStyle comparing shadow styles. Found while checking performance for issue 603621. Style sharing is done during style recalc at which point we know that the active style is up-to-date. Instead of using the API for document.styleSheets, compare active stylesheets in ScopedStyleResolver directly. BUG=603621 Review-Url: https://codereview.chromium.org/2610513003 ========== to ========== Avoid unnecessary updateActiveStyle comparing shadow styles. Found while checking performance for issue 603621. Style sharing is done during style recalc at which point we know that the active style is up-to-date. Instead of using the API for document.styleSheets, compare active stylesheets in ScopedStyleResolver directly. BUG=603621 Committed: https://crrev.com/f831fc79824c0281cdd4f6a90d7bc92dbadd44b0 Cr-Commit-Position: refs/heads/master@{#441357} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f831fc79824c0281cdd4f6a90d7bc92dbadd44b0 Cr-Commit-Position: refs/heads/master@{#441357}
Message was sent while issue was closed.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Message was sent while issue was closed.
The updateActiveStyle() calls should be free though because nothing is dirty? What's the performance issue here?
Message was sent while issue was closed.
On 2017/01/04 21:51:55, esprehn wrote: > The updateActiveStyle() calls should be free though because nothing is dirty? > What's the performance issue here? This is callgrind output for styleForElement running the 603621 test case before and after the fix: http://imgur.com/gallery/RqVsN updateActiveStyle() looks like this and is not inline: void StyleEngine::updateActiveStyle() { DCHECK(document().isActive()); updateViewport(); updateActiveStyleSheets(); updateGlobalRuleSet(); } The _before_ image is missing updateActiveStyle(), but a lot of time is actually spent on hashtable lookups in styleSheetsForStyleSheetList(). Another reason why this change makes sense is that the active stylesheets are more correct than the stylesheet list, since sheets not matching media queries would still be part of the styleSheetsForStyleSheetList.
Message was sent while issue was closed.
On 2017/01/05 at 22:17:46, rune wrote: > On 2017/01/04 21:51:55, esprehn wrote: > > The updateActiveStyle() calls should be free though because nothing is dirty? > > What's the performance issue here? > > This is callgrind output for styleForElement running the 603621 test case before and after the fix: http://imgur.com/gallery/RqVsN > > updateActiveStyle() looks like this and is not inline: > > void StyleEngine::updateActiveStyle() { > DCHECK(document().isActive()); > updateViewport(); > updateActiveStyleSheets(); > updateGlobalRuleSet(); > } > > The _before_ image is missing updateActiveStyle(), but a lot of time is actually spent on hashtable lookups in styleSheetsForStyleSheetList(). > > Another reason why this change makes sense is that the active stylesheets are more correct than the stylesheet list, since sheets not matching media queries would still be part of the styleSheetsForStyleSheetList. Interesting, I see dirty bit checks at the start of all three of those function calls, perhaps we should have a global dirty bit check at the start of updateActiveStyle() and make it inline.
Message was sent while issue was closed.
On 2017/01/05 22:48:55, esprehn wrote: > On 2017/01/05 at 22:17:46, rune wrote: > > On 2017/01/04 21:51:55, esprehn wrote: > > > The updateActiveStyle() calls should be free though because nothing is > dirty? > > > What's the performance issue here? > > > > This is callgrind output for styleForElement running the 603621 test case > before and after the fix: http://imgur.com/gallery/RqVsN > > > > updateActiveStyle() looks like this and is not inline: > > > > void StyleEngine::updateActiveStyle() { > > DCHECK(document().isActive()); > > updateViewport(); > > updateActiveStyleSheets(); > > updateGlobalRuleSet(); > > } > > > > The _before_ image is missing updateActiveStyle(), but a lot of time is > actually spent on hashtable lookups in styleSheetsForStyleSheetList(). > > > > Another reason why this change makes sense is that the active stylesheets are > more correct than the stylesheet list, since sheets not matching media queries > would still be part of the styleSheetsForStyleSheetList. > > Interesting, I see dirty bit checks at the start of all three of those function > calls, perhaps we should have a global dirty bit check at the start of > updateActiveStyle() and make it inline. Yes. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
