Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2540763002:
Notifications: Split up image loading histograms by image type (Closed)
DescriptionNotifications: Split up image loading histograms by image type
It'll be useful to compare timings and byte sizes for the
different image types (content image, icon, badge, action icon).
BUG=669621, 614456
Committed: https://crrev.com/468ed78ec76f5db7d89a0829b33c0f6427553411
Cr-Commit-Position: refs/heads/master@{#435601}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Address review comment & fix NotificationImageLoaderTest #Patch Set 3 : NOTIFICATION_HISTOGRAM_COUNTS #Patch Set 4 : git cl format #
Total comments: 6
Patch Set 5 : Add test #
Total comments: 3
Patch Set 6 : Use histogram_suffixes #
Messages
Total messages: 30 (17 generated)
johnme@chromium.org changed reviewers: + peter@chromium.org
https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp (right): https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:29: 1 /* min */, max, 50 /* buckets */)); \ nit: fix alignment https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:39: DEFINE_SINGLE_TYPE_COUNT_HISTOGRAM(metric, ActionIcon, value, max); \ No semi-colons after these lines. (You wouldn't add them after a closing bracket either.) https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:63: maxHeightPx = kWebNotificationMaxImageHeightPx; You're going to want some |break| statements in this switch. https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:73: } DCHECK_NE on maxWidthPx/maxHeightPx not being -1 https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:74: // TODO(mvanouwerkerk): Explore doing the scaling on a background thread. s/mvanouwerkerk/another name/ https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:87: return scaledImage; nit: std::move(scaledImage) to prevent silly compilers from making a copy? (Even though it's just ref churn...) https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:150: 1000 * 60 * 60 /* 1 hour max */); Please find a better name for DEFINE_COUNT_HISTOGRAM. It doesn't just define the histogram, but it also counts the value. https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h (right): https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h:30: // Type names and values are used in UMAs, so do not rename/renumber. nit: s/renumber/reorder/ (since the numbering is implicit) https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h:37: NotificationImageLoader(Type); explicit https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h:40: // Scales down |image| to fit within |maxWidthPx|x|maxHeightPx| if it's larger nit: update comment. Neither |maxWidthPx| nor |maxHeightPx| are passed as arguments anymore.
Description was changed from ========== Notifications: Split up image loading histograms by image type BUG=614456 ========== to ========== Notifications: Split up image loading histograms by image type BUG=669621,614456 ==========
Description was changed from ========== Notifications: Split up image loading histograms by image type BUG=669621,614456 ========== to ========== Notifications: Split up image loading histograms by image type It'll be useful to compare timings and byte sizes for the different image types (content image, icon, badge, action icon). BUG=669621,614456 ==========
https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp (right): https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:29: 1 /* min */, max, 50 /* buckets */)); \ On 2016/11/29 19:01:40, Peter Beverloo wrote: > nit: fix alignment Done. https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:39: DEFINE_SINGLE_TYPE_COUNT_HISTOGRAM(metric, ActionIcon, value, max); \ On 2016/11/29 19:01:40, Peter Beverloo wrote: > No semi-colons after these lines. (You wouldn't add them after a closing bracket > either.) Done. https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:63: maxHeightPx = kWebNotificationMaxImageHeightPx; On 2016/11/29 19:01:40, Peter Beverloo wrote: > You're going to want some |break| statements in this switch. Done :) https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:73: } On 2016/11/29 19:01:40, Peter Beverloo wrote: > DCHECK_NE on maxWidthPx/maxHeightPx not being -1 Done (DCHECK_GT 0). https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:74: // TODO(mvanouwerkerk): Explore doing the scaling on a background thread. On 2016/11/29 19:01:40, Peter Beverloo wrote: > s/mvanouwerkerk/another name/ Done. https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:87: return scaledImage; On 2016/11/29 19:01:40, Peter Beverloo wrote: > nit: std::move(scaledImage) to prevent silly compilers from making a copy? (Even > though it's just ref churn...) error: moving a local object in a return statement prevents copy elision [-Werror,-Wpessimizing-move] https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:150: 1000 * 60 * 60 /* 1 hour max */); On 2016/11/29 19:01:40, Peter Beverloo wrote: > Please find a better name for DEFINE_COUNT_HISTOGRAM. It doesn't just define the > histogram, but it also counts the value. Done (NOTIFICATION_HISTOGRAM_COUNTS, by analogy to UMA_HISTOGRAM_CUSTOM_COUNTS in chromium, which also both defines a static local and does the counting). https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h (right): https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h:30: // Type names and values are used in UMAs, so do not rename/renumber. On 2016/11/29 19:01:40, Peter Beverloo wrote: > nit: s/renumber/reorder/ (since the numbering is implicit) Done (actually, numbering isn't used for UMA, so simplified this comment to only mention renaming). https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h:37: NotificationImageLoader(Type); On 2016/11/29 19:01:40, Peter Beverloo wrote: > explicit Done. https://codereview.chromium.org/2540763002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h:40: // Scales down |image| to fit within |maxWidthPx|x|maxHeightPx| if it's larger On 2016/11/29 19:01:40, Peter Beverloo wrote: > nit: update comment. Neither |maxWidthPx| nor |maxHeightPx| are passed as > arguments anymore. Done.
johnme@chromium.org changed reviewers: + isherman@chromium.org
isherman: please review histograms - thanks :)
The CQ bit was checked by johnme@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm https://codereview.chromium.org/2540763002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp (right): https://codereview.chromium.org/2540763002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:24: case NotificationImageLoader::Type::type_name: { \ It's technically not needed to quality NotificationImageLoader, but OK to keep it since the macro's definition is outside the class' scope.. https://codereview.chromium.org/2540763002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:78: DCHECK_GT(maxHeightPx, 0); micro nit: I have a very mild preference for initializing the variables on line 58 to "0". There's a bit of an inconsistency now.. Won't block on that. https://codereview.chromium.org/2540763002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/notifications/NotificationImageLoaderTest.cpp (right): https://codereview.chromium.org/2540763002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/notifications/NotificationImageLoaderTest.cpp:40: new NotificationImageLoader(NotificationImageLoader::Type::Icon)) {} ISTM that testing that the right histogram is used is a fair use-case. Note that we have a blink::HistogramTester now.
Addressed review comments. https://codereview.chromium.org/2540763002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp (right): https://codereview.chromium.org/2540763002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:24: case NotificationImageLoader::Type::type_name: { \ On 2016/11/30 17:44:02, Peter Beverloo wrote: > It's technically not needed to quality NotificationImageLoader, but OK to keep > it since the macro's definition is outside the class' scope.. Acknowledged. https://codereview.chromium.org/2540763002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:78: DCHECK_GT(maxHeightPx, 0); On 2016/11/30 17:44:02, Peter Beverloo wrote: > micro nit: I have a very mild preference for initializing the variables on line > 58 to "0". There's a bit of an inconsistency now.. Won't block on that. Done. https://codereview.chromium.org/2540763002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/notifications/NotificationImageLoaderTest.cpp (right): https://codereview.chromium.org/2540763002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/notifications/NotificationImageLoaderTest.cpp:40: new NotificationImageLoader(NotificationImageLoader::Type::Icon)) {} On 2016/11/30 17:44:02, Peter Beverloo wrote: > ISTM that testing that the right histogram is used is a fair use-case. Note that > we have a blink::HistogramTester now. Done.
The CQ bit was checked by johnme@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.
metrics LGTM % a comment: https://codereview.chromium.org/2540763002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2540763002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39210: +</histogram> Could you please use histogram_suffixes to reduce the amount of repetition from these histograms?
https://codereview.chromium.org/2540763002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2540763002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39210: +</histogram> On 2016/12/01 01:00:45, Ilya Sherman wrote: > Could you please use histogram_suffixes to reduce the amount of repetition from > these histograms? Done, though a downside is that histogram_suffixes also generates unsuffixed histograms, so there are now histograms called Notifications.LoadFailTime etc that will never have data uploaded to them. This seems to affect many other users of histogram suffixes from cursory inspection of the UMA dashboard.
The CQ bit was checked by johnme@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2540763002/#ps100001 (title: "Use histogram_suffixes")
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": 100001, "attempt_start_ts": 1480593236615200,
"parent_rev": "254391c9e6ec119955601265cac51a8c4a70c5ca", "commit_rev":
"41c3288c43444d2ffa0ef55c56f39840613dfb5c"}
Message was sent while issue was closed.
Description was changed from ========== Notifications: Split up image loading histograms by image type It'll be useful to compare timings and byte sizes for the different image types (content image, icon, badge, action icon). BUG=669621,614456 ========== to ========== Notifications: Split up image loading histograms by image type It'll be useful to compare timings and byte sizes for the different image types (content image, icon, badge, action icon). BUG=669621,614456 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Notifications: Split up image loading histograms by image type It'll be useful to compare timings and byte sizes for the different image types (content image, icon, badge, action icon). BUG=669621,614456 ========== to ========== Notifications: Split up image loading histograms by image type It'll be useful to compare timings and byte sizes for the different image types (content image, icon, badge, action icon). BUG=669621,614456 Committed: https://crrev.com/468ed78ec76f5db7d89a0829b33c0f6427553411 Cr-Commit-Position: refs/heads/master@{#435601} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/468ed78ec76f5db7d89a0829b33c0f6427553411 Cr-Commit-Position: refs/heads/master@{#435601}
Message was sent while issue was closed.
On 2016/12/01 at 14:03:44, commit-bot wrote: > Patchset 6 (id:??) landed as https://crrev.com/468ed78ec76f5db7d89a0829b33c0f6427553411 > Cr-Commit-Position: refs/heads/master@{#435601} Manual revert in flight: https://codereview.chromium.org/2540423003 Failing on bots. NotificationImageLoaderTest.SuccessTest (run #1): [ RUN ] NotificationImageLoaderTest.SuccessTest ../../base/test/histogram_tester.cc:158: Failure Value of: actual_count Actual: 1 Expected: expected_count Which is: 0 Histogram "Notifications.LoadFinishTime.Icon" does not have the right number of samples (0) in the expected bucket (0). It has (1). [ FAILED ] NotificationImageLoaderTest.SuccessTest (5 ms) https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty...
Message was sent while issue was closed.
https://codereview.chromium.org/2540763002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2540763002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39210: +</histogram> On 2016/12/01 11:53:49, johnme wrote: > On 2016/12/01 01:00:45, Ilya Sherman wrote: > > Could you please use histogram_suffixes to reduce the amount of repetition > from > > these histograms? > > Done, though a downside is that histogram_suffixes also generates unsuffixed > histograms, so there are now histograms called Notifications.LoadFailTime etc > that will never have data uploaded to them. This seems to affect many other > users of histogram suffixes from cursory inspection of the UMA dashboard. Ah, to work around this you can add an attribute base='true'. It's a recently added trick =) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
