Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2388933003:
Make ResourceReporter a client of memory coordinator (Closed)
|
Created:
4 years, 2 months ago by hajimehoshi Modified:
4 years, 1 month ago CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake ResourceReporter a client of memory coordinator
When the memory coordinator is enabled, ResourceReporter becomes a
client of the memory coordinator instead of installing
MemoryPressureListener.
In the implmentation of this CL, both MemoryPressureListener and
MemoryCoordinatorClient are installed regardless of the flag, but as
default MemoryCoordinator is not enabled and when MemoryCoordinator is
enabled by a flag, it is planed to suppress MemoryPressureMonitor
features.
Design Doc: https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8Uo/edit?ts=57d7968b#
BUG=639700
Committed: https://crrev.com/71fa1218175050b39812872fc707f97250860b5e
Cr-Commit-Position: refs/heads/master@{#428664}
Patch Set 1 #
Total comments: 2
Patch Set 2 : (rebasing) #
Total comments: 13
Patch Set 3 : Address on reviews #
Total comments: 4
Patch Set 4 : Address on afakhry's review #
Depends on Patchset: Messages
Total messages: 32 (17 generated)
The CQ bit was checked by hajimehoshi@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...
hajimehoshi@chromium.org changed reviewers: + afakhry@chromium.org, bashi@chromium.org, chrisha@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2388933003/diff/1/chrome/browser/chromeos/res... File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/2388933003/diff/1/chrome/browser/chromeos/res... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:376: RecordCurrentState(); If I remember the thread we've discussed this migration correctly, these UMAs/rappor aren't very useful. In memory coordinator world, we might want to remove RecordCurrentState(). So my suggestion is that not making ResourceReporter be a client of memory coordinator. When MemoryPressureListener is replaced with memory coordinator, we can remove OnTrimMemory(). afakhry@: What do you think?
On 2016/10/04 10:12:20, bashi1 wrote: > https://codereview.chromium.org/2388933003/diff/1/chrome/browser/chromeos/res... > File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): > > https://codereview.chromium.org/2388933003/diff/1/chrome/browser/chromeos/res... > chrome/browser/chromeos/resource_reporter/resource_reporter.cc:376: > RecordCurrentState(); > If I remember the thread we've discussed this migration correctly, these > UMAs/rappor aren't very useful. In memory coordinator world, we might want to > remove RecordCurrentState(). So my suggestion is that not making > ResourceReporter be a client of memory coordinator. When MemoryPressureListener > is replaced with memory coordinator, we can remove OnTrimMemory(). > > afakhry@: What do you think? ping
https://codereview.chromium.org/2388933003/diff/1/chrome/browser/chromeos/res... File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/2388933003/diff/1/chrome/browser/chromeos/res... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:376: RecordCurrentState(); On 2016/10/04 10:12:20, bashi1 wrote: > If I remember the thread we've discussed this migration correctly, these > UMAs/rappor aren't very useful. In memory coordinator world, we might want to > remove RecordCurrentState(). So my suggestion is that not making > ResourceReporter be a client of memory coordinator. When MemoryPressureListener > is replaced with memory coordinator, we can remove OnTrimMemory(). > > afakhry@: What do you think? The situation has been changed by https://codereview.chromium.org/2363223002 and now UMAs are much more meaningful?
The CQ bit was checked by hajimehoshi@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/2388933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (left): https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:121: base::Bind(&ResourceReporter::OnMemoryPressure, base::Unretained(this)))); Now, I'm a bit confused! I thought we're moving to a MemoryCoordinatorClient as a replacement for being a MemoryPressureListener. What am I missing? https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:223: base::MemoryCoordinatorClientRegistry::GetInstance()->Register(this); I believe this line belongs to StartMonitoring(). Also shouldn't there be a corresponding: base::MemoryCoordinatorClientRegistry::GetInstance()->Unregister(this); in StopMonitoring()? https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:438: // |state|. ResourceReporter doesn't have a feature to limit memory usage at I don't understand this comment. The ResourceReporter was never meant to be used to limit memory usage. I'm not aware of any design docs suggesting to extend its behavior. Please clarify. https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:450: NOTREACHED(); What effect does this NOTREACHED() have on Chrome OS debug builds? Do we ever hit SUSPENDED or UNKNOWN? I just want to make sure that we don't cause developers a lot of pain crashing their runs because of this NOTREACHED(). https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right): https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.h:154: // base::MemoryCoordinatorClient implementation: Nit: remove "implementation". Just say: // base::MemoryCoordinatorClient:
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://chromiumcodereview.appspot.com/2388933003/diff/20001/chrome/browser/c... File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://chromiumcodereview.appspot.com/2388933003/diff/20001/chrome/browser/c... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:438: // |state|. ResourceReporter doesn't have a feature to limit memory usage at On 2016/10/18 16:20:20, afakhry wrote: > I don't understand this comment. The ResourceReporter was never meant to be used > to limit memory usage. I'm not aware of any design docs suggesting to extend its > behavior. Please clarify. You're correct. ResourceReporter is used only for UMA reporting. Let's just remove the comment.
Thank you! https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (left): https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:121: base::Bind(&ResourceReporter::OnMemoryPressure, base::Unretained(this)))); On 2016/10/18 16:20:20, afakhry wrote: > Now, I'm a bit confused! I thought we're moving to a MemoryCoordinatorClient as > a replacement for being a MemoryPressureListener. What am I missing? Temporarily MemoryCoordinatorClient and MemoryPressureListener are present at the same time during migration. MemoryCoordinator and MemoryPressure are exclusive and an experimental flag --memory-coordinator enables MemoryCoordinator and disables MemoryPressure. https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:223: base::MemoryCoordinatorClientRegistry::GetInstance()->Register(this); On 2016/10/18 16:20:20, afakhry wrote: > I believe this line belongs to StartMonitoring(). > > Also shouldn't there be a corresponding: > base::MemoryCoordinatorClientRegistry::GetInstance()->Unregister(this); > in StopMonitoring()? Done. https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:438: // |state|. ResourceReporter doesn't have a feature to limit memory usage at On 2016/10/18 18:52:20, haraken wrote: > On 2016/10/18 16:20:20, afakhry wrote: > > I don't understand this comment. The ResourceReporter was never meant to be > used > > to limit memory usage. I'm not aware of any design docs suggesting to extend > its > > behavior. Please clarify. > > You're correct. ResourceReporter is used only for UMA reporting. Let's just > remove the comment. > > Hmm, so this doesn't match with MemoryCoordinator's 'stateful' concept. I removed this comment, but actually should ResourceReporter be MemoryCoordinatorClient? +bashi, +chrisha https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:450: NOTREACHED(); On 2016/10/18 16:20:20, afakhry wrote: > What effect does this NOTREACHED() have on Chrome OS debug builds? Do we ever > hit SUSPENDED or UNKNOWN? I just want to make sure that we don't cause > developers a lot of pain crashing their runs because of this NOTREACHED(). Right, SUSPENDED and UNKNOWN never come to this component. https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right): https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.h:154: // base::MemoryCoordinatorClient implementation: On 2016/10/18 16:20:20, afakhry wrote: > Nit: remove "implementation". Just say: > // base::MemoryCoordinatorClient: Done.
https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:438: // |state|. ResourceReporter doesn't have a feature to limit memory usage at On 2016/10/25 07:39:42, hajimehoshi wrote: > On 2016/10/18 18:52:20, haraken wrote: > > On 2016/10/18 16:20:20, afakhry wrote: > > > I don't understand this comment. The ResourceReporter was never meant to be > > used > > > to limit memory usage. I'm not aware of any design docs suggesting to extend > > its > > > behavior. Please clarify. > > > > You're correct. ResourceReporter is used only for UMA reporting. Let's just > > remove the comment. > > > > > > Hmm, so this doesn't match with MemoryCoordinator's 'stateful' concept. I > removed this comment, but actually should ResourceReporter be > MemoryCoordinatorClient? +bashi, +chrisha On second thought, this starting/stopping recording UMA seems stateful and this matches MemoryCoordinator's concept. Sorry for confusion. PTAL
The CQ bit was checked by hajimehoshi@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...
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/2388933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:438: // |state|. ResourceReporter doesn't have a feature to limit memory usage at On 2016/10/26 07:42:36, hajimehoshi wrote: > On 2016/10/25 07:39:42, hajimehoshi wrote: > > On 2016/10/18 18:52:20, haraken wrote: > > > On 2016/10/18 16:20:20, afakhry wrote: > > > > I don't understand this comment. The ResourceReporter was never meant to > be > > > used > > > > to limit memory usage. I'm not aware of any design docs suggesting to > extend > > > its > > > > behavior. Please clarify. > > > > > > You're correct. ResourceReporter is used only for UMA reporting. Let's just > > > remove the comment. > > > > > > > > > > Hmm, so this doesn't match with MemoryCoordinator's 'stateful' concept. I > > removed this comment, but actually should ResourceReporter be > > MemoryCoordinatorClient? +bashi, +chrisha > > On second thought, this starting/stopping recording UMA seems stateful and this > matches MemoryCoordinator's concept. Sorry for confusion. > > PTAL Yeah, this seems like a reasonable use to me, to grab a snapshot of the resource situation when under pressure.
lgtm with 2 nits. https://codereview.chromium.org/2388933003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/2388933003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:137: observed_task_manager()->RemoveObserver(this); Nit: Since we now have StopRecordingCurrentState(), for consistency and future changes, let's use it here instead. https://codereview.chromium.org/2388933003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:198: observed_task_manager()->RemoveObserver(this); Nit: Same reason as above. Let's use StopRecordingCurrentState() here too.
Thank you! https://codereview.chromium.org/2388933003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/2388933003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:137: observed_task_manager()->RemoveObserver(this); On 2016/10/27 16:16:20, afakhry wrote: > Nit: Since we now have StopRecordingCurrentState(), for consistency and future > changes, let's use it here instead. Done. https://codereview.chromium.org/2388933003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/resource_reporter/resource_reporter.cc:198: observed_task_manager()->RemoveObserver(this); On 2016/10/27 16:16:20, afakhry wrote: > Nit: Same reason as above. Let's use StopRecordingCurrentState() here too. Done.
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org, haraken@chromium.org, chrisha@chromium.org Link to the patchset: https://codereview.chromium.org/2388933003/#ps60001 (title: "Address on afakhry's review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Make ResourceReporter a client of memory coordinator When the memory coordinator is enabled, ResourceReporter becomes a client of the memory coordinator instead of installing MemoryPressureListener. In the implmentation of this CL, both MemoryPressureListener and MemoryCoordinatorClient are installed regardless of the flag, but as default MemoryCoordinator is not enabled and when MemoryCoordinator is enabled by a flag, it is planed to suppress MemoryPressureMonitor features. Design Doc: https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8... BUG=639700 ========== to ========== Make ResourceReporter a client of memory coordinator When the memory coordinator is enabled, ResourceReporter becomes a client of the memory coordinator instead of installing MemoryPressureListener. In the implmentation of this CL, both MemoryPressureListener and MemoryCoordinatorClient are installed regardless of the flag, but as default MemoryCoordinator is not enabled and when MemoryCoordinator is enabled by a flag, it is planed to suppress MemoryPressureMonitor features. Design Doc: https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8... BUG=639700 Committed: https://crrev.com/71fa1218175050b39812872fc707f97250860b5e Cr-Commit-Position: refs/heads/master@{#428664} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/71fa1218175050b39812872fc707f97250860b5e Cr-Commit-Position: refs/heads/master@{#428664} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
