Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2775983003:
[Chromoting] Retrieve process resource usage (ProcessStats) and its tests (Closed)
Description[Chromoting] Retrieve process resource usage (ProcessStats) and its tests
ProcessStatsStub is an interface to receive process resource usage information
from a ProcessStats.
ProcessResourceUsage is a structure to store the resource usage of one process.
AggregatedProcessResourceUsage is a structure to store the resource usgaes of
several processes.
ProcessStatsAgent is an interface to retrieve resource usage of one process.
CurrentProcessStatsAgent is a ProcessStatsAgent implementation to retrieve
resource usage from current process.
ForwardProcessStatsAgent receives resource usage from a third-party and forwards
the latest usage as a ProcessStatsAgent.
ProcessStatsSender is a class to regularly report resource usages from various
ProcessStatsAgent implementations to a ProcessStatsStub. It merges several
non-empty ProcessResourceUsage into one AggregatedProcessResourceUsage.
This is typical useful on Windows: we need to calculate the resource usages from
both network process and desktop process.
R=sergeyu@chromium.org, jamiewalch@chromium.org
BUG=650926
Review-Url: https://codereview.chromium.org/2775983003
Cr-Original-Commit-Position: refs/heads/master@{#467753}
Committed: https://chromium.googlesource.com/chromium/src/+/5928f440f84065ab45f848dcd135e3c2f52ca5bb
Review-Url: https://codereview.chromium.org/2775983003
Cr-Commit-Position: refs/heads/master@{#467844}
Committed: https://chromium.googlesource.com/chromium/src/+/4f3f095f04100e1e443cfdcba1813bdaa625bc77
Patch Set 1 #
Total comments: 16
Patch Set 2 : Resolve review comments #
Total comments: 28
Patch Set 3 : Resolve review comments #
Total comments: 4
Patch Set 4 : Resolve review comments #
Total comments: 58
Patch Set 5 : Resolve review comments #
Total comments: 4
Patch Set 6 : Resolve review comments #Patch Set 7 : Update the test case #
Total comments: 28
Patch Set 8 : Resolve review comments #
Total comments: 2
Patch Set 9 : Resolve review comments #Patch Set 10 : thread_checker_ should be correctly constructed #
Messages
Total messages: 97 (70 generated)
The CQ bit was checked by zijiehe@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/2775983003/diff/1/remoting/base/process_resou... File remoting/base/process_resource_usage.cc (right): https://codereview.chromium.org/2775983003/diff/1/remoting/base/process_resou... remoting/base/process_resource_usage.cc:10: processor_usage += other.processor_usage; I think it would be useful to collect info for each process individually and it shouldn't be hard to implement here. https://codereview.chromium.org/2775983003/diff/1/remoting/base/process_resou... File remoting/base/process_resource_usage.h (right): https://codereview.chromium.org/2775983003/diff/1/remoting/base/process_resou... remoting/base/process_resource_usage.h:19: size_t working_set_size = 0; Is it in bytes? https://codereview.chromium.org/2775983003/diff/1/remoting/base/process_resou... remoting/base/process_resource_usage.h:25: void Append(const ProcessResourceUsage& other); Call it Add(), since the order is irrelevant. https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats.cc File remoting/host/host_stats.cc (right): https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats.cc... remoting/host/host_stats.cc:28: base::MessageLoop::current()->task_runner()->PostDelayedTask(FROM_HERE, I think it would be better to use base::RepeatingTimer here. Otherwise please use ThreadTaskRunnerHandle::Get() instead of base::MessageLoop::current()->task_runner() https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats.cc... remoting/host/host_stats.cc:39: base::AutoLock lock(lock_); Why do we need lock? https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats.h File remoting/host/host_stats.h (right): https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats.h#... remoting/host/host_stats.h:25: // to accumulate the total resource usage of several processes. I don't think that chaining is the best approach here. It's not intuitive and now scheduling and multiplexing logic is mixed with the core. What happens if interval passed to these objects is different. It also makes destruction harder. How about a simple class that holds multiple HostStats sources and multiplexes the stats it gets from them. I would also prefer that object to be responsible for scheduling. https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats.h#... remoting/host/host_stats.h:26: class HostStats final : public HostStatsStub { HostStatsSender or maybe HostStatsAgenet https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats_st... File remoting/host/host_stats_stub.h (right): https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats_st... remoting/host/host_stats_stub.h:17: ~HostStatsStub() = default; destructor must be virtual in interfaces
Description was changed from ========== [Chromoting] Retrieve host process resource usage (HostStats) and its tests ProcessResourceUsage is a trivial structure to store various resource usage of a process. HostStatsStub is an interface to receive host process resource usage information from a HostStats. HostStats is a class to regularly report ProcessResourceUsage of current process to a HostStatsStub. It also implements HostStatsStub, so we can accumulate resource usages from multiple processes and send to client side for logging. This is typical useful on Windows: we need to calculate the resource usages from both network process and desktop process. R=sergeyu@chromium.org, jamiewalch@chromium.org BUG=650926 ========== to ========== [Chromoting] Retrieve process resource usage (ProcessStats) and its tests ProcessStatsStub is an interface to receive process resource usage information from a ProcessStats. ProcessResourceUsage is a structure to store the resource usage of one process. AggregatedProcessResourceUsage is a structure to store the resource usgaes of several processes. ProcessStatsAgent is an interface to retrieve resource usage of one process. CurrentProcessStatsAgent is a ProcessStatsAgent implementation to retrieve resource usage from current process. ForwardProcessStatsAgent receives resource usage from a third-party and forwards the latest usage as a ProcessStatsAgent. ProcessStats is a class to regularly report resource usages from various ProcessStatsAgent implementations to a ProcessStatsStub. It merges several non-empty ProcessResourceUsage into one AggregatedProcessResourceUsage. This is typical useful on Windows: we need to calculate the resource usages from both network process and desktop process. R=sergeyu@chromium.org, jamiewalch@chromium.org BUG=650926 ==========
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
The CQ bit was checked by zijiehe@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 checked by zijiehe@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@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...
Patchset #2 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #2 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2775983003/diff/1/remoting/base/process_resou... File remoting/base/process_resource_usage.cc (right): https://codereview.chromium.org/2775983003/diff/1/remoting/base/process_resou... remoting/base/process_resource_usage.cc:10: processor_usage += other.processor_usage; On 2017/03/27 19:43:19, Sergey Ulanov wrote: > I think it would be useful to collect info for each process individually and it > shouldn't be hard to implement here. Done. https://codereview.chromium.org/2775983003/diff/1/remoting/base/process_resou... File remoting/base/process_resource_usage.h (right): https://codereview.chromium.org/2775983003/diff/1/remoting/base/process_resou... remoting/base/process_resource_usage.h:19: size_t working_set_size = 0; On 2017/03/27 19:43:20, Sergey Ulanov wrote: > Is it in bytes? Yes. https://codereview.chromium.org/2775983003/diff/1/remoting/base/process_resou... remoting/base/process_resource_usage.h:25: void Append(const ProcessResourceUsage& other); On 2017/03/27 19:43:19, Sergey Ulanov wrote: > Call it Add(), since the order is irrelevant. Done. https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats.cc File remoting/host/host_stats.cc (right): https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats.cc... remoting/host/host_stats.cc:28: base::MessageLoop::current()->task_runner()->PostDelayedTask(FROM_HERE, On 2017/03/27 19:43:20, Sergey Ulanov wrote: > I think it would be better to use base::RepeatingTimer here. > > Otherwise please use ThreadTaskRunnerHandle::Get() instead of > base::MessageLoop::current()->task_runner() Done. https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats.cc... remoting/host/host_stats.cc:39: base::AutoLock lock(lock_); On 2017/03/27 19:43:20, Sergey Ulanov wrote: > Why do we need lock? I suppose HostStats::OnHostStats() should be called by DesktopSessionProxy::OnMessageReceived(), which is in network thread. But this class should be generated in ClientSession::OnConnectionAuthenticated(), which I have not found a very clear information which thread it belongs to. So they may belong to different threads. Posting task seems over-kill to me, considering it copies only several small fields. If we can confirm the ClientSession::OnConnectionAuthenticated() is also executed in network thread. We can also replace this lock with a thread-checker. https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats.h File remoting/host/host_stats.h (right): https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats.h#... remoting/host/host_stats.h:25: // to accumulate the total resource usage of several processes. On 2017/03/27 19:43:20, Sergey Ulanov wrote: > I don't think that chaining is the best approach here. It's not intuitive and > now scheduling and multiplexing logic is mixed with the core. What happens if > interval passed to these objects is different. It also makes destruction harder. > How about a simple class that holds multiple HostStats sources and multiplexes > the stats it gets from them. I would also prefer that object to be responsible > for scheduling. I have no objection on holding several HostStats sources in a different class. But since the process metrics of another process are always coming from IPC channel, we always cannot assume the intervals of two HostStats are the same. So the received_usage_ is always the latest states. https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats.h#... remoting/host/host_stats.h:26: class HostStats final : public HostStatsStub { On 2017/03/27 19:43:20, Sergey Ulanov wrote: > HostStatsSender or maybe HostStatsAgenet Done. https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats_st... File remoting/host/host_stats_stub.h (right): https://codereview.chromium.org/2775983003/diff/1/remoting/host/host_stats_st... remoting/host/host_stats_stub.h:17: ~HostStatsStub() = default; On 2017/03/27 19:43:20, Sergey Ulanov wrote: > destructor must be virtual in interfaces Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
https://codereview.chromium.org/2775983003/diff/80001/remoting/base/BUILD.gn File remoting/base/BUILD.gn (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/BUILD.gn#... remoting/base/BUILD.gn:55: if (!is_nacl) { In most other places we put everything in sources and then conditionally remove it. There is already if (is_nacl) block below. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/BUILD.gn#... remoting/base/BUILD.gn:56: # There is no ProcessMetrics implementation on NACL. It's not implemented for Android either. Do we need any of this stuff outside of remoting/host? If not, maybe move it to host directory? https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... File remoting/base/process_stats.cc (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats.cc:9: #include "remoting/base/process_stats.h" This should be the first include in this file. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats.cc:43: protocol::ProcessResourceUsage usage = agent->GetResourceUsage(); Can this operation block? Would it be better to run it on a background thread? https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... File remoting/base/process_stats.h (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats.h:15: #include "remoting/proto/process_stats.pb.h" base shouldn't depend on proto. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats.h:20: class ProcessStats final { Maybe ProcessStatsSender? https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats.h:23: // be outlived this instance. s/It needs to be outlived this instance./It must outlive the ProcessStats object./ https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats.h:35: void Stop(); Do we need this methods? Would it be better just destroy this object? https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... File remoting/base/process_stats_agent.cc (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats_agent.cc:10: ProcessStatsAgent::~ProcessStatsAgent() = default; ProcessStatsAgent is an abstract interface, so these can be moved to the header. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... File remoting/base/process_stats_stub.h (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats_stub.h:13: class ProcessStatsStub { All other stats reside in remoting/protocol. It's best to move this one there as well. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats_stub.h:19: const protocol::AggregatedProcessResourceUsage& usage) = 0; base/ shouldn't depend on protocol https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... File remoting/base/process_stats_util.cc (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats_util.cc:33: aggregated.mutable_name()->append(usage.process_name()).append(", "); nit: it would be cleaner to assemble the aggregate name into a local string variable and then set it using set_name()
The CQ bit was checked by zijiehe@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/2775983003/diff/80001/remoting/base/BUILD.gn File remoting/base/BUILD.gn (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/BUILD.gn#... remoting/base/BUILD.gn:55: if (!is_nacl) { On 2017/04/03 23:50:21, Sergey Ulanov wrote: > In most other places we put everything in sources and then conditionally remove > it. There is already if (is_nacl) block below. Done. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/BUILD.gn#... remoting/base/BUILD.gn:56: # There is no ProcessMetrics implementation on NACL. On 2017/04/03 23:50:21, Sergey Ulanov wrote: > It's not implemented for Android either. Do we need any of this stuff outside of > remoting/host? If not, maybe move it to host directory? Done. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... File remoting/base/process_stats.cc (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats.cc:9: #include "remoting/base/process_stats.h" On 2017/04/03 23:50:21, Sergey Ulanov wrote: > This should be the first include in this file. Done. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats.cc:43: protocol::ProcessResourceUsage usage = agent->GetResourceUsage(); On 2017/04/03 23:50:21, Sergey Ulanov wrote: > Can this operation block? Would it be better to run it on a background thread? Emm, good point. On Windows and Mac OS, this operation does not contains IO operations. But on Linux, it needs to access /proc/<pid/task file. But at https://cs.chromium.org/chromium/src/base/process/process_metrics_linux.cc?rc..., the comment states accessing /proc files does not require disk operations. So we should be fine here. I would suggest we keep it as-is. Once we hit the IO violence, a background thread wrapper would help. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... File remoting/base/process_stats.h (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats.h:15: #include "remoting/proto/process_stats.pb.h" On 2017/04/03 23:50:21, Sergey Ulanov wrote: > base shouldn't depend on proto. These files have been moved to host. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats.h:20: class ProcessStats final { On 2017/04/03 23:50:21, Sergey Ulanov wrote: > Maybe ProcessStatsSender? Done. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats.h:23: // be outlived this instance. On 2017/04/03 23:50:21, Sergey Ulanov wrote: > s/It needs to be outlived this instance./It must outlive the ProcessStats > object./ Done. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats.h:35: void Stop(); On 2017/04/03 23:50:21, Sergey Ulanov wrote: > Do we need this methods? Would it be better just destroy this object? Emmm, indeed I believe this class should start immediately after its constructor, and stop in destruction. But consumers may need to add agents to it before it can actually work. I would argue if no agent is added, this class does nothing anyway. I think we can remove both Start() and Stop() functions, but add a ThreadChecker in AddProcessStatsAgent() function to ensure no cross-threads violence. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... File remoting/base/process_stats_agent.cc (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats_agent.cc:10: ProcessStatsAgent::~ProcessStatsAgent() = default; On 2017/04/03 23:50:21, Sergey Ulanov wrote: > ProcessStatsAgent is an abstract interface, so these can be moved to the header. Done. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... File remoting/base/process_stats_stub.h (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats_stub.h:13: class ProcessStatsStub { On 2017/04/03 23:50:21, Sergey Ulanov wrote: > All other stats reside in remoting/protocol. It's best to move this one there as > well. Do you mean all other "stubs"? Done. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats_stub.h:19: const protocol::AggregatedProcessResourceUsage& usage) = 0; On 2017/04/03 23:50:21, Sergey Ulanov wrote: > base/ shouldn't depend on protocol Done. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... File remoting/base/process_stats_util.cc (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats_util.cc:33: aggregated.mutable_name()->append(usage.process_name()).append(", "); On 2017/04/03 23:50:21, Sergey Ulanov wrote: > nit: it would be cleaner to assemble the aggregate name into a local string > variable and then set it using set_name() Done. Also updated other variables.
Couple more comments about ForwardProcessStatsAgent. Sorry I overlooked it on the previous round https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... File remoting/base/process_stats.cc (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats.cc:43: protocol::ProcessResourceUsage usage = agent->GetResourceUsage(); On 2017/04/05 20:52:16, Hzj_jie wrote: > On 2017/04/03 23:50:21, Sergey Ulanov wrote: > > Can this operation block? Would it be better to run it on a background thread? > > Emm, good point. On Windows and Mac OS, this operation does not contains IO > operations. But on Linux, it needs to access /proc/<pid/task file. But at > https://cs.chromium.org/chromium/src/base/process/process_metrics_linux.cc?rc..., > the comment states accessing /proc files does not require disk operations. > > So we should be fine here. I would suggest we keep it as-is. Once we hit the IO > violence, a background thread wrapper would help. Even if it doesn't contain any IO operation it may still take significant time to finish this operation, depending on how it's implemented. This is particularly problematic for WebRTC as it's sensetive to delays (e.g. delay of 2ms is already too much and may affect BW estimate). It's unlikely to be a problem when we getting stats for the current process. Maybe add a comment to mention that this may be a problem. https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... File remoting/base/process_stats_unittest.cc (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats_unittest.cc:123: FROM_HERE, run_loop.QuitClosure(), base::TimeDelta::FromSecondsD(3.2)); Looks like this test may be flaky if the timer task is delayed by 100ms, which is not unusual for tests. I think it would be better to use a fake StatsAgent that would return result depending on how many times GetResourceUsage() has been called. That would also allow to increase timer frequency for tests, so this test would complete faster (3 seconds is too long for simple unittests) https://codereview.chromium.org/2775983003/diff/100001/remoting/host/forward_... File remoting/host/forward_process_stats_agent.h (right): https://codereview.chromium.org/2775983003/diff/100001/remoting/host/forward_... remoting/host/forward_process_stats_agent.h:15: // A component to forward statistic data of another process. It's not completely clear how this is supposed to be used. Particularly how will the GetResourceUsage() is synchronized with OnProcessStats()? Do you need it just for tests? https://codereview.chromium.org/2775983003/diff/100001/remoting/host/process_... File remoting/host/process_stats_util.h (right): https://codereview.chromium.org/2775983003/diff/100001/remoting/host/process_... remoting/host/process_stats_util.h:13: namespace host { We have host namespace in other places. All other host-related classes are in remoting namespace. We could add it, but I don't think it makes sense to put just the new code you are adding in this CL. Will you move rest of host code there?
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
The CQ bit was checked by zijiehe@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...
https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... File remoting/base/process_stats.cc (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats.cc:43: protocol::ProcessResourceUsage usage = agent->GetResourceUsage(); On 2017/04/06 05:55:50, Sergey Ulanov wrote: > On 2017/04/05 20:52:16, Hzj_jie wrote: > > On 2017/04/03 23:50:21, Sergey Ulanov wrote: > > > Can this operation block? Would it be better to run it on a background > thread? > > > > Emm, good point. On Windows and Mac OS, this operation does not contains IO > > operations. But on Linux, it needs to access /proc/<pid/task file. But at > > > https://cs.chromium.org/chromium/src/base/process/process_metrics_linux.cc?rc..., > > the comment states accessing /proc files does not require disk operations. > > > > So we should be fine here. I would suggest we keep it as-is. Once we hit the > IO > > violence, a background thread wrapper would help. > > Even if it doesn't contain any IO operation it may still take significant time > to finish this operation, depending on how it's implemented. This is > particularly problematic for WebRTC as it's sensetive to delays (e.g. delay of > 2ms is already too much and may affect BW estimate). It's unlikely to be a > problem when we getting stats for the current process. Maybe add a comment to > mention that this may be a problem. Definitely. The comment is added into ProcessStatsAgent::GetResourceUsage(). https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... File remoting/base/process_stats_unittest.cc (right): https://codereview.chromium.org/2775983003/diff/80001/remoting/base/process_s... remoting/base/process_stats_unittest.cc:123: FROM_HERE, run_loop.QuitClosure(), base::TimeDelta::FromSecondsD(3.2)); On 2017/04/06 05:55:50, Sergey Ulanov wrote: > Looks like this test may be flaky if the timer task is delayed by 100ms, which > is not unusual for tests. I think it would be better to use a fake StatsAgent > that would return result depending on how many times GetResourceUsage() has been > called. That would also allow to increase timer frequency for tests, so this > test would complete faster (3 seconds is too long for simple unittests) Done. https://codereview.chromium.org/2775983003/diff/100001/remoting/host/forward_... File remoting/host/forward_process_stats_agent.h (right): https://codereview.chromium.org/2775983003/diff/100001/remoting/host/forward_... remoting/host/forward_process_stats_agent.h:15: // A component to forward statistic data of another process. On 2017/04/06 05:55:50, Sergey Ulanov wrote: > It's not completely clear how this is supposed to be used. Particularly how will > the GetResourceUsage() is synchronized with OnProcessStats()? > Do you need it just for tests? No, this class is designed to receive process usage from desktop process and merges with the usage of network process. https://codereview.chromium.org/2775983003/diff/100001/remoting/host/process_... File remoting/host/process_stats_util.h (right): https://codereview.chromium.org/2775983003/diff/100001/remoting/host/process_... remoting/host/process_stats_util.h:13: namespace host { On 2017/04/06 05:55:50, Sergey Ulanov wrote: > We have host namespace in other places. All other host-related classes are in > remoting namespace. We could add it, but I don't think it makes sense to put > just the new code you are adding in this CL. Will you move rest of host code > there? I have not noticed it :( I will remove the host namespace.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Any other comments for this change?
joedow@chromium.org changed reviewers: + joedow@chromium.org
https://codereview.chromium.org/2775983003/diff/120001/remoting/host/current_... File remoting/host/current_process_stats_agent.h (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/current_... remoting/host/current_process_stats_agent.h:11: #include "base/process/process_metrics.h" forward declare 'base::ProcessMetrics' since you don't need the definition in the header. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/current_... remoting/host/current_process_stats_agent.h:20: CurrentProcessStatsAgent(const std::string& process_name); single param c'tor should be marked explicit. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/current_... remoting/host/current_process_stats_agent.h:23: protocol::ProcessResourceUsage GetResourceUsage() override; nit: comment the override "// ProcessStatsAgent interface." https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.cc:27: DCHECK(thread_checker_.CalledOnValidThread()); nit: You don't need this DCHECK as thread_checker_ will DCHECK on its own when it is destroyed. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender.h (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.h:5: #ifndef REMOTING_HOST_PROCESS_STATS_H_ fix the macro guard (REMOTING_HOST_PROCESS_STATS_SENDER_H_) https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.h:14: #include "remoting/host/process_stats_agent.h" ProcessStatsAgent can be forward decalared https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.h:23: // ProcessStatsSender starts immediately, and reports the statistic data to nit: remove comma and 'the' https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.h:24: // |host_stats_stub| once per |interval_|. s/interval_/interval https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.h:31: // needs to be destructed on the same thread as constructing. You might want to move the thread requirements into the documentation for the class. Then again, having a thread checker as a member typically implies that it needs to be created/destroyed on the same thread. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.h:36: private: Do you want a private c'tor here? https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender_unittest.cc (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:7: #include <stdint.h> nit: #include <cstdint> instead of old C style header. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:25: class MockProcessStatsStub : public protocol::ProcessStatsStub { Rename MockProcessStatsStub to FakeProcessStatsStub (or similar) as it isn't a mock. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:46: class MockProcessStatsAgent : public ProcessStatsAgent { Rename to FakeProcessStatsAgent or TestProcessStatsAgent https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:62: // from 0) GetResourceUsage(). I'm a little confused by this term '|index|-th', maybe clean the comment up to something like "Checks the expected usage based on index"? https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:113: base::TimeDelta::FromMilliseconds(100)); You should avoid relying on intervals (i.e. 100ms) in unit tests. Your tests will run on a variety of machines and these can cause inadvertent race conditions. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:119: ASSERT_GT(stub.received().size(), 0U); Can you assert a specific value (rather than a generic greater than check)? I think it would be better to make this test deterministic rather than rely on a timeout (100ms). https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:173: MockProcessStatsAgent::AssertExpected(stub.received()[i], i * 2); IIUC you are verifying the aggregate, but can you also verify the individual payloads? https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_util.cc (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_util.cc:7: #include <stdint.h> #include <cstdint> https://codereview.chromium.org/2775983003/diff/120001/remoting/proto/process... File remoting/proto/process_stats.proto (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/proto/process... remoting/proto/process_stats.proto:34: optional string name = 1; I'm not sure how useful the aggregate is and whether we should spend time on the host building the struct up. I feel like it is much more useful to target a specific process (daemon, network, rdp, desktop) than having an aggregate view of them combined.
https://codereview.chromium.org/2775983003/diff/120001/remoting/host/forward_... File remoting/host/forward_process_stats_agent.h (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/forward_... remoting/host/forward_process_stats_agent.h:14: // A component to forward statistic data of another process. I don't think this comment is clear and it doesn't match what this class actually does. One might assume that this class talks to another process to get stats, but it doesn't. Maybe reword it as follows: // ProcessStatsAgent implementation that returns stats passed to // OnProcessStats(). Used to collect stats from processes other than // the current one. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/forward_... remoting/host/forward_process_stats_agent.h:25: base::Lock lock_; I don't think you need the lock. Normally all IPC messages are handled on the network thread, so OnProcessStats() will be called on the same thread that calls GetResourceUsage().
The CQ bit was checked by zijiehe@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...
Description was changed from ========== [Chromoting] Retrieve process resource usage (ProcessStats) and its tests ProcessStatsStub is an interface to receive process resource usage information from a ProcessStats. ProcessResourceUsage is a structure to store the resource usage of one process. AggregatedProcessResourceUsage is a structure to store the resource usgaes of several processes. ProcessStatsAgent is an interface to retrieve resource usage of one process. CurrentProcessStatsAgent is a ProcessStatsAgent implementation to retrieve resource usage from current process. ForwardProcessStatsAgent receives resource usage from a third-party and forwards the latest usage as a ProcessStatsAgent. ProcessStats is a class to regularly report resource usages from various ProcessStatsAgent implementations to a ProcessStatsStub. It merges several non-empty ProcessResourceUsage into one AggregatedProcessResourceUsage. This is typical useful on Windows: we need to calculate the resource usages from both network process and desktop process. R=sergeyu@chromium.org, jamiewalch@chromium.org BUG=650926 ========== to ========== [Chromoting] Retrieve process resource usage (ProcessStats) and its tests ProcessStatsStub is an interface to receive process resource usage information from a ProcessStats. ProcessResourceUsage is a structure to store the resource usage of one process. AggregatedProcessResourceUsage is a structure to store the resource usgaes of several processes. ProcessStatsAgent is an interface to retrieve resource usage of one process. CurrentProcessStatsAgent is a ProcessStatsAgent implementation to retrieve resource usage from current process. ForwardProcessStatsAgent receives resource usage from a third-party and forwards the latest usage as a ProcessStatsAgent. ProcessStatsSender is a class to regularly report resource usages from various ProcessStatsAgent implementations to a ProcessStatsStub. It merges several non-empty ProcessResourceUsage into one AggregatedProcessResourceUsage. This is typical useful on Windows: we need to calculate the resource usages from both network process and desktop process. R=sergeyu@chromium.org, jamiewalch@chromium.org BUG=650926 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #5 (id:140001) has been deleted
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/2775983003/diff/120001/remoting/host/current_... File remoting/host/current_process_stats_agent.h (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/current_... remoting/host/current_process_stats_agent.h:11: #include "base/process/process_metrics.h" On 2017/04/24 17:16:08, joedow wrote: > forward declare 'base::ProcessMetrics' since you don't need the definition in > the header. Done. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/current_... remoting/host/current_process_stats_agent.h:20: CurrentProcessStatsAgent(const std::string& process_name); On 2017/04/24 17:16:08, joedow wrote: > single param c'tor should be marked explicit. Done. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/current_... remoting/host/current_process_stats_agent.h:23: protocol::ProcessResourceUsage GetResourceUsage() override; On 2017/04/24 17:16:08, joedow wrote: > nit: comment the override "// ProcessStatsAgent interface." Done. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/forward_... File remoting/host/forward_process_stats_agent.h (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/forward_... remoting/host/forward_process_stats_agent.h:14: // A component to forward statistic data of another process. On 2017/04/24 18:49:11, Sergey Ulanov wrote: > I don't think this comment is clear and it doesn't match what this class > actually does. One might assume that this class talks to another process to get > stats, but it doesn't. Maybe reword it as follows: > // ProcessStatsAgent implementation that returns stats passed to > // OnProcessStats(). Used to collect stats from processes other than > // the current one. Done. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/forward_... remoting/host/forward_process_stats_agent.h:25: base::Lock lock_; On 2017/04/24 18:49:11, Sergey Ulanov wrote: > I don't think you need the lock. Normally all IPC messages are handled on the > network thread, so OnProcessStats() will be called on the same thread that calls > GetResourceUsage(). Changed to ThreadChecker. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.cc:27: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/04/24 17:16:08, joedow wrote: > nit: You don't need this DCHECK as thread_checker_ will DCHECK on its own when > it is destroyed. https://cs.chromium.org/chromium/src/base/threading/thread_checker_impl.cc?rc... It looks like the destructor of ThreadCheckerImpl is an no-op. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender.h (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.h:5: #ifndef REMOTING_HOST_PROCESS_STATS_H_ On 2017/04/24 17:16:09, joedow wrote: > fix the macro guard (REMOTING_HOST_PROCESS_STATS_SENDER_H_) Done. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.h:14: #include "remoting/host/process_stats_agent.h" On 2017/04/24 17:16:09, joedow wrote: > ProcessStatsAgent can be forward decalared Done. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.h:23: // ProcessStatsSender starts immediately, and reports the statistic data to On 2017/04/24 17:16:09, joedow wrote: > nit: remove comma and 'the' Done. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.h:24: // |host_stats_stub| once per |interval_|. On 2017/04/24 17:16:09, joedow wrote: > s/interval_/interval Done. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.h:31: // needs to be destructed on the same thread as constructing. On 2017/04/24 17:16:09, joedow wrote: > You might want to move the thread requirements into the documentation for the > class. Then again, having a thread checker as a member typically implies that > it needs to be created/destroyed on the same thread. Done. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.h:36: private: On 2017/04/24 17:16:09, joedow wrote: > Do you want a private c'tor here? Sorry, I do not follow. Why? https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender_unittest.cc (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:7: #include <stdint.h> On 2017/04/24 17:16:09, joedow wrote: > nit: #include <cstdint> instead of old C style header. It looks like stdint.h is preferred in Chromium: it defines types in global namespace instead of std. https://cs.chromium.org/search/?q=include.*cstdint+package:%5Echromium$&type=cs returns 80 results. https://cs.chromium.org/search/?q=include.*stdint%5C.h+package:%5Echromium$&t... returns 12600 results. I do see some files using std::uint32_t, but it's really not popular. https://cs.chromium.org/search/?q=std::uint32_t+package:%5Echromium$&type=cs returns 18 results. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:25: class MockProcessStatsStub : public protocol::ProcessStatsStub { On 2017/04/24 17:16:09, joedow wrote: > Rename MockProcessStatsStub to FakeProcessStatsStub (or similar) as it isn't a > mock. Oh, you have made two similar comments in my two changes. Would you mind to give me more hints why fake is preferred here than mock? (My pool English skill tells me both of these words have the meaning of "not real".) https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:46: class MockProcessStatsAgent : public ProcessStatsAgent { On 2017/04/24 17:16:09, joedow wrote: > Rename to FakeProcessStatsAgent or TestProcessStatsAgent Done. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:62: // from 0) GetResourceUsage(). On 2017/04/24 17:16:09, joedow wrote: > I'm a little confused by this term '|index|-th', maybe clean the comment up to > something like "Checks the expected usage based on index"? Done. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:113: base::TimeDelta::FromMilliseconds(100)); On 2017/04/24 17:16:09, joedow wrote: > You should avoid relying on intervals (i.e. 100ms) in unit tests. Your tests > will run on a variety of machines and these can cause inadvertent race > conditions. It's a little bit hard to do so. Indeed this test does not rely on this time interval, but only depends on a sequential task runner, and the first PostTask won't spend 100 ms. Here the trouble is we need to test that the "new ProcessStatsSender" starts the sending process. But the only place we can get the count is in the callback of the first PostDelayedTask call. assert_eq(stub.received().size(), stats->time_calls()) does not really help: the sending process may not start at all and causes all following assertions to be ignored. Do you have a better solution? https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:119: ASSERT_GT(stub.received().size(), 0U); On 2017/04/24 17:16:09, joedow wrote: > Can you assert a specific value (rather than a generic greater than check)? I > think it would be better to make this test deterministic rather than rely on a > timeout (100ms). See my previous comment. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:173: MockProcessStatsAgent::AssertExpected(stub.received()[i], i * 2); On 2017/04/24 17:16:09, joedow wrote: > IIUC you are verifying the aggregate, but can you also verify the individual > payloads? I think it's covered by ReportUsage test already. I am happy to add it. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_util.cc (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_util.cc:7: #include <stdint.h> On 2017/04/24 17:16:09, joedow wrote: > #include <cstdint> Same as process_stats_sender_unittest.cc. https://codereview.chromium.org/2775983003/diff/120001/remoting/proto/process... File remoting/proto/process_stats.proto (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/proto/process... remoting/proto/process_stats.proto:34: optional string name = 1; On 2017/04/24 17:16:09, joedow wrote: > I'm not sure how useful the aggregate is and whether we should spend time on the > host building the struct up. I feel like it is much more useful to target a > specific process (daemon, network, rdp, desktop) than having an aggregate view > of them combined. Yes, each individual usage is recorded in |usages| field.
https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.cc:27: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/04/25 00:17:53, Hzj_jie wrote: > On 2017/04/24 17:16:08, joedow wrote: > > nit: You don't need this DCHECK as thread_checker_ will DCHECK on its own when > > it is destroyed. > > https://cs.chromium.org/chromium/src/base/threading/thread_checker_impl.cc?rc... > It looks like the destructor of ThreadCheckerImpl is an no-op. The ThreadChecker class contains a 'SequenceToken' member which has a DCHECK in its d'tor that will trigger if it is destroyed on a different thread than its affinity. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender.h (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.h:36: private: On 2017/04/25 00:17:53, Hzj_jie wrote: > On 2017/04/24 17:16:09, joedow wrote: > > Do you want a private c'tor here? > > Sorry, I do not follow. Why? To prevent someone from creating a uninitialized instance (i.e. an instance w/o a valid host_stats-stub and interval). https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender_unittest.cc (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:7: #include <stdint.h> On 2017/04/25 00:17:54, Hzj_jie wrote: > On 2017/04/24 17:16:09, joedow wrote: > > nit: #include <cstdint> instead of old C style header. > > It looks like stdint.h is preferred in Chromium: it defines types in global > namespace instead of std. > https://cs.chromium.org/search/?q=include.*cstdint+package:%5Echromium$&type=cs > returns 80 results. > https://cs.chromium.org/search/?q=include.*stdint%5C.h+package:%5Echromium$&t... > returns 12600 results. > I do see some files using std::uint32_t, but it's really not popular. > https://cs.chromium.org/search/?q=std::uint32_t+package:%5Echromium$&type=cs > returns 18 results. It is one of the newly allowed C++11 features (https://chromium-cpp.appspot.com/) so I'm not surprised that older code doesn't use it. I'd prefer C++ headers over old c-style, also I can't think of a downside of using the new header either. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:25: class MockProcessStatsStub : public protocol::ProcessStatsStub { On 2017/04/25 00:17:54, Hzj_jie wrote: > On 2017/04/24 17:16:09, joedow wrote: > > Rename MockProcessStatsStub to FakeProcessStatsStub (or similar) as it isn't a > > mock. > > Oh, you have made two similar comments in my two changes. Would you mind to give > me more hints why fake is preferred here than mock? (My pool English skill tells > me both of these words have the meaning of "not real".) They have different meanings when it comes to testing (for instance https://www.martinfowler.com/articles/mocksArentStubs.html). The gist is that a mock object (in chromium these derive from GMock classes) must be set up with call expectations and possibly with default return values. A Fake object has a simplified impl of the real object. A stub is like a mock but doesn't care about which methods were called. This class feels like a Fake since it receives the usages but does nothing with them except provide them for testing. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:113: base::TimeDelta::FromMilliseconds(100)); On 2017/04/25 00:17:54, Hzj_jie wrote: > On 2017/04/24 17:16:09, joedow wrote: > > You should avoid relying on intervals (i.e. 100ms) in unit tests. Your tests > > will run on a variety of machines and these can cause inadvertent race > > conditions. > > It's a little bit hard to do so. Indeed this test does not rely on this time > interval, but only depends on a sequential task runner, and the first PostTask > won't spend 100 ms. > Here the trouble is we need to test that the "new ProcessStatsSender" starts the > sending process. But the only place we can get the count is in the callback of > the first PostDelayedTask call. assert_eq(stub.received().size(), > stats->time_calls()) does not really help: the sending process may not start at > all and causes all following assertions to be ignored. > > Do you have a better solution? It might spend 100ms if it was running on a machine with heavy CPU usage and one of the thread/memory sanitizers running. We can talk offline if you'd like, but it is important to make sure these tests are solid as you don't want to have to deal with flakiness on the TSAN/ASAN bots. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:173: MockProcessStatsAgent::AssertExpected(stub.received()[i], i * 2); On 2017/04/25 00:17:54, Hzj_jie wrote: > On 2017/04/24 17:16:09, joedow wrote: > > IIUC you are verifying the aggregate, but can you also verify the individual > > payloads? > > I think it's covered by ReportUsage test already. I am happy to add it. Thanks for adding it. It isn't necessarily covered by the previous test as you only had one usage agent before, not two. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_util.cc (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_util.cc:7: #include <stdint.h> On 2017/04/25 00:17:54, Hzj_jie wrote: > On 2017/04/24 17:16:09, joedow wrote: > > #include <cstdint> > > Same as process_stats_sender_unittest.cc. Same as my comment there as well, I think it is a good idea to use the new C++11 header instead of the c-style header. https://codereview.chromium.org/2775983003/diff/160001/remoting/host/current_... File remoting/host/current_process_stats_agent.h (right): https://codereview.chromium.org/2775983003/diff/160001/remoting/host/current_... remoting/host/current_process_stats_agent.h:26: // ProcessStatsAgent implementation nit: add period to the end of comments. https://codereview.chromium.org/2775983003/diff/160001/remoting/host/forward_... File remoting/host/forward_process_stats_agent.h (right): https://codereview.chromium.org/2775983003/diff/160001/remoting/host/forward_... remoting/host/forward_process_stats_agent.h:26: // ProcessStatsAgent implementation nit: add period
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.cc:27: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/04/25 15:57:27, joedow wrote: > On 2017/04/25 00:17:53, Hzj_jie wrote: > > On 2017/04/24 17:16:08, joedow wrote: > > > nit: You don't need this DCHECK as thread_checker_ will DCHECK on its own > when > > > it is destroyed. > > > > > https://cs.chromium.org/chromium/src/base/threading/thread_checker_impl.cc?rc... > > It looks like the destructor of ThreadCheckerImpl is an no-op. > > The ThreadChecker class contains a 'SequenceToken' member which has a DCHECK in > its d'tor that will trigger if it is destroyed on a different thread than its > affinity. SequenceToken has no destructor. Have I misunderstood something? But the design you have described seems not correct, what if a class using ThreadChecker is designed to be destructed in a different thread? https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender.h (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.h:36: private: On 2017/04/25 15:57:28, joedow wrote: > On 2017/04/25 00:17:53, Hzj_jie wrote: > > On 2017/04/24 17:16:09, joedow wrote: > > > Do you want a private c'tor here? > > > > Sorry, I do not follow. Why? > > To prevent someone from creating a uninitialized instance (i.e. an instance w/o > a valid host_stats-stub and interval). Ah, that seems not necessary. An explicit constructor (say, the one in line 27) will always suppress the default constructor. See http://cpp.sh/4bb4. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender_unittest.cc (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:7: #include <stdint.h> On 2017/04/25 15:57:28, joedow wrote: > On 2017/04/25 00:17:54, Hzj_jie wrote: > > On 2017/04/24 17:16:09, joedow wrote: > > > nit: #include <cstdint> instead of old C style header. > > > > It looks like stdint.h is preferred in Chromium: it defines types in global > > namespace instead of std. > > > https://cs.chromium.org/search/?q=include.*cstdint+package:%5Echromium$&type=cs > > returns 80 results. > > > https://cs.chromium.org/search/?q=include.*stdint%5C.h+package:%5Echromium$&t... > > returns 12600 results. > > I do see some files using std::uint32_t, but it's really not popular. > > https://cs.chromium.org/search/?q=std::uint32_t+package:%5Echromium$&type=cs > > returns 18 results. > > It is one of the newly allowed C++11 features > (https://chromium-cpp.appspot.com/) so I'm not surprised that older code doesn't > use it. > > I'd prefer C++ headers over old c-style, also I can't think of a downside of > using the new header either. I believe it's an allowed feature, but a standard implementation of cstdint equals to namespace std { #include <stdint.h> } // namespace std http://en.cppreference.com/w/cpp/header/cstdint v.s. http://en.cppreference.com/w/c/types/integer So it means we need to change all int32_t to std::int32_t in this file. I believe some files mistakenly use cstdint with ::int32_t, but it's not standard. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:25: class MockProcessStatsStub : public protocol::ProcessStatsStub { On 2017/04/25 15:57:28, joedow wrote: > On 2017/04/25 00:17:54, Hzj_jie wrote: > > On 2017/04/24 17:16:09, joedow wrote: > > > Rename MockProcessStatsStub to FakeProcessStatsStub (or similar) as it isn't > a > > > mock. > > > > Oh, you have made two similar comments in my two changes. Would you mind to > give > > me more hints why fake is preferred here than mock? (My pool English skill > tells > > me both of these words have the meaning of "not real".) > > They have different meanings when it comes to testing (for instance > https://www.martinfowler.com/articles/mocksArentStubs.html). > > The gist is that a mock object (in chromium these derive from GMock classes) > must be set up with call expectations and possibly with default return values. > > A Fake object has a simplified impl of the real object. > > A stub is like a mock but doesn't care about which methods were called. > > This class feels like a Fake since it receives the usages but does nothing with > them except provide them for testing. Thank you for the explanation, very clear. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:113: base::TimeDelta::FromMilliseconds(100)); On 2017/04/25 15:57:28, joedow wrote: > On 2017/04/25 00:17:54, Hzj_jie wrote: > > On 2017/04/24 17:16:09, joedow wrote: > > > You should avoid relying on intervals (i.e. 100ms) in unit tests. Your > tests > > > will run on a variety of machines and these can cause inadvertent race > > > conditions. > > > > It's a little bit hard to do so. Indeed this test does not rely on this time > > interval, but only depends on a sequential task runner, and the first PostTask > > won't spend 100 ms. > > Here the trouble is we need to test that the "new ProcessStatsSender" starts > the > > sending process. But the only place we can get the count is in the callback of > > the first PostDelayedTask call. assert_eq(stub.received().size(), > > stats->time_calls()) does not really help: the sending process may not start > at > > all and causes all following assertions to be ignored. > > > > Do you have a better solution? > > It might spend 100ms if it was running on a machine with heavy CPU usage and one > of the thread/memory sanitizers running. > > We can talk offline if you'd like, but it is important to make sure these tests > are solid as you don't want to have to deal with flakiness on the TSAN/ASAN > bots. I agree. A simple choice is to remove the ASSERT_GT in line 119. But it means part of the logic has not been tested. We may talk offline. https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:173: MockProcessStatsAgent::AssertExpected(stub.received()[i], i * 2); On 2017/04/25 15:57:28, joedow wrote: > On 2017/04/25 00:17:54, Hzj_jie wrote: > > On 2017/04/24 17:16:09, joedow wrote: > > > IIUC you are verifying the aggregate, but can you also verify the individual > > > payloads? > > > > I think it's covered by ReportUsage test already. I am happy to add it. > > Thanks for adding it. It isn't necessarily covered by the previous test as you > only had one usage agent before, not two. Oh, reasonable. https://codereview.chromium.org/2775983003/diff/160001/remoting/host/current_... File remoting/host/current_process_stats_agent.h (right): https://codereview.chromium.org/2775983003/diff/160001/remoting/host/current_... remoting/host/current_process_stats_agent.h:26: // ProcessStatsAgent implementation On 2017/04/25 15:57:28, joedow wrote: > nit: add period to the end of comments. Done. https://codereview.chromium.org/2775983003/diff/160001/remoting/host/forward_... File remoting/host/forward_process_stats_agent.h (right): https://codereview.chromium.org/2775983003/diff/160001/remoting/host/forward_... remoting/host/forward_process_stats_agent.h:26: // ProcessStatsAgent implementation On 2017/04/25 15:57:28, joedow wrote: > nit: add period Done.
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 checked by zijiehe@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.
lgtm https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.cc:27: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/04/25 22:19:19, Hzj_jie wrote: > On 2017/04/25 15:57:27, joedow wrote: > > On 2017/04/25 00:17:53, Hzj_jie wrote: > > > On 2017/04/24 17:16:08, joedow wrote: > > > > nit: You don't need this DCHECK as thread_checker_ will DCHECK on its own > > when > > > > it is destroyed. > > > > > > > > > https://cs.chromium.org/chromium/src/base/threading/thread_checker_impl.cc?rc... > > > It looks like the destructor of ThreadCheckerImpl is an no-op. > > > > The ThreadChecker class contains a 'SequenceToken' member which has a DCHECK > in > > its d'tor that will trigger if it is destroyed on a different thread than its > > affinity. > > SequenceToken has no destructor. Have I misunderstood something? I think you are right. I thought myself that ThreadChecker had this check in the destructor (because NonThreadSafe has it) and with some of my CL comments on other CLs led Joe to believe it as well. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/current_... File remoting/host/current_process_stats_agent.h (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/current_... remoting/host/current_process_stats_agent.h:20: // A component to report statistic data of current process. s/current/the current/ https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender.cc:40: if (agents_.empty()) { Maybe replace this with a check that usages is not empty below? I don't think we want to send an empty AggregateProcessResourceUsage. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... File remoting/host/process_stats_sender.h (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender.h:42: const base::TimeDelta interval_; I don't think you need this field. The constructor can just pass interval parameter to the timer_
lgtm lgtm https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.cc:27: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/04/25 22:19:19, Hzj_jie wrote: > On 2017/04/25 15:57:27, joedow wrote: > > On 2017/04/25 00:17:53, Hzj_jie wrote: > > > On 2017/04/24 17:16:08, joedow wrote: > > > > nit: You don't need this DCHECK as thread_checker_ will DCHECK on its own > > when > > > > it is destroyed. > > > > > > > > > https://cs.chromium.org/chromium/src/base/threading/thread_checker_impl.cc?rc... > > > It looks like the destructor of ThreadCheckerImpl is an no-op. > > > > The ThreadChecker class contains a 'SequenceToken' member which has a DCHECK > in > > its d'tor that will trigger if it is destroyed on a different thread than its > > affinity. > > SequenceToken has no destructor. Have I misunderstood something? I think you are right. I thought myself that ThreadChecker had this check in the destructor (because NonThreadSafe has it) and with some of my CL comments on other CLs led Joe to believe it as well. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/current_... File remoting/host/current_process_stats_agent.h (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/current_... remoting/host/current_process_stats_agent.h:20: // A component to report statistic data of current process. s/current/the current/ https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender.cc:40: if (agents_.empty()) { Maybe replace this with a check that usages is not empty below? I don't think we want to send an empty AggregateProcessResourceUsage. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... File remoting/host/process_stats_sender.h (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender.h:42: const base::TimeDelta interval_; I don't think you need this field. The constructor can just pass interval parameter to the timer_
https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.cc:27: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/04/26 05:42:44, Sergey Ulanov wrote: > On 2017/04/25 22:19:19, Hzj_jie wrote: > > On 2017/04/25 15:57:27, joedow wrote: > > > On 2017/04/25 00:17:53, Hzj_jie wrote: > > > > On 2017/04/24 17:16:08, joedow wrote: > > > > > nit: You don't need this DCHECK as thread_checker_ will DCHECK on its > own > > > when > > > > > it is destroyed. > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/base/threading/thread_checker_impl.cc?rc... > > > > It looks like the destructor of ThreadCheckerImpl is an no-op. > > > > > > The ThreadChecker class contains a 'SequenceToken' member which has a DCHECK > > in > > > its d'tor that will trigger if it is destroyed on a different thread than > its > > > affinity. > > > > SequenceToken has no destructor. Have I misunderstood something? > > I think you are right. I thought myself that ThreadChecker had this check in the > destructor (because NonThreadSafe has it) and with some of my CL comments on > other CLs led Joe to believe it as well. I was looking at ScopedSequenceToken :( I did a quick test and no DCHECKs are done when ThreadChecker is destroyed. I am going to file a bug to fix up the rest of the classes where we want this protection but don't have it. Thanks for pointing this out Zijie!
https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... File remoting/host/process_stats_sender.h (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender.h:33: // ProcessStatsSender stops in destruction. I don't think this comment is needed (most classes stop doing things when they are destroyed). If it is important for usage of the calss, you can move it into the class documentation but I think it is safe to delete. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... File remoting/host/process_stats_sender_unittest.cc (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:34: if (received_.size() >= static_cast<size_t>(expected_usage_received_) && Would this ever be a problem if you expected 2 usage structs but instead of one call with 2 you received 2 calls with 1 struct each? You may not but if that is the case then you would want to use a current_usage_count_ to compare against. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:36: quit_closure_.Run(); Do you want to immediately run this closure? You might be safer posting it to the task_runner (that way the method that calls this function can return and clean up before the test runs more code). https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:45: void SetQuitWhen(int expected_usage_received, base::Closure quit_closure) { The names here are a little confusing. Instead of having this method, I think it would be better to have individual setters for |quit_closure_| and |expected_usage_received_|. If you default |expected_usage_received_| to 1, then you'd only need to set that value in tests where you have multiple agents sending usage data. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:46: expected_usage_received_ = expected_usage_received; nit: rename expected_usage_received_ to expected_usage_count_ ? https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:52: int expected_usage_received_ = 0; If this member were a size_t, could you remove the cast above? https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:135: ASSERT_GT(stub.received().size(), 0U); You can use a specific comparison now (since you aren't relying on a timeout), can you update these checks?
The CQ bit was checked by zijiehe@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...
https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2775983003/diff/120001/remoting/host/process_... remoting/host/process_stats_sender.cc:27: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/04/26 16:07:57, joedow wrote: > On 2017/04/26 05:42:44, Sergey Ulanov wrote: > > On 2017/04/25 22:19:19, Hzj_jie wrote: > > > On 2017/04/25 15:57:27, joedow wrote: > > > > On 2017/04/25 00:17:53, Hzj_jie wrote: > > > > > On 2017/04/24 17:16:08, joedow wrote: > > > > > > nit: You don't need this DCHECK as thread_checker_ will DCHECK on its > > own > > > > when > > > > > > it is destroyed. > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/base/threading/thread_checker_impl.cc?rc... > > > > > It looks like the destructor of ThreadCheckerImpl is an no-op. > > > > > > > > The ThreadChecker class contains a 'SequenceToken' member which has a > DCHECK > > > in > > > > its d'tor that will trigger if it is destroyed on a different thread than > > its > > > > affinity. > > > > > > SequenceToken has no destructor. Have I misunderstood something? > > > > I think you are right. I thought myself that ThreadChecker had this check in > the > > destructor (because NonThreadSafe has it) and with some of my CL comments on > > other CLs led Joe to believe it as well. > > I was looking at ScopedSequenceToken :( I did a quick test and no DCHECKs are > done when ThreadChecker is destroyed. I am going to file a bug to fix up the > rest of the classes where we want this protection but don't have it. > > Thanks for pointing this out Zijie! :) https://codereview.chromium.org/2775983003/diff/200001/remoting/host/current_... File remoting/host/current_process_stats_agent.h (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/current_... remoting/host/current_process_stats_agent.h:20: // A component to report statistic data of current process. On 2017/04/26 05:42:44, Sergey Ulanov wrote: > s/current/the current/ Done. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender.cc:40: if (agents_.empty()) { On 2017/04/26 05:42:44, Sergey Ulanov wrote: > Maybe replace this with a check that usages is not empty below? I don't think we > want to send an empty AggregateProcessResourceUsage. If so, I would also update the comment in header file to explain the expected behavior: users should call AddProcessStatsAgent immediately after the constructor. Done. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... File remoting/host/process_stats_sender.h (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender.h:33: // ProcessStatsSender stops in destruction. On 2017/04/26 16:44:06, joedow wrote: > I don't think this comment is needed (most classes stop doing things when they > are destroyed). If it is important for usage of the calss, you can move it into > the class documentation but I think it is safe to delete. I have moved the comments regarding to the time of starting and stopping all into the class documentation. Done. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender.h:42: const base::TimeDelta interval_; On 2017/04/26 05:42:44, Sergey Ulanov wrote: > I don't think you need this field. The constructor can just pass interval > parameter to the timer_ Oh, yes, that's for the old Start() and Stop() design. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... File remoting/host/process_stats_sender_unittest.cc (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:34: if (received_.size() >= static_cast<size_t>(expected_usage_received_) && On 2017/04/26 16:44:06, joedow wrote: > Would this ever be a problem if you expected 2 usage structs but instead of one > call with 2 you received 2 calls with 1 struct each? You may not but if that is > the case then you would want to use a current_usage_count_ to compare against. No, I do not think it's possible, since we push_back one usage per OnProcessStats() call. I would change this line into DCHECK_LE(received_.size(), static_cast<size_t>(expected_usage_received_)). And also, I believe checking whether quit_closure_.is_null() is not necessary: tests should always call SetQuitWhen(). Instead of testing the behavior of the ProcessStatsSender, these ifs are checking the behavior of the FakeProcessStatsStub itself. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:36: quit_closure_.Run(); On 2017/04/26 16:44:06, joedow wrote: > Do you want to immediately run this closure? You might be safer posting it to > the task_runner (that way the method that calls this function can return and > clean up before the test runs more code). I think running it immediately should be a better choice, otherwise we may encounter some uncertainties whether the quit_closure_ or the next ProcessStatsSender::Report would be run first. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:45: void SetQuitWhen(int expected_usage_received, base::Closure quit_closure) { On 2017/04/26 16:44:06, joedow wrote: > The names here are a little confusing. Instead of having this method, I think > it would be better to have individual setters for |quit_closure_| and > |expected_usage_received_|. If you default |expected_usage_received_| to 1, > then you'd only need to set that value in tests where you have multiple agents > sending usage data. Done. But no matter how many agents, the stub receives consistent count of AggregatedProcessResourceUsage. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:46: expected_usage_received_ = expected_usage_received; On 2017/04/26 16:44:06, joedow wrote: > nit: rename expected_usage_received_ to expected_usage_count_ ? Done. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:52: int expected_usage_received_ = 0; On 2017/04/26 16:44:06, joedow wrote: > If this member were a size_t, could you remove the cast above? I believe the code style discourages the usage of size_t (unsigned). But unfortunately, std::vector::size() returns size_t. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:135: ASSERT_GT(stub.received().size(), 0U); On 2017/04/26 16:44:06, joedow wrote: > You can use a specific comparison now (since you aren't relying on a timeout), > can you update these checks? Sorry, I simply forgot to update it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender.cc:40: if (agents_.empty()) { On 2017/04/26 20:47:19, Hzj_jie wrote: > On 2017/04/26 05:42:44, Sergey Ulanov wrote: > > Maybe replace this with a check that usages is not empty below? I don't think > we > > want to send an empty AggregateProcessResourceUsage. > > If so, I would also update the comment in header file to explain the expected > behavior: users should call AddProcessStatsAgent immediately after the > constructor. > Done. That seems like a dangerous assumption. Maybe you should not start the timer until an agent is added. That kind of makes sense as there is no reason to fire a timer if there are no agents registered. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... File remoting/host/process_stats_sender_unittest.cc (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:36: quit_closure_.Run(); On 2017/04/26 20:47:19, Hzj_jie wrote: > On 2017/04/26 16:44:06, joedow wrote: > > Do you want to immediately run this closure? You might be safer posting it to > > the task_runner (that way the method that calls this function can return and > > clean up before the test runs more code). > > I think running it immediately should be a better choice, otherwise we may > encounter some uncertainties whether the quit_closure_ or the next > ProcessStatsSender::Report would be run first. This callback is only used for testing so I don't see a benefit for calling it directly. The common pattern is to post the resume closure so the previous code can finish. Not doing so can lead (though not in this specific CL) to your test instance being torn down while it is running (if an ASSERT FAILS) for instance. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:52: int expected_usage_received_ = 0; On 2017/04/26 20:47:20, Hzj_jie wrote: > On 2017/04/26 16:44:06, joedow wrote: > > If this member were a size_t, could you remove the cast above? > > I believe the code style discourages the usage of size_t (unsigned). But > unfortunately, std::vector::size() returns size_t. Coding style discourages it if your intention is to show that a value should never be negative. It is fine if you need to interop with methods that return it. I would switch it an remove the casts above. https://codereview.chromium.org/2775983003/diff/220001/remoting/host/process_... File remoting/host/process_stats_sender.h (right): https://codereview.chromium.org/2775983003/diff/220001/remoting/host/process_... remoting/host/process_stats_sender.h:22: // after construction, and stops before destruction. s/before/after The class stops when the d'tor is called, but I don't think it stops beforehand.
The CQ bit was checked by zijiehe@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...
https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender.cc:40: if (agents_.empty()) { On 2017/04/26 23:48:01, joedow wrote: > On 2017/04/26 20:47:19, Hzj_jie wrote: > > On 2017/04/26 05:42:44, Sergey Ulanov wrote: > > > Maybe replace this with a check that usages is not empty below? I don't > think > > we > > > want to send an empty AggregateProcessResourceUsage. > > > > If so, I would also update the comment in header file to explain the expected > > behavior: users should call AddProcessStatsAgent immediately after the > > constructor. > > Done. > > That seems like a dangerous assumption. Maybe you should not start the timer > until an agent is added. That kind of makes sense as there is no reason to fire > a timer if there are no agents registered. Emmm, looks like once the if-check has been changed into a DCHECK, it's more reasonable to move the functionality of AddProcessStatsAgent() into the constructor directly. Meanwhile taking ownership of ProcessStatsAgent is also not a good idea, especially for ForwardProcessStatsAgent, it needs to work with both ProcessStatsSender and IPC channel. https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... File remoting/host/process_stats_sender_unittest.cc (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:36: quit_closure_.Run(); On 2017/04/26 23:48:01, joedow wrote: > On 2017/04/26 20:47:19, Hzj_jie wrote: > > On 2017/04/26 16:44:06, joedow wrote: > > > Do you want to immediately run this closure? You might be safer posting it > to > > > the task_runner (that way the method that calls this function can return and > > > clean up before the test runs more code). > > > > I think running it immediately should be a better choice, otherwise we may > > encounter some uncertainties whether the quit_closure_ or the next > > ProcessStatsSender::Report would be run first. > > This callback is only used for testing so I don't see a benefit for calling it > directly. The common pattern is to post the resume closure so the previous code > can finish. > > Not doing so can lead (though not in this specific CL) to your test instance > being torn down while it is running (if an ASSERT FAILS) for instance. Sorry, I have not followed. IMO, if the quit_closure_ is posted to the task runner but after the next Report() function, i.e. this line uses over 1 millisecond, we will have 11 samples instead of 10. RepeatedTimer schedules next task before running current one. (https://cs.chromium.org/chromium/src/base/timer/timer.cc?rcl=65f71c8b5c87a14a...) https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender_unittest.cc:52: int expected_usage_received_ = 0; On 2017/04/26 23:48:01, joedow wrote: > On 2017/04/26 20:47:20, Hzj_jie wrote: > > On 2017/04/26 16:44:06, joedow wrote: > > > If this member were a size_t, could you remove the cast above? > > > > I believe the code style discourages the usage of size_t (unsigned). But > > unfortunately, std::vector::size() returns size_t. > > Coding style discourages it if your intention is to show that a value should > never be negative. It is fine if you need to interop with methods that return > it. I would switch it an remove the casts above. Done. https://codereview.chromium.org/2775983003/diff/220001/remoting/host/process_... File remoting/host/process_stats_sender.h (right): https://codereview.chromium.org/2775983003/diff/220001/remoting/host/process_... remoting/host/process_stats_sender.h:22: // after construction, and stops before destruction. On 2017/04/26 23:48:02, joedow wrote: > s/before/after > > The class stops when the d'tor is called, but I don't think it stops beforehand. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender.cc:40: if (agents_.empty()) { On 2017/04/27 02:21:33, Hzj_jie wrote: > On 2017/04/26 23:48:01, joedow wrote: > > On 2017/04/26 20:47:19, Hzj_jie wrote: > > > On 2017/04/26 05:42:44, Sergey Ulanov wrote: > > > > Maybe replace this with a check that usages is not empty below? I don't > > think > > > we > > > > want to send an empty AggregateProcessResourceUsage. > > > > > > If so, I would also update the comment in header file to explain the > expected > > > behavior: users should call AddProcessStatsAgent immediately after the > > > constructor. > > > Done. > > > > That seems like a dangerous assumption. Maybe you should not start the timer > > until an agent is added. That kind of makes sense as there is no reason to > fire > > a timer if there are no agents registered. > > Emmm, looks like once the if-check has been changed into a DCHECK, it's more > reasonable to move the functionality of AddProcessStatsAgent() into the > constructor directly. > Meanwhile taking ownership of ProcessStatsAgent is also not a good idea, > especially for ForwardProcessStatsAgent, it needs to work with both > ProcessStatsSender and IPC channel. I don't think you need to fire the timer if no one registers an agent. We can see how it feels once you have a working implementation.
https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2775983003/diff/200001/remoting/host/process_... remoting/host/process_stats_sender.cc:40: if (agents_.empty()) { On 2017/04/27 15:01:55, joedow wrote: > On 2017/04/27 02:21:33, Hzj_jie wrote: > > On 2017/04/26 23:48:01, joedow wrote: > > > On 2017/04/26 20:47:19, Hzj_jie wrote: > > > > On 2017/04/26 05:42:44, Sergey Ulanov wrote: > > > > > Maybe replace this with a check that usages is not empty below? I don't > > > think > > > > we > > > > > want to send an empty AggregateProcessResourceUsage. > > > > > > > > If so, I would also update the comment in header file to explain the > > expected > > > > behavior: users should call AddProcessStatsAgent immediately after the > > > > constructor. > > > > Done. > > > > > > That seems like a dangerous assumption. Maybe you should not start the > timer > > > until an agent is added. That kind of makes sense as there is no reason to > > fire > > > a timer if there are no agents registered. > > > > Emmm, looks like once the if-check has been changed into a DCHECK, it's more > > reasonable to move the functionality of AddProcessStatsAgent() into the > > constructor directly. > > Meanwhile taking ownership of ProcessStatsAgent is also not a good idea, > > especially for ForwardProcessStatsAgent, it needs to work with both > > ProcessStatsSender and IPC channel. > > I don't think you need to fire the timer if no one registers an agent. We can > see how it feels once you have a working implementation. As far as I can imagine, it's definitely wrong if one creates a ProcessStatsSender but does not register an agent. Yes, let me finish the next change so we can discuss this with more context.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2775983003/#ps240001 (title: "Resolve review comments")
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": 240001, "attempt_start_ts": 1493317788660710,
"parent_rev": "bdf153cfc2bce1eb1b22e0d00e0e3580d54b152d", "commit_rev":
"5928f440f84065ab45f848dcd135e3c2f52ca5bb"}
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Retrieve process resource usage (ProcessStats) and its tests ProcessStatsStub is an interface to receive process resource usage information from a ProcessStats. ProcessResourceUsage is a structure to store the resource usage of one process. AggregatedProcessResourceUsage is a structure to store the resource usgaes of several processes. ProcessStatsAgent is an interface to retrieve resource usage of one process. CurrentProcessStatsAgent is a ProcessStatsAgent implementation to retrieve resource usage from current process. ForwardProcessStatsAgent receives resource usage from a third-party and forwards the latest usage as a ProcessStatsAgent. ProcessStatsSender is a class to regularly report resource usages from various ProcessStatsAgent implementations to a ProcessStatsStub. It merges several non-empty ProcessResourceUsage into one AggregatedProcessResourceUsage. This is typical useful on Windows: we need to calculate the resource usages from both network process and desktop process. R=sergeyu@chromium.org, jamiewalch@chromium.org BUG=650926 ========== to ========== [Chromoting] Retrieve process resource usage (ProcessStats) and its tests ProcessStatsStub is an interface to receive process resource usage information from a ProcessStats. ProcessResourceUsage is a structure to store the resource usage of one process. AggregatedProcessResourceUsage is a structure to store the resource usgaes of several processes. ProcessStatsAgent is an interface to retrieve resource usage of one process. CurrentProcessStatsAgent is a ProcessStatsAgent implementation to retrieve resource usage from current process. ForwardProcessStatsAgent receives resource usage from a third-party and forwards the latest usage as a ProcessStatsAgent. ProcessStatsSender is a class to regularly report resource usages from various ProcessStatsAgent implementations to a ProcessStatsStub. It merges several non-empty ProcessResourceUsage into one AggregatedProcessResourceUsage. This is typical useful on Windows: we need to calculate the resource usages from both network process and desktop process. R=sergeyu@chromium.org, jamiewalch@chromium.org BUG=650926 Review-Url: https://codereview.chromium.org/2775983003 Cr-Commit-Position: refs/heads/master@{#467753} Committed: https://chromium.googlesource.com/chromium/src/+/5928f440f84065ab45f848dcd135... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001) as https://chromium.googlesource.com/chromium/src/+/5928f440f84065ab45f848dcd135...
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:240001) has been created in https://codereview.chromium.org/2847743004/ by warx@chromium.org. The reason for reverting is: Breaks x86-alex-tot-chrome-pfq-informational build: https://uberchromegw.corp.google.com/i/chromeos.chrome/builders/x86-alex-tot-....
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Retrieve process resource usage (ProcessStats) and its tests ProcessStatsStub is an interface to receive process resource usage information from a ProcessStats. ProcessResourceUsage is a structure to store the resource usage of one process. AggregatedProcessResourceUsage is a structure to store the resource usgaes of several processes. ProcessStatsAgent is an interface to retrieve resource usage of one process. CurrentProcessStatsAgent is a ProcessStatsAgent implementation to retrieve resource usage from current process. ForwardProcessStatsAgent receives resource usage from a third-party and forwards the latest usage as a ProcessStatsAgent. ProcessStatsSender is a class to regularly report resource usages from various ProcessStatsAgent implementations to a ProcessStatsStub. It merges several non-empty ProcessResourceUsage into one AggregatedProcessResourceUsage. This is typical useful on Windows: we need to calculate the resource usages from both network process and desktop process. R=sergeyu@chromium.org, jamiewalch@chromium.org BUG=650926 Review-Url: https://codereview.chromium.org/2775983003 Cr-Commit-Position: refs/heads/master@{#467753} Committed: https://chromium.googlesource.com/chromium/src/+/5928f440f84065ab45f848dcd135... ========== to ========== [Chromoting] Retrieve process resource usage (ProcessStats) and its tests ProcessStatsStub is an interface to receive process resource usage information from a ProcessStats. ProcessResourceUsage is a structure to store the resource usage of one process. AggregatedProcessResourceUsage is a structure to store the resource usgaes of several processes. ProcessStatsAgent is an interface to retrieve resource usage of one process. CurrentProcessStatsAgent is a ProcessStatsAgent implementation to retrieve resource usage from current process. ForwardProcessStatsAgent receives resource usage from a third-party and forwards the latest usage as a ProcessStatsAgent. ProcessStatsSender is a class to regularly report resource usages from various ProcessStatsAgent implementations to a ProcessStatsStub. It merges several non-empty ProcessResourceUsage into one AggregatedProcessResourceUsage. This is typical useful on Windows: we need to calculate the resource usages from both network process and desktop process. R=sergeyu@chromium.org, jamiewalch@chromium.org BUG=650926 Review-Url: https://codereview.chromium.org/2775983003 Cr-Commit-Position: refs/heads/master@{#467753} Committed: https://chromium.googlesource.com/chromium/src/+/5928f440f84065ab45f848dcd135... ==========
The CQ bit was checked by zijiehe@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 zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, joedow@chromium.org Link to the patchset: https://codereview.chromium.org/2775983003/#ps260001 (title: "thread_checker_ should be correctly constructed")
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": 260001, "attempt_start_ts": 1493345153823650,
"parent_rev": "4df7199f5d2cafd62bf9c9d016caa341230139ca", "commit_rev":
"4f3f095f04100e1e443cfdcba1813bdaa625bc77"}
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Retrieve process resource usage (ProcessStats) and its tests ProcessStatsStub is an interface to receive process resource usage information from a ProcessStats. ProcessResourceUsage is a structure to store the resource usage of one process. AggregatedProcessResourceUsage is a structure to store the resource usgaes of several processes. ProcessStatsAgent is an interface to retrieve resource usage of one process. CurrentProcessStatsAgent is a ProcessStatsAgent implementation to retrieve resource usage from current process. ForwardProcessStatsAgent receives resource usage from a third-party and forwards the latest usage as a ProcessStatsAgent. ProcessStatsSender is a class to regularly report resource usages from various ProcessStatsAgent implementations to a ProcessStatsStub. It merges several non-empty ProcessResourceUsage into one AggregatedProcessResourceUsage. This is typical useful on Windows: we need to calculate the resource usages from both network process and desktop process. R=sergeyu@chromium.org, jamiewalch@chromium.org BUG=650926 Review-Url: https://codereview.chromium.org/2775983003 Cr-Commit-Position: refs/heads/master@{#467753} Committed: https://chromium.googlesource.com/chromium/src/+/5928f440f84065ab45f848dcd135... ========== to ========== [Chromoting] Retrieve process resource usage (ProcessStats) and its tests ProcessStatsStub is an interface to receive process resource usage information from a ProcessStats. ProcessResourceUsage is a structure to store the resource usage of one process. AggregatedProcessResourceUsage is a structure to store the resource usgaes of several processes. ProcessStatsAgent is an interface to retrieve resource usage of one process. CurrentProcessStatsAgent is a ProcessStatsAgent implementation to retrieve resource usage from current process. ForwardProcessStatsAgent receives resource usage from a third-party and forwards the latest usage as a ProcessStatsAgent. ProcessStatsSender is a class to regularly report resource usages from various ProcessStatsAgent implementations to a ProcessStatsStub. It merges several non-empty ProcessResourceUsage into one AggregatedProcessResourceUsage. This is typical useful on Windows: we need to calculate the resource usages from both network process and desktop process. R=sergeyu@chromium.org, jamiewalch@chromium.org BUG=650926 Review-Url: https://codereview.chromium.org/2775983003 Cr-Original-Commit-Position: refs/heads/master@{#467753} Committed: https://chromium.googlesource.com/chromium/src/+/5928f440f84065ab45f848dcd135... Review-Url: https://codereview.chromium.org/2775983003 Cr-Commit-Position: refs/heads/master@{#467844} Committed: https://chromium.googlesource.com/chromium/src/+/4f3f095f04100e1e443cfdcba181... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001) as https://chromium.googlesource.com/chromium/src/+/4f3f095f04100e1e443cfdcba181... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
