Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1780183003:
ArcLowMemoryKillerMonitor: (Closed)
DescriptionArcLowMemoryKillerMonitor:
Track lowmemorykiller events from kernel and reports to UMA.
It reports:
- Memory released by each kill.
- Time to last kill for knowing how frequently the event occurs.
The strike-up code would be in another CL.
BUG=none
Committed: https://crrev.com/9de1bd84038ad343f49b685e6b65415e9ee69fc2
Cr-Commit-Position: refs/heads/master@{#381510}
Patch Set 1 #
Total comments: 28
Patch Set 2 : y #Patch Set 3 : y #Patch Set 4 : #
Total comments: 12
Patch Set 5 : #
Total comments: 6
Patch Set 6 : histograms.xml #
Total comments: 2
Messages
Total messages: 21 (9 generated)
Description was changed from
==========
ArcLowMemoryKillerMonitor:
Track lowmemorykiller events from kernel and reports to UMA.
It reports:
- Memory released by each kill.
- Time to last kill for knowing how frequently the event occurs.
The strike-up code would be in another CL.
BUG=none
==========
to
==========
ArcLowMemoryKillerMonitor:
Track lowmemorykiller events from kernel and reports to UMA.
It reports:
- Memory released by each kill.
- Time to last kill for knowing how frequently the event occurs.
The strike-up code would be in another CL.
BUG=none
==========
cylee@chromium.org changed reviewers: + yusukes@chromium.org
https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc (right): https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. high level question: what's the point for having this in arc/? I don't see any ARC specific feature in the files. https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:5: #include "chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.h" I believe the general mapping rule for class/file names is from FooBar to foo_bar. Can you rename arc_low_memory_killer_monitor.h/cc to follow that? https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:23: using base::TimeDelta; Can you sort them alphabetically? https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:32: : worker_pool_(new SequencedWorkerPool(1, "arc_lmk_monitor")) {} please spell that out. lmk -> low_memory_killer https://google.github.io/styleguide/cppguide.html#General_Naming_Rules https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:43: base::Bind(&ArcLowMemoryKillerMonitor::Run, base::Unretained(this))); * Is it always safe to use Unretained(this)? What's the lifetime of |this| object? Any reason not to use a weak pointer here? * Instead of having L.53, can't we bind a ref'ed worker_pool_ here and pass it to Run() as a function argument? https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:53: scoped_refptr<SequencedWorkerPool> worker_pool(worker_pool_); Is SequencedWorkerPool derived from RefCountedThreadSafe? ..ah yes it is. https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:57: FILE* kmsg_fd = fopen("/dev/kmsg", "r"); Using the raw stdio library seems error-prone. Can you use base/files/ instead? https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:61: TimeDelta time_delta; Add a variable comment please (to explain what this is for.) I think you can just copy and paste some part of the CL description if you want. https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:64: if (worker_pool->IsShutdownInProgress()) { Is it safe to use worker_pool_ from this thread? https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:69: "lowmemorykiller: .* to free (\\d+)kB", * Please give 1-2 example lines of /dev/kmsg (as a code comment) which the code tries to capture. * Is the unit always 'kB'? https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:86: UMA_HISTOGRAM_LOWMEMORYKILL_TIMES( Shouldn't we move this call to the 'if (fields.size() >= 3) {}' block? Do we want to record TimeDelta::Max()? https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.h (right): https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.h:19: // power cycle of a ChromeOS device, otherwise an lowmemorykiller event would be Chrome OS (here and elsewhere) https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.h:35: DISALLOW_COPY_AND_ASSIGN(ArcLowMemoryKillerMonitor); #include base/macros.h is needed for DISALLOW_COPY_AND_ASSIGN (although sequenced_worker_pool.h does that too as of today.)
https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc (right): https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/03/12 00:24:29, Yusuke Sato wrote: > high level question: what's the point for having this in arc/? I don't see any > ARC specific feature in the files. It's a good question. Though it didn't ARC specific feature, it reports ARC-only metrics (namely ARC memory management like app kills). Also it would only be used by ArcXXXService. So I guess it's fine to land here. https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:5: #include "chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.h" On 2016/03/12 00:24:29, Yusuke Sato wrote: > I believe the general mapping rule for class/file names is from FooBar to > foo_bar. Can you rename arc_low_memory_killer_monitor.h/cc to follow that? Done. https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:23: using base::TimeDelta; On 2016/03/12 00:24:29, Yusuke Sato wrote: > Can you sort them alphabetically? Done. https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:32: : worker_pool_(new SequencedWorkerPool(1, "arc_lmk_monitor")) {} On 2016/03/12 00:24:29, Yusuke Sato wrote: > please spell that out. lmk -> low_memory_killer > > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules Done. https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:43: base::Bind(&ArcLowMemoryKillerMonitor::Run, base::Unretained(this))); On 2016/03/12 00:24:29, Yusuke Sato wrote: > * Is it always safe to use Unretained(this)? What's the lifetime of |this| > object? Any reason not to use a weak pointer here? > * Instead of having L.53, can't we bind a ref'ed worker_pool_ here and pass it > to Run() as a function argument? * The life cycle of |this| is meant to be the same as the main thread (e.g., the same as ChromeBrowserMainPartsChromeos so I thought it's fine). Changed to weak pointer for better safety anyway. * Yes but it adds one more copy when calling Bind, or some other type of overhead. I think it's more light weight this way? https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:53: scoped_refptr<SequencedWorkerPool> worker_pool(worker_pool_); On 2016/03/12 00:24:29, Yusuke Sato wrote: > Is SequencedWorkerPool derived from RefCountedThreadSafe? > > ..ah yes it is. Acknowledged. https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:57: FILE* kmsg_fd = fopen("/dev/kmsg", "r"); On 2016/03/12 00:24:29, Yusuke Sato wrote: > Using the raw stdio library seems error-prone. Can you use base/files/ instead? I didn't see class like FileLineReader in chrome codebase. Is there any library in base/files which let me read in one line at a time? https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:61: TimeDelta time_delta; On 2016/03/12 00:24:29, Yusuke Sato wrote: > Add a variable comment please (to explain what this is for.) I think you can > just copy and paste some part of the CL description if you want. Done. https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:64: if (worker_pool->IsShutdownInProgress()) { On 2016/03/12 00:24:29, Yusuke Sato wrote: > Is it safe to use worker_pool_ from this thread? From the comments of IsShutdownInProcess // Check if Shutdown was called for given threading pool. This method is used // for aborting time consuming operation to avoid blocking shutdown. // // Can be called from any thread. https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq... https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:69: "lowmemorykiller: .* to free (\\d+)kB", On 2016/03/12 00:24:29, Yusuke Sato wrote: > * Please give 1-2 example lines of /dev/kmsg (as a code comment) which the code > tries to capture. > * Is the unit always 'kB'? Done. Yes it's always in kB as reported from kernel. https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:86: UMA_HISTOGRAM_LOWMEMORYKILL_TIMES( On 2016/03/12 00:24:29, Yusuke Sato wrote: > Shouldn't we move this call to the 'if (fields.size() >= 3) {}' block? Do we > want to record TimeDelta::Max()? You're right that I should move it to the if clause. But I may want to record TimeDelta::Max() in case of the first kill (comment added at the variable description), since it gives us a clue that a sequence of kill events occurs (serves as an approximation of #(memory pressure events), where one memory pressure events triggers a sequence of kill events). https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.h (right): https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.h:19: // power cycle of a ChromeOS device, otherwise an lowmemorykiller event would be On 2016/03/12 00:24:29, Yusuke Sato wrote: > Chrome OS (here and elsewhere) Done. https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.h:35: DISALLOW_COPY_AND_ASSIGN(ArcLowMemoryKillerMonitor); On 2016/03/12 00:24:29, Yusuke Sato wrote: > #include base/macros.h is needed for DISALLOW_COPY_AND_ASSIGN (although > sequenced_worker_pool.h does that too as of today.) Done.
https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc (right): https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:43: base::Bind(&ArcLowMemoryKillerMonitor::Run, base::Unretained(this))); > * Yes but it adds one more copy when calling Bind, or some other type of > overhead. I think it's more light weight this way? Copying a smart pointer seems just fine. What I'm saying here is that there might be a race. With the current code, what happens if |this| is destructed after Run() is called and before worker_pool(worker_pool_); is executed? Such a destruction will unlikely happen hopefully, but the code looks a bit scary to me still. WDYT? https://codereview.chromium.org/1780183003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc (right): https://codereview.chromium.org/1780183003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:57: scoped_refptr<SequencedWorkerPool> worker_pool(worker_pool_); please remove this and modify the Bind code if you agree. https://codereview.chromium.org/1780183003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:61: FILE* kmsg_handle = fopen("/dev/kmsg", "r"); If you really want to go with FILE, that's fine, but I think you should still check the current calling thread with ThreadRestrictions::AssertIOAllowed() and handle EINTR properly. In short, instead of the raw fopen, please use OpenFile in base/files/file_util.h at least. https://codereview.chromium.org/1780183003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:72: fcntl(kmsg_fd, F_SETFL, flags | O_NONBLOCK); SetNonBlocking() https://codereview.chromium.org/1780183003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:73: while (fgets(buf, kMaxBufSize, kmsg_handle)) {} The code (setting O_NONBLOCK to the underlying fd, then entering fgets loop) seems a bit complicated, can't we just fseek to the end of the stream? Or, can't we do something like the following? ct = currnettime(); while (fgets()) { timestamp = parseline(); if (timestamp < ct) continue; ... } https://codereview.chromium.org/1780183003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:76: fcntl(kmsg_fd, F_SETFL, flags & ~O_NONBLOCK); same https://codereview.chromium.org/1780183003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:113: fclose(kmsg_handle); CloseFile()
Thanks for reviewing! https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc (right): https://codereview.chromium.org/1780183003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc:43: base::Bind(&ArcLowMemoryKillerMonitor::Run, base::Unretained(this))); On 2016/03/14 18:02:12, Yusuke Sato wrote: > > > * Yes but it adds one more copy when calling Bind, or some other type of > > overhead. I think it's more light weight this way? > > Copying a smart pointer seems just fine. What I'm saying here is that there > might be a race. With the current code, what happens if |this| is destructed > after Run() is called and before worker_pool(worker_pool_); is executed? Such a > destruction will unlikely happen hopefully, but the code looks a bit scary to me > still. WDYT? I see. Changed. https://codereview.chromium.org/1780183003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc (right): https://codereview.chromium.org/1780183003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:57: scoped_refptr<SequencedWorkerPool> worker_pool(worker_pool_); On 2016/03/14 18:02:12, Yusuke Sato wrote: > please remove this and modify the Bind code if you agree. Done. https://codereview.chromium.org/1780183003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:61: FILE* kmsg_handle = fopen("/dev/kmsg", "r"); On 2016/03/14 18:02:12, Yusuke Sato wrote: > If you really want to go with FILE, that's fine, but I think you should still > check the current calling thread with ThreadRestrictions::AssertIOAllowed() and > handle EINTR properly. In short, instead of the raw fopen, please use OpenFile > in base/files/file_util.h at least. Ah sorry I'm not familiar with base/files so missed such a function. I didn't use base::File() because I can't see how to read a line using that interface. https://codereview.chromium.org/1780183003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:72: fcntl(kmsg_fd, F_SETFL, flags | O_NONBLOCK); On 2016/03/14 18:02:12, Yusuke Sato wrote: > SetNonBlocking() Done. https://codereview.chromium.org/1780183003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:73: while (fgets(buf, kMaxBufSize, kmsg_handle)) {} On 2016/03/14 18:02:12, Yusuke Sato wrote: > The code (setting O_NONBLOCK to the underlying fd, then entering fgets loop) > seems a bit complicated, can't we just fseek to the end of the stream? Or, can't > we do something like the following? > > ct = currnettime(); > while (fgets()) { > timestamp = parseline(); > if (timestamp < ct) > continue; > ... > } fseek is better, I missed it. I can't use the time recording method as you described because when logging out Chrome OS may starts a whole new main chrome process, so losses all states. To remember the state the only way is writing the state to disk. https://codereview.chromium.org/1780183003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:76: fcntl(kmsg_fd, F_SETFL, flags & ~O_NONBLOCK); On 2016/03/14 18:02:12, Yusuke Sato wrote: > same Done.
lgtm w/ some comments: https://codereview.chromium.org/1780183003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc (right): https://codereview.chromium.org/1780183003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:10: #include <vector> nit: space between line 9 and 10 https://codereview.chromium.org/1780183003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:81: LOG(WARNING) << "Chrome is shutting down, exit now."; DLOG(INFO), probably? https://codereview.chromium.org/1780183003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.h (right): https://codereview.chromium.org/1780183003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.h:36: void Run(scoped_refptr<base::SequencedWorkerPool> worker_pool); Can this function be static now? If so, I think you can remove line 10 and 40-42.
cylee@chromium.org changed reviewers: + rkaplow@chromium.org
add rkaplow@ for historgrams.xml https://codereview.chromium.org/1780183003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc (right): https://codereview.chromium.org/1780183003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:113: fclose(kmsg_handle); On 2016/03/14 18:02:12, Yusuke Sato wrote: > CloseFile() Done. https://codereview.chromium.org/1780183003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc (right): https://codereview.chromium.org/1780183003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:10: #include <vector> On 2016/03/15 16:59:45, Yusuke Sato wrote: > nit: space between line 9 and 10 Done. https://codereview.chromium.org/1780183003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.cc:81: LOG(WARNING) << "Chrome is shutting down, exit now."; On 2016/03/15 16:59:45, Yusuke Sato wrote: > DLOG(INFO), probably? DVLOG Added https://codereview.chromium.org/1780183003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.h (right): https://codereview.chromium.org/1780183003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_low_memory_killer_monitor.h:36: void Run(scoped_refptr<base::SequencedWorkerPool> worker_pool); On 2016/03/15 16:59:45, Yusuke Sato wrote: > Can this function be static now? If so, I think you can remove line 10 and > 40-42. True. Thanks !
lgtm
The CQ bit was checked by cylee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/1780183003/#ps100001 (title: "histograms.xml")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1780183003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1780183003/100001
Message was sent while issue was closed.
Description was changed from
==========
ArcLowMemoryKillerMonitor:
Track lowmemorykiller events from kernel and reports to UMA.
It reports:
- Memory released by each kill.
- Time to last kill for knowing how frequently the event occurs.
The strike-up code would be in another CL.
BUG=none
==========
to
==========
ArcLowMemoryKillerMonitor:
Track lowmemorykiller events from kernel and reports to UMA.
It reports:
- Memory released by each kill.
- Time to last kill for knowing how frequently the event occurs.
The strike-up code would be in another CL.
BUG=none
==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from
==========
ArcLowMemoryKillerMonitor:
Track lowmemorykiller events from kernel and reports to UMA.
It reports:
- Memory released by each kill.
- Time to last kill for knowing how frequently the event occurs.
The strike-up code would be in another CL.
BUG=none
==========
to
==========
ArcLowMemoryKillerMonitor:
Track lowmemorykiller events from kernel and reports to UMA.
It reports:
- Memory released by each kill.
- Time to last kill for knowing how frequently the event occurs.
The strike-up code would be in another CL.
BUG=none
Committed: https://crrev.com/9de1bd84038ad343f49b685e6b65415e9ee69fc2
Cr-Commit-Position: refs/heads/master@{#381510}
==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9de1bd84038ad343f49b685e6b65415e9ee69fc2 Cr-Commit-Position: refs/heads/master@{#381510}
Message was sent while issue was closed.
elijahtaylor@chromium.org changed reviewers: + elijahtaylor@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1780183003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1780183003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:793: +<histogram name="ArcRuntime.LowMemoryKiller.TimeDelta" units="ms"> as we discussed offline, please rename these to "Arc.*" to match other metrics inside of chrome
Message was sent while issue was closed.
cylee@google.com changed reviewers: + cylee@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/1780183003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1780183003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:793: +<histogram name="ArcRuntime.LowMemoryKiller.TimeDelta" units="ms"> On 2016/03/22 22:18:58, elijahtaylor1 wrote: > as we discussed offline, please rename these to "Arc.*" to match other metrics > inside of chrome This CL has been submitted. Another CL which renames the metrics is under review https://codereview.chromium.org/1828533002/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
