Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2162783002:
Add Memory.{Browser,Renderer}.Large2 metrics (Closed)
DescriptionAdd Memory.{Browser,Renderer}.Large2 metrics
Memory.Browser.Large reports incorrect numbers (it should report in
MB but in KB now). Deprecate it and add a new metric to report
correct numbers. Also deprecate Memory.{Browser,Renderer} because
they are capped at .5GB which isn't enough. Memory.Renderer is
replaced with Memory.Renderer.Large2 ("Large2" is for consistency).
BUG=629354
Committed: https://crrev.com/a5ab9c63598a171e32513ae23df9c034cbd9849b
Cr-Commit-Position: refs/heads/master@{#406245}
Patch Set 1 #
Total comments: 4
Patch Set 2 : comment #Patch Set 3 : Fix test #
Messages
Total messages: 33 (18 generated)
The CQ bit was checked by bashi@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...
bashi@chromium.org changed reviewers: + kouhei@chromium.org
PTAL
non-owner lgtm
bashi@chromium.org changed reviewers: + isherman@chromium.org
isherman@, could you do OWNER's review?
(cc Rob as an FYI) https://codereview.chromium.org/2162783002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2162783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:23322: + </obsolete> If this metric is obsolete, then please remove the code for recording it as well. OTOH, if it's still useful for viewing more fine-grained data, then please do not mark it as obsolete. (And, ditto for the other affected metrics.) https://codereview.chromium.org/2162783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:23333: + Deprecated 07/2016 as it reports wrong numbers. crbug.com/629354 Please indicate what metric replaced this one.
Thanks for quick review! https://codereview.chromium.org/2162783002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2162783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:23322: + </obsolete> On 2016/07/19 01:23:52, Ilya Sherman wrote: > If this metric is obsolete, then please remove the code for recording it as > well. OTOH, if it's still useful for viewing more fine-grained data, then > please do not mark it as obsolete. > > (And, ditto for the other affected metrics.) Done. https://codereview.chromium.org/2162783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:23333: + Deprecated 07/2016 as it reports wrong numbers. crbug.com/629354 On 2016/07/19 01:23:52, Ilya Sherman wrote: > Please indicate what metric replaced this one. Done.
LGTM, thanks.
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/2162783002/#ps20001 (title: "comment")
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by bashi@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by bashi@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.
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/2162783002/#ps40001 (title: "Fix test")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from
==========
Add Memory.{Browser,Renderer}.Large2 metrics
Memory.Browser.Large reports incorrect numbers (it should report in
MB but in KB now). Deprecate it and add a new metric to report
correct numbers. Also deprecate Memory.{Browser,Renderer} because
they are capped at .5GB which isn't enough. Memory.Renderer is
replaced with Memory.Renderer.Large2 ("Large2" is for consistency).
BUG=629354
==========
to
==========
Add Memory.{Browser,Renderer}.Large2 metrics
Memory.Browser.Large reports incorrect numbers (it should report in
MB but in KB now). Deprecate it and add a new metric to report
correct numbers. Also deprecate Memory.{Browser,Renderer} because
they are capped at .5GB which isn't enough. Memory.Renderer is
replaced with Memory.Renderer.Large2 ("Large2" is for consistency).
BUG=629354
Committed: https://crrev.com/a5ab9c63598a171e32513ae23df9c034cbd9849b
Cr-Commit-Position: refs/heads/master@{#406245}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a5ab9c63598a171e32513ae23df9c034cbd9849b Cr-Commit-Position: refs/heads/master@{#406245}
Message was sent while issue was closed.
On 2016/07/19 11:18:32, commit-bot: I haz the power wrote: > Patchset 3 (id:??) landed as > https://crrev.com/a5ab9c63598a171e32513ae23df9c034cbd9849b > Cr-Commit-Position: refs/heads/master@{#406245} Actually, can you create a new CL that un-obsoletes the Memory.Browser and Memory.Renderer metrics? Until the new metrics are on stable for a while, we still want the old metrics so we can access them in the meantime. I would suggest removing the obsolete tags and putting a note in their descriptions that they will be replaced by the new metrics.
Message was sent while issue was closed.
On 2016/07/21 15:52:13, rkaplow wrote: > On 2016/07/19 11:18:32, commit-bot: I haz the power wrote: > > Patchset 3 (id:??) landed as > > https://crrev.com/a5ab9c63598a171e32513ae23df9c034cbd9849b > > Cr-Commit-Position: refs/heads/master@{#406245} > > Actually, can you create a new CL that un-obsoletes the Memory.Browser and > Memory.Renderer metrics? Until the new metrics are on stable for a while, we > still want the old metrics so we can access them in the meantime. > > I would suggest removing the obsolete tags and putting a note in their > descriptions that they will be replaced by the new metrics. I'm happy to un-obsolete these metrics but could you elaborate why do you need it? What does "stable" in this context?
Message was sent while issue was closed.
On 2016/07/21 22:09:00, bashi1 wrote: > On 2016/07/21 15:52:13, rkaplow wrote: > > On 2016/07/19 11:18:32, commit-bot: I haz the power wrote: > > > Patchset 3 (id:??) landed as > > > https://crrev.com/a5ab9c63598a171e32513ae23df9c034cbd9849b > > > Cr-Commit-Position: refs/heads/master@{#406245} > > > > Actually, can you create a new CL that un-obsoletes the Memory.Browser and > > Memory.Renderer metrics? Until the new metrics are on stable for a while, we > > still want the old metrics so we can access them in the meantime. > > > > I would suggest removing the obsolete tags and putting a note in their > > descriptions that they will be replaced by the new metrics. > > I'm happy to un-obsolete these metrics but could you elaborate why do you need > it? What does "stable" in this context? Stable means the stable channel of Chrome :)
Message was sent while issue was closed.
On 2016/07/21 22:27:04, Ilya Sherman wrote: > On 2016/07/21 22:09:00, bashi1 wrote: > > On 2016/07/21 15:52:13, rkaplow wrote: > > > On 2016/07/19 11:18:32, commit-bot: I haz the power wrote: > > > > Patchset 3 (id:??) landed as > > > > https://crrev.com/a5ab9c63598a171e32513ae23df9c034cbd9849b > > > > Cr-Commit-Position: refs/heads/master@{#406245} > > > > > > Actually, can you create a new CL that un-obsoletes the Memory.Browser and > > > Memory.Renderer metrics? Until the new metrics are on stable for a while, we > > > still want the old metrics so we can access them in the meantime. > > > > > > I would suggest removing the obsolete tags and putting a note in their > > > descriptions that they will be replaced by the new metrics. > > > > I'm happy to un-obsolete these metrics but could you elaborate why do you need > > it? What does "stable" in this context? > > Stable means the stable channel of Chrome :) Ah, I see. Thanks :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
