Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
582253002:
Cast: Reduce the size of stats (Closed)
|
Created:
6 years, 3 months ago by Alpha Left Google Modified:
6 years, 3 months ago CC:
chromium-reviews, hclam+watch_chromium.org, imcheng+watch_chromium.org, hguihot+watch_chromium.org, jasonroberts+watch_google.com, avayvod+watch_chromium.org, pwestin+watch_google.com, feature-media-reviews_chromium.org, miu+watch_chromium.org, hubbe+watch_chromium.org, mikhal+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionCast: Reduce the size of stats
Stats generated by cast streaming API contains some redundancy stuff
like "bucket" and "count".
Also remove 0 buckets in the stats to reduce size.
Committed: https://crrev.com/94f57a1781b0749fdb7884a9b3b7036e58b1908c
Cr-Commit-Position: refs/heads/master@{#296309}
Patch Set 1 #Patch Set 2 : nothing #
Total comments: 1
Patch Set 3 : ordered #Patch Set 4 : more space saving #
Total comments: 8
Patch Set 5 : updated #Patch Set 6 : remove 0 buckets #Messages
Total messages: 25 (5 generated)
hclam@chromium.org changed reviewers: + imcheng@chromium.org
https://codereview.chromium.org/582253002/diff/20001/media/cast/logging/stats... File media/cast/logging/stats_event_subscriber.cc (right): https://codereview.chromium.org/582253002/diff/20001/media/cast/logging/stats... media/cast/logging/stats_event_subscriber.cc:62: scoped_ptr<base::DictionaryValue> The problem with this approach is that the buckets will no longer be in order, since dictionary sorts their entries by key alphabetically. While that might probably be ok when analyzing stats programatically, the histogram will become harder to read for humans.
On 2014/09/18 22:06:20, imcheng1 wrote: > https://codereview.chromium.org/582253002/diff/20001/media/cast/logging/stats... > File media/cast/logging/stats_event_subscriber.cc (right): > > https://codereview.chromium.org/582253002/diff/20001/media/cast/logging/stats... > media/cast/logging/stats_event_subscriber.cc:62: > scoped_ptr<base::DictionaryValue> > The problem with this approach is that the buckets will no longer be in order, > since dictionary sorts their entries by key alphabetically. While that might > probably be ok when analyzing stats programatically, the histogram will become > harder to read for humans. There's a trade off. I think saving 8KB is more important than readability, the data is uploaded and parsed more often then read by a human,
Updated such that result in a list. The size is now 9.2KB.
lgtm Thanks!
The CQ bit was checked by imcheng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/582253002/40001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
hclam@chromium.org changed reviewers: + hubbe@chromium.org
+hubbe for owner approval.
Code updated to compress consecutive 0 count buckets into one. This saves even more.
https://codereview.chromium.org/582253002/diff/60001/media/cast/logging/stats... File media/cast/logging/stats_event_subscriber.cc (right): https://codereview.chromium.org/582253002/diff/60001/media/cast/logging/stats... media/cast/logging/stats_event_subscriber.cc:68: bucket->SetInteger(base::StringPrintf("< %" PRId64, min_), I think you can also get rid of the spaces between the signs and numbers, that would save you even more space and makes parsing easier.
https://codereview.chromium.org/582253002/diff/60001/media/cast/logging/stats... File media/cast/logging/stats_event_subscriber.cc (right): https://codereview.chromium.org/582253002/diff/60001/media/cast/logging/stats... media/cast/logging/stats_event_subscriber.cc:78: if (!buckets_[index]) { nit: prefer explicitly checking != 0. https://codereview.chromium.org/582253002/diff/60001/media/cast/logging/stats... media/cast/logging/stats_event_subscriber.cc:79: while (end_index + 2 < buckets_.size() && !buckets_[end_index + 1]) ditto https://codereview.chromium.org/582253002/diff/60001/media/cast/logging/stats... media/cast/logging/stats_event_subscriber.cc:83: int64 upper = min_ + end_index * width_ - 1; Shouldn't this be min_ + (end_index - index) * width_ - 1; ?
https://codereview.chromium.org/582253002/diff/60001/media/cast/logging/stats... File media/cast/logging/stats_event_subscriber.cc (right): https://codereview.chromium.org/582253002/diff/60001/media/cast/logging/stats... media/cast/logging/stats_event_subscriber.cc:83: int64 upper = min_ + end_index * width_ - 1; On 2014/09/19 20:52:33, imcheng1 wrote: > Shouldn't this be min_ + (end_index - index) * width_ - 1; ? Never mind, I misread this.
https://codereview.chromium.org/582253002/diff/60001/media/cast/logging/stats... File media/cast/logging/stats_event_subscriber.cc (right): https://codereview.chromium.org/582253002/diff/60001/media/cast/logging/stats... media/cast/logging/stats_event_subscriber.cc:68: bucket->SetInteger(base::StringPrintf("< %" PRId64, min_), On 2014/09/19 20:48:14, imcheng1 wrote: > I think you can also get rid of the spaces between the signs and numbers, that > would save you even more space and makes parsing easier. Done. https://codereview.chromium.org/582253002/diff/60001/media/cast/logging/stats... media/cast/logging/stats_event_subscriber.cc:78: if (!buckets_[index]) { On 2014/09/19 20:52:33, imcheng1 wrote: > nit: prefer explicitly checking != 0. Done. https://codereview.chromium.org/582253002/diff/60001/media/cast/logging/stats... media/cast/logging/stats_event_subscriber.cc:79: while (end_index + 2 < buckets_.size() && !buckets_[end_index + 1]) On 2014/09/19 20:52:33, imcheng1 wrote: > ditto Done.
lgtm
Ping hubbe.
Updated code and removed 0 buckets.
The CQ bit was checked by hubbe@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/582253002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as d66e60e380391577b284bbe269fd17dfed1c60da
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/94f57a1781b0749fdb7884a9b3b7036e58b1908c Cr-Commit-Position: refs/heads/master@{#296309} |
