Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2964613002:
[Chromoting] Deprecate AggregatedProcessResourceUsage.* (Closed)
Description[Chromoting] Deprecate AggregatedProcessResourceUsage.*
Resource usages of each individual process do not need to be aggregated in host
anymore.
BUG=650926
Review-Url: https://codereview.chromium.org/2964613002
Cr-Commit-Position: refs/heads/master@{#487684}
Committed: https://chromium.googlesource.com/chromium/src/+/f401c5186e4f98d89388ee8cf23886a75d2da310
Patch Set 1 #
Total comments: 8
Patch Set 2 : Resolve review comments #
Total comments: 10
Patch Set 3 : Resolve review comments #Patch Set 4 : Use latest ipc_message_protobuf_utils to serialize and deserialize AggregatedProcessResourceUsage #
Messages
Total messages: 45 (26 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...
Description was changed from ========== [Chromoting] Deprecate AggregatedProcessResourceUsage.* Resource usages of each individual process do not need to be aggregated in host anymore. BUG=650926 ========== to ========== [Chromoting] Deprecate AggregatedProcessResourceUsage.* Resource usages of each individual process do not need to be aggregated in host anymore. BUG=650926 ==========
zijiehe@chromium.org changed reviewers: + joedow@chromium.org, kelvinp@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2964613002/diff/1/remoting/host/process_stats... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2964613002/diff/1/remoting/host/process_stats... remoting/host/process_stats_sender.cc:18: return !usage.has_process_name() && !usage.has_processor_usage() && Is it ok to add a usage which is missing one of these fields? It seems like missing something like |process_name| would indicate the buffer was malformed. https://codereview.chromium.org/2964613002/diff/1/remoting/host/process_stats... remoting/host/process_stats_sender.cc:19: !usage.has_working_set_size() && !usage.has_pagefile_size(); I think the logic would be simpler if the method name was re-written to assert that |usage| is valid. bool IsProcessResourceUsageValid(const protocol::ProcessResourceUsage& usage) { return usage.has_process_name() || usage.has_processor_usage() || usage.has_working_set_size() || usage.has_pagefile_size(); } https://codereview.chromium.org/2964613002/diff/1/remoting/host/process_stats... File remoting/host/process_stats_sender_unittest.cc (right): https://codereview.chromium.org/2964613002/diff/1/remoting/host/process_stats... remoting/host/process_stats_sender_unittest.cc:80: ASSERT_EQ(usage.usages().Get(0).processor_usage(), index); Do you want to ASSERT on processor_name as well? https://codereview.chromium.org/2964613002/diff/1/remoting/proto/process_stat... File remoting/proto/process_stats.proto (right): https://codereview.chromium.org/2964613002/diff/1/remoting/proto/process_stat... remoting/proto/process_stats.proto:34: optional string name = 1 [deprecated=true]; Instead of marking these as deprecated, I think you can delete them and renumber. No one depends on this structure so you aren't breaking any existing users and the file will look a lot cleaner.
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/2964613002/diff/1/remoting/host/process_stats... File remoting/host/process_stats_sender.cc (right): https://codereview.chromium.org/2964613002/diff/1/remoting/host/process_stats... remoting/host/process_stats_sender.cc:18: return !usage.has_process_name() && !usage.has_processor_usage() && On 2017/06/30 15:13:43, joedow wrote: > Is it ok to add a usage which is missing one of these fields? It seems like > missing something like |process_name| would indicate the buffer was malformed. Done. https://codereview.chromium.org/2964613002/diff/1/remoting/host/process_stats... remoting/host/process_stats_sender.cc:19: !usage.has_working_set_size() && !usage.has_pagefile_size(); On 2017/06/30 15:13:43, joedow wrote: > I think the logic would be simpler if the method name was re-written to assert > that |usage| is valid. > > bool IsProcessResourceUsageValid(const protocol::ProcessResourceUsage& usage) { > return usage.has_process_name() || usage.has_processor_usage() || > usage.has_working_set_size() || usage.has_pagefile_size(); > } Done. https://codereview.chromium.org/2964613002/diff/1/remoting/host/process_stats... File remoting/host/process_stats_sender_unittest.cc (right): https://codereview.chromium.org/2964613002/diff/1/remoting/host/process_stats... remoting/host/process_stats_sender_unittest.cc:80: ASSERT_EQ(usage.usages().Get(0).processor_usage(), index); On 2017/06/30 15:13:43, joedow wrote: > Do you want to ASSERT on processor_name as well? Done. https://codereview.chromium.org/2964613002/diff/1/remoting/proto/process_stat... File remoting/proto/process_stats.proto (right): https://codereview.chromium.org/2964613002/diff/1/remoting/proto/process_stat... remoting/proto/process_stats.proto:34: optional string name = 1 [deprecated=true]; On 2017/06/30 15:13:43, joedow wrote: > Instead of marking these as deprecated, I think you can delete them and > renumber. No one depends on this structure so you aren't breaking any existing > users and the file will look a lot cleaner. Done.
lgtm
The CQ bit was checked by zijiehe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
zijiehe@chromium.org changed reviewers: + estark@chromium.org
Emily, would you please review the changes to chromoting_param_traits.*? Thank you.
https://codereview.chromium.org/2964613002/diff/20001/remoting/host/chromotin... File remoting/host/chromoting_param_traits.cc (right): https://codereview.chromium.org/2964613002/diff/20001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:300: m->WriteInt(p.usages_size()); Can this be a WriteUint64?
https://codereview.chromium.org/2964613002/diff/20001/remoting/host/chromotin... File remoting/host/chromoting_param_traits.cc (right): https://codereview.chromium.org/2964613002/diff/20001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:300: m->WriteInt(p.usages_size()); On 2017/07/02 00:24:04, estark (OOO until Jul 10) wrote: > Can this be a WriteUint64? According to the generated code (https://cs.chromium.org/chromium/src/out/Debug/gen/remoting/proto/process_sta...), this function should return an integer. Though I have no concern to use WriteUint64(), which should have (almost) the same effect.
zijiehe@chromium.org changed reviewers: + dcheng@chromium.org - estark@chromium.org
Emily is OOO until 10th. Daniel, would you mind to have a look at this change? Thank you.
https://codereview.chromium.org/2964613002/diff/20001/remoting/host/chromotin... File remoting/host/chromoting_param_traits.cc (right): https://codereview.chromium.org/2964613002/diff/20001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:300: m->WriteInt(p.usages_size()); On 2017/07/02 20:59:05, Hzj_jie wrote: > On 2017/07/02 00:24:04, estark (OOO until Jul 10) wrote: > > Can this be a WriteUint64? > > According to the generated code > (https://cs.chromium.org/chromium/src/out/Debug/gen/remoting/proto/process_sta...), > this function should return an integer. Though I have no concern to use > WriteUint64(), which should have (almost) the same effect. Since this is specified to return int (currently), I think it probably makes sense to keep it as int. =/ https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#r... However, rather than iterating through usages here, perhaps we can add a ParamTraits for RepeatedPtrField<T>, similar to how we have one for std::vector<T>? https://codereview.chromium.org/2964613002/diff/20001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:320: for (int i = 0; i < usages_size; i++) { Most importantly, that lets us consolidate checks for things like |size| being reasonable: while it doesn't necessarily crash if size is negative here, it would probably be better to return false and fail deserialization.
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/2964613002/diff/20001/remoting/host/chromotin... File remoting/host/chromoting_param_traits.cc (right): https://codereview.chromium.org/2964613002/diff/20001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:300: m->WriteInt(p.usages_size()); On 2017/07/06 00:01:56, dcheng wrote: > On 2017/07/02 20:59:05, Hzj_jie wrote: > > On 2017/07/02 00:24:04, estark (OOO until Jul 10) wrote: > > > Can this be a WriteUint64? > > > > According to the generated code > > > (https://cs.chromium.org/chromium/src/out/Debug/gen/remoting/proto/process_sta...), > > this function should return an integer. Though I have no concern to use > > WriteUint64(), which should have (almost) the same effect. > > Since this is specified to return int (currently), I think it probably makes > sense to keep it as int. =/ > > https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#r... > > However, rather than iterating through usages here, perhaps we can add a > ParamTraits for RepeatedPtrField<T>, similar to how we have one for > std::vector<T>? Since RepeatedField<T> is a standard instead of an implementation detail, we can definitely add it. But it looks like adding ParamTraits<RepeatedField<T>> is out of the scope of this change: would you mind me to do it in a different change? https://codereview.chromium.org/2964613002/diff/20001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:320: for (int i = 0; i < usages_size; i++) { On 2017/07/06 00:01:57, dcheng wrote: > Most importantly, that lets us consolidate checks for things like |size| being > reasonable: while it doesn't necessarily crash if size is negative here, it > would probably be better to return false and fail deserialization. I have replaced GetInt() with GetLength(). But I do not think it would crash here. Have I missed anything?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2964613002/diff/20001/remoting/host/chromotin... File remoting/host/chromoting_param_traits.cc (right): https://codereview.chromium.org/2964613002/diff/20001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:300: m->WriteInt(p.usages_size()); On 2017/07/06 00:45:37, Hzj_jie wrote: > On 2017/07/06 00:01:56, dcheng wrote: > > On 2017/07/02 20:59:05, Hzj_jie wrote: > > > On 2017/07/02 00:24:04, estark (OOO until Jul 10) wrote: > > > > Can this be a WriteUint64? > > > > > > According to the generated code > > > > > > (https://cs.chromium.org/chromium/src/out/Debug/gen/remoting/proto/process_sta...), > > > this function should return an integer. Though I have no concern to use > > > WriteUint64(), which should have (almost) the same effect. > > > > Since this is specified to return int (currently), I think it probably makes > > sense to keep it as int. =/ > > > > > https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#r... > > > > However, rather than iterating through usages here, perhaps we can add a > > ParamTraits for RepeatedPtrField<T>, similar to how we have one for > > std::vector<T>? > > Since RepeatedField<T> is a standard instead of an implementation detail, we can > definitely add it. > > But it looks like adding ParamTraits<RepeatedField<T>> is out of the scope of > this change: would you mind me to do it in a different change? Well, it seems like this CL introduces serialization of a repeated proto field, doesn't it? (If you want it to do it in a CL that lands before this one, that would be fine though) https://codereview.chromium.org/2964613002/diff/20001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:320: for (int i = 0; i < usages_size; i++) { On 2017/07/06 00:45:37, Hzj_jie wrote: > On 2017/07/06 00:01:57, dcheng wrote: > > Most importantly, that lets us consolidate checks for things like |size| being > > reasonable: while it doesn't necessarily crash if size is negative here, it > > would probably be better to return false and fail deserialization. > > I have replaced GetInt() with GetLength(). But I do not think it would crash > here. Have I missed anything? It won't crash here. But in general, it's preferable to signal deserialization failure if we unpickled an invalid message (and a negative length seems like it should always be invalid to me)
https://codereview.chromium.org/2964613002/diff/20001/remoting/host/chromotin... File remoting/host/chromoting_param_traits.cc (right): https://codereview.chromium.org/2964613002/diff/20001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:300: m->WriteInt(p.usages_size()); On 2017/07/06 08:08:10, dcheng wrote: > On 2017/07/06 00:45:37, Hzj_jie wrote: > > On 2017/07/06 00:01:56, dcheng wrote: > > > On 2017/07/02 20:59:05, Hzj_jie wrote: > > > > On 2017/07/02 00:24:04, estark (OOO until Jul 10) wrote: > > > > > Can this be a WriteUint64? > > > > > > > > According to the generated code > > > > > > > > > > (https://cs.chromium.org/chromium/src/out/Debug/gen/remoting/proto/process_sta...), > > > > this function should return an integer. Though I have no concern to use > > > > WriteUint64(), which should have (almost) the same effect. > > > > > > Since this is specified to return int (currently), I think it probably makes > > > sense to keep it as int. =/ > > > > > > > > > https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#r... > > > > > > However, rather than iterating through usages here, perhaps we can add a > > > ParamTraits for RepeatedPtrField<T>, similar to how we have one for > > > std::vector<T>? > > > > Since RepeatedField<T> is a standard instead of an implementation detail, we > can > > definitely add it. > > > > But it looks like adding ParamTraits<RepeatedField<T>> is out of the scope of > > this change: would you mind me to do it in a different change? > > Well, it seems like this CL introduces serialization of a repeated proto field, > doesn't it? > > (If you want it to do it in a CL that lands before this one, that would be fine > though) Definitely, the change is at https://codereview.chromium.org/2968003005/, I will send it to you for reviewing after trybots finished. https://codereview.chromium.org/2964613002/diff/20001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:320: for (int i = 0; i < usages_size; i++) { On 2017/07/06 08:08:09, dcheng wrote: > On 2017/07/06 00:45:37, Hzj_jie wrote: > > On 2017/07/06 00:01:57, dcheng wrote: > > > Most importantly, that lets us consolidate checks for things like |size| > being > > > reasonable: while it doesn't necessarily crash if size is negative here, it > > > would probably be better to return false and fail deserialization. > > > > I have replaced GetInt() with GetLength(). But I do not think it would crash > > here. Have I missed anything? > > It won't crash here. But in general, it's preferable to signal deserialization > failure if we unpickled an invalid message (and a negative length seems like it > should always be invalid to me) Got you. The newly prepared change should also fix this issue.
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.
Daniel, the code has been updated to use the ipc_message_protobuf_utils. Would you please have a look? Thank you.
ipc lgtm. thanks for splitting out the traits for repeated proto fields!
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org Link to the patchset: https://codereview.chromium.org/2964613002/#ps60001 (title: "Use latest ipc_message_protobuf_utils to serialize and deserialize AggregatedProcessResourceUsage")
On 2017/07/19 00:15:39, dcheng wrote: > ipc lgtm. thanks for splitting out the traits for repeated proto fields! :)
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": 60001, "attempt_start_ts": 1500423390929470,
"parent_rev": "89a278c8a89ebaaa6a941166672c7ead4f019709", "commit_rev":
"f401c5186e4f98d89388ee8cf23886a75d2da310"}
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Deprecate AggregatedProcessResourceUsage.* Resource usages of each individual process do not need to be aggregated in host anymore. BUG=650926 ========== to ========== [Chromoting] Deprecate AggregatedProcessResourceUsage.* Resource usages of each individual process do not need to be aggregated in host anymore. BUG=650926 Review-Url: https://codereview.chromium.org/2964613002 Cr-Commit-Position: refs/heads/master@{#487684} Committed: https://chromium.googlesource.com/chromium/src/+/f401c5186e4f98d89388ee8cf238... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f401c5186e4f98d89388ee8cf238... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
