Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2289303004:
WebFonts: measure network loading time (Closed)
|
Created:
4 years, 3 months ago by Takashi Toyoshima Modified:
4 years, 3 months ago CC:
darktears, apavlov+blink_chromium.org, asvitkine+watch_chromium.org, blink-reviews, blink-reviews-css, chromium-reviews, dglazkov+blink, rwlbuis, Shao-Chuan Lee Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebFonts: measure network loading time
We have metrics that measures WebFonts loading time, but now that it
turned out majority in the report comes from caches.
This new metrics variant measure WebFonts loading time only for requests
that actually make network requets. This will contribute to understand
WebFont loading performance distributions.
Committed: https://crrev.com/364799d5bf4e30dcad82acb246163cbec077d8d7
Cr-Commit-Position: refs/heads/master@{#417907}
Patch Set 1 #
Total comments: 2
Patch Set 2 : use histogram_suffixes #
Total comments: 2
Patch Set 3 : enum #Patch Set 4 : minor fix #
Total comments: 12
Patch Set 5 : review #17 #Patch Set 6 : maySet will be better #Patch Set 7 : [rebase] #Patch Set 8 : build fix #
Total comments: 2
Patch Set 9 : review #27 #
Messages
Total messages: 36 (16 generated)
Description was changed from ========== WebFonts: measure network loading time We have metrics that measures WebFonts loading time, but now that it turned out majority in the report comes from caches. This new metrics variant measure WebFonts loading time only for requests that actually make network requets. This will contribute to understand WebFont loading performance distributions. ========== to ========== WebFonts: measure network loading time We have metrics that measures WebFonts loading time, but now that it turned out majority in the report comes from caches. This new metrics variant measure WebFonts loading time only for requests that actually make network requets. This will contribute to understand WebFont loading performance distributions. ==========
toyoshim@chromium.org changed reviewers: + ksakamoto@chromium.org
Sakamoto-san. We used this metrics in the first preliminary survey for the intervention. https://docs.google.com/document/d/1f2tcvW72A9haFkCNsVhKgi1iteNrCRHYA7Y_uIaut... This is useful to understand font loading distributions, but noise from cache is too big for now.
RemoteFontFaceSource lgtm https://codereview.chromium.org/2289303004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2289303004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:65782: +<histogram name="WebFont.DownloadTime.MissedCache.0.Under10KB" units="ms"> You might want to use a <histogram_suffixes>.
https://codereview.chromium.org/2289303004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2289303004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:65782: +<histogram name="WebFont.DownloadTime.MissedCache.0.Under10KB" units="ms"> OK. So to make the suffix rule simple, let me rename them to WebFont.MissedCache.*.
toyoshim@chromium.org changed reviewers: + isherman@chromium.org, kouhei@chromium.org
Requesting OWNERS reviews, +kouhei@ for WebKit/Source/core +isherman@ for tools/metrics
toyoshim@chromium.org changed reviewers: + tkent@chromium.org
+tkent for WebKit since I remember that kouhei takes a day off today.
lgtm
Hmm, do you need both sets of histograms, or could you drop the existing ones in favor of the MissedCache ones?
Old one is still useful to understand cache performance. So, yes both we need both sets.
https://codereview.chromium.org/2289303004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.h (right): https://codereview.chromium.org/2289303004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.h:73: void recordLoadTimeHistogram(const FontResource*, int duration, bool isLoadedFromNetwork); Can we have a enum for describing the source? Looking at the below diff, we already have isLoadedFromMemoryCache which seems to be opposite of this.
PTAL Patch Set 4. https://codereview.chromium.org/2289303004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.h (right): https://codereview.chromium.org/2289303004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.h:73: void recordLoadTimeHistogram(const FontResource*, int duration, bool isLoadedFromNetwork); Actually, it's not opposite of this since we have two other cases. But that means enum is more preferable here. Once concern is that the source couldn't be determined at a single timing, but at multiple events. But I think we can manage it easily, and could reduce risks to make mistakes.
On 2016/09/02 03:16:32, toyoshim wrote: > Old one is still useful to understand cache performance. > So, yes both we need both sets. Okay. Metrics LGTM, then. You might want to think about consolidating these metrics down to a smaller set in the future.
RemoteFontFaceSource was largely modified to address kouhei's point, using enum. ksakamoto@ can you take a look again? If ksakamoto@ is fine with this change, I'll submit this without waiting kouhei's lgtm since he might be busy on biztrip.
lgtm % minor comments. Using enum, the lode look cleaner. https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:48: // Still loading implies coming from network. Having this logic in this conversion function feels a bit weird... Can we just call NOTREACHED() here? https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:72: bool shouldTriggerWebFontsIntervention(Document* document, FontDisplay display, RemoteFontFaceSource::DataSource dataSource) Maybe make this a private member function of RemoteFonTaceSource, so m_dataSource can be accessed directly. https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:179: { How about having if (m_dataSource == FromUnknown) m_dataSource = FromNetwork; here? So you don't need to write the logic of "consider Unknown as FromNetwork" in multiple places. https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:299: cacheHitHistogram.count(cacheHitValue); Call getMetricsValue() here and remove cacheHitValue variable?
https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:48: // Still loading implies coming from network. On 2016/09/06 18:04:53, Kunihiko Sakamoto wrote: > Having this logic in this conversion function feels a bit weird... > Can we just call NOTREACHED() here? Done. https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:72: bool shouldTriggerWebFontsIntervention(Document* document, FontDisplay display, RemoteFontFaceSource::DataSource dataSource) Hum... I'm neutral on this, but I'd rather consider to have dataSource inside FontLoadHistograms since this is a state used for metrics. I didn't do so because dataSource is used outside the inner class, here. But probably I can write it in a reasonable way. https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:179: { Move it inside longLimigExceeded() to have the member inside the inner class. https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:299: cacheHitHistogram.count(cacheHitValue); On 2016/09/06 18:04:53, Kunihiko Sakamoto wrote: > Call getMetricsValue() here and remove cacheHitValue variable? Done. https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:316: DEFINE_STATIC_LOCAL(CustomCountHistogram, missedCacheLoadErrorHistogram, ("WebFont.MissedCache.DownloadTime.LoadError", 0, 10000, 50)); Also, I'd say there is another requirement to have another variant, HitDiskCache. sakamoto-san, could you follow mail thread I forwarded?
The CQ bit was checked by toyoshim@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by toyoshim@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:72: bool shouldTriggerWebFontsIntervention(Document* document, FontDisplay display, RemoteFontFaceSource::DataSource dataSource) On 2016/09/07 08:49:08, toyoshim wrote: > Hum... I'm neutral on this, but I'd rather consider to have dataSource inside > FontLoadHistograms since this is a state used for metrics. I didn't do so > because dataSource is used outside the inner class, here. But probably I can > write it in a reasonable way. ok, sgtm. https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:316: DEFINE_STATIC_LOCAL(CustomCountHistogram, missedCacheLoadErrorHistogram, ("WebFont.MissedCache.DownloadTime.LoadError", 0, 10000, 50)); On 2016/09/07 08:49:08, toyoshim wrote: > Also, I'd say there is another requirement to have another variant, > HitDiskCache. sakamoto-san, could you follow mail thread I forwarded? Yeah I'm following the thread, and I agree we need cache-hit variant. I have some questions: - Should we add separate histogram(s) for DiskCacheHit and MemoryCacheHit? - Breakdown by font size is needed? - Can we deprecate existing cache-unaware histogram in favor of new ones? https://codereview.chromium.org/2289303004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.h (right): https://codereview.chromium.org/2289303004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/RemoteFontFaceSource.h:68: FontLoadHistograms(DataSource) : m_loadStartTime(0), m_blankPaintTime(0), m_isLongLimitExceeded(false) { } m_dataSource isn't initialized?
https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2289303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:316: DEFINE_STATIC_LOCAL(CustomCountHistogram, missedCacheLoadErrorHistogram, ("WebFont.MissedCache.DownloadTime.LoadError", 0, 10000, 50)); Good point. > Should we add separate histogram(s) for DiskCacheHit and MemoryCacheHit? If data comes from memory cache or data URL, we can have data on constructor. That means load time is always 0. So, I think we don't need separate one for MemoryCacheHit. - Breakdown by font size is needed? Probably yes for disk cache. Our current result implies it may have a wide distribution. - Can we deprecate existing cache-unaware histogram in favor of new ones? Agreed. At some point, when we understand cache hit rate and loadtime for each correctly, we won't need this (we can calculate it indirectly) https://codereview.chromium.org/2289303004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.h (right): https://codereview.chromium.org/2289303004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/RemoteFontFaceSource.h:68: FontLoadHistograms(DataSource) : m_loadStartTime(0), m_blankPaintTime(0), m_isLongLimitExceeded(false) { } oops, that's right.
cc: shaochuan@ who may work on adding new variants we discussed here.
The CQ bit was checked by toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, ksakamoto@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2289303004/#ps160001 (title: "review #27")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== WebFonts: measure network loading time We have metrics that measures WebFonts loading time, but now that it turned out majority in the report comes from caches. This new metrics variant measure WebFonts loading time only for requests that actually make network requets. This will contribute to understand WebFont loading performance distributions. ========== to ========== WebFonts: measure network loading time We have metrics that measures WebFonts loading time, but now that it turned out majority in the report comes from caches. This new metrics variant measure WebFonts loading time only for requests that actually make network requets. This will contribute to understand WebFont loading performance distributions. ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== WebFonts: measure network loading time We have metrics that measures WebFonts loading time, but now that it turned out majority in the report comes from caches. This new metrics variant measure WebFonts loading time only for requests that actually make network requets. This will contribute to understand WebFont loading performance distributions. ========== to ========== WebFonts: measure network loading time We have metrics that measures WebFonts loading time, but now that it turned out majority in the report comes from caches. This new metrics variant measure WebFonts loading time only for requests that actually make network requets. This will contribute to understand WebFont loading performance distributions. Committed: https://crrev.com/364799d5bf4e30dcad82acb246163cbec077d8d7 Cr-Commit-Position: refs/heads/master@{#417907} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/364799d5bf4e30dcad82acb246163cbec077d8d7 Cr-Commit-Position: refs/heads/master@{#417907} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
