Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2533093002:
[LazyParseCSS] Add histogram for rule usage % (Closed)
|
Created:
4 years ago by Charlie Harrison Modified:
4 years ago CC:
chromium-reviews, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, asvitkine+watch_chromium.org, darktears, blink-reviews, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[LazyParseCSS] Add histogram for rule usage %
This patch adds a histogram which tracks a how many style sheets reach
certain rule usage milestones (i.e. > 10% rules used).
This is useful for determining how worthwhile lazy parsing is in the wild.
BUG=642722
Committed: https://crrev.com/1f7a3938ff76d3db59e3f54159bd25a153832357
Cr-Commit-Position: refs/heads/master@{#436279}
Patch Set 1 #Patch Set 2 : clear up histogram description (trybots prev) #
Total comments: 6
Patch Set 3 : timloh review + add test #
Total comments: 9
Patch Set 4 : isherman review #Patch Set 5 : rebase #
Messages
Total messages: 37 (21 generated)
The CQ bit was checked by csharrison@chromium.org 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...
csharrison@chromium.org changed reviewers: + timloh@chromium.org
timloh, would you take a quick look at this? I originally wanted to do a single calculation at a certain point in time, but this ended up being a bit simpler and covers more cases than an arbitrary stopping point (e.g. onload). WDYT?
lgtm https://codereview.chromium.org/2533093002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp (right): https://codereview.chromium.org/2533093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp:40: // Minor hack, as grabbing the context is done once per deferred property set. Can't we just add a countRuleParsed() function or something like that? https://codereview.chromium.org/2533093002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h (right): https://codereview.chromium.org/2533093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h:11: #include "core/css/parser/CSSParserTokenRange.h" Probably don't need this include? https://codereview.chromium.org/2533093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h:54: int m_parsedProperties; Properties -> StyleRules
The CQ bit was checked by csharrison@chromium.org 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...
csharrison@chromium.org changed reviewers: + isherman@chromium.org
Thanks! I decided to write a unit test because this functionality is nontrivial. This led me to the rewrite the logic a bit so that multiple buckets can be counted at one time if the style sheet is small (which is it in tests). This entailed substituting the if statement with a while loop. +isherman for histograms.xml https://codereview.chromium.org/2533093002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp (right): https://codereview.chromium.org/2533093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp:40: // Minor hack, as grabbing the context is done once per deferred property set. On 2016/11/30 14:55:22, Timothy Loh wrote: > Can't we just add a countRuleParsed() function or something like that? Done. https://codereview.chromium.org/2533093002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h (right): https://codereview.chromium.org/2533093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h:11: #include "core/css/parser/CSSParserTokenRange.h" On 2016/11/30 14:55:22, Timothy Loh wrote: > Probably don't need this include? It is needed for the createLazyParser method. I've refactored that to take a const ref, so the fwd include should suffice. https://codereview.chromium.org/2533093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h:54: int m_parsedProperties; On 2016/11/30 14:55:22, Timothy Loh wrote: > Properties -> StyleRules Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2533093002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp (right): https://codereview.chromium.org/2533093002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp:81: ("Style.LazyUsage.Percent", UsageAll)); This should be UsageAll + 1, i.e. enumerated histograms expect a count of the number of buckets, not the index of the last bucket. https://codereview.chromium.org/2533093002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h (right): https://codereview.chromium.org/2533093002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h:42: // Exposed for tests. Please document that this enum is used to back an UMA histogram, and should therefore be treated as append-only. https://codereview.chromium.org/2533093002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2533093002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:63318: + stylesheet which uses all of its rules will log counts in every bucket. Hmm, why structure this to record to every bucket? It seems like interpreting the data would be easier if each parse recorded to just a single bucket. https://codereview.chromium.org/2533093002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:63318: + stylesheet which uses all of its rules will log counts in every bucket. Please document precisely when this metric is recorded -- when the page is fully parsed?
The CQ bit was checked by csharrison@chromium.org 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...
Thanks Ilya! https://codereview.chromium.org/2533093002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp (right): https://codereview.chromium.org/2533093002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp:81: ("Style.LazyUsage.Percent", UsageAll)); On 2016/12/01 00:48:21, Ilya Sherman wrote: > This should be UsageAll + 1, i.e. enumerated histograms expect a count of the > number of buckets, not the index of the last bucket. Oops you're right. I added another LastValue enum value and added it here. https://codereview.chromium.org/2533093002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h (right): https://codereview.chromium.org/2533093002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h:42: // Exposed for tests. On 2016/12/01 00:48:21, Ilya Sherman wrote: > Please document that this enum is used to back an UMA histogram, and should > therefore be treated as append-only. Done. https://codereview.chromium.org/2533093002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2533093002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:63318: + stylesheet which uses all of its rules will log counts in every bucket. On 2016/12/01 00:48:21, Ilya Sherman wrote: > Please document precisely when this metric is recorded -- when the page is fully > parsed? Done. We log as the rules are parsed. https://codereview.chromium.org/2533093002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:63318: + stylesheet which uses all of its rules will log counts in every bucket. On 2016/12/01 00:48:21, Ilya Sherman wrote: > Hmm, why structure this to record to every bucket? It seems like interpreting > the data would be easier if each parse recorded to just a single bucket. Yeah this would be ideal, but it is very hard to determine when to actually record the metric. Ideally we would do this in the destructor, but fast shutdown makes this very unreliable. For a bit of context: we share StyleSheetContexts, often between Documents, so if we record based on Document lifecycle events we'll sometimes get inaccurate data.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
metrics lgtm https://codereview.chromium.org/2533093002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2533093002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:63318: + stylesheet which uses all of its rules will log counts in every bucket. On 2016/12/01 02:43:51, Charlie Harrison wrote: > On 2016/12/01 00:48:21, Ilya Sherman wrote: > > Hmm, why structure this to record to every bucket? It seems like interpreting > > the data would be easier if each parse recorded to just a single bucket. > > Yeah this would be ideal, but it is very hard to determine when to actually > record the metric. Ideally we would do this in the destructor, but fast shutdown > makes this very unreliable. > > For a bit of context: we share StyleSheetContexts, often between Documents, so > if we record based on Document lifecycle events we'll sometimes get inaccurate > data. Ah, okay, that makes sense. Fair enough -- thanks!
Thanks, Timothy would you mind doing one more pass now that the code has changed a bit since your l-g?
On 2016/12/01 14:14:05, Charlie Harrison wrote: > Thanks, Timothy would you mind doing one more pass now that the code has changed > a bit since your l-g? Still looks good.
Thank you, Iet me send to CQ.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org Link to the patchset: https://codereview.chromium.org/2533093002/#ps60001 (title: "isherman review")
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
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2533093002/#ps80001 (title: "rebase")
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by csharrison@chromium.org
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": 80001, "attempt_start_ts": 1480942768727210,
"parent_rev": "4b084d0822b7f4b98628c7bfe16728f33a1637da", "commit_rev":
"61ef2c572953a9a4c2cdc8304ed2ec9e7bf6e426"}
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [LazyParseCSS] Add histogram for rule usage % This patch adds a histogram which tracks a how many style sheets reach certain rule usage milestones (i.e. > 10% rules used). This is useful for determining how worthwhile lazy parsing is in the wild. BUG=642722 ========== to ========== [LazyParseCSS] Add histogram for rule usage % This patch adds a histogram which tracks a how many style sheets reach certain rule usage milestones (i.e. > 10% rules used). This is useful for determining how worthwhile lazy parsing is in the wild. BUG=642722 Committed: https://crrev.com/1f7a3938ff76d3db59e3f54159bd25a153832357 Cr-Commit-Position: refs/heads/master@{#436279} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1f7a3938ff76d3db59e3f54159bd25a153832357 Cr-Commit-Position: refs/heads/master@{#436279} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
