Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2097753002:
Make memory pressure notifier configurable (Closed)
|
Created:
4 years, 6 months ago by bashi Modified:
4 years, 5 months ago CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, gavinp+memory_chromium.org, halliwell+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake memory pressure notifier configurable
In memory coordinator v0 we try to replace MemoryPressureListener
with MemoryCoordinatorClient as described in [1]. This CL adds
SetNotifier() method which takes a callback for dispatching
memory pressure notifications. This allows us to replace
MemoryPressureListener::NotifyMemoryPressure() with an arbitrary
function.
[1] https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVIYI/edit#
BUG=617492
Committed: https://crrev.com/456eef44d60c8cddc72f53d4e3d13ffd80158df3
Cr-Commit-Position: refs/heads/master@{#404327}
Patch Set 1 : fix #
Total comments: 19
Patch Set 2 : comment #Patch Set 3 : comment #
Total comments: 4
Patch Set 4 : comment #
Total comments: 6
Patch Set 5 : use callback #Patch Set 6 : fix compile errors #Patch Set 7 : Fix another error #
Total comments: 1
Patch Set 8 : Fix win #Messages
Total messages: 48 (18 generated)
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2097753002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
bashi@chromium.org changed reviewers: + chrisha@chromium.org
PTAL (or suggest reviewers?) Thanks! https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:42: void Notify(MemoryPressureLevel level); public because this is called by a static method (mac::MemoryPressureMonitor::NotifyMemoryPressureChanged()).
This allows you to intercept memory pressure notifications, and have them be dispatched the something else. Presumably the MemoryCoordinator? Which will then push notifications to MemoryPressureListeners as needed, until they've all been removed? Am I understanding this correctly? You mentioned [1] in the CL comment, is there a document I'm missing?
On 2016/06/27 20:19:36, chrisha (slow) wrote: > This allows you to intercept memory pressure notifications, and have them be > dispatched the something else. Presumably the MemoryCoordinator? Which will then > push notifications to MemoryPressureListeners as needed, until they've all been > removed? Yes. That's what I want to do that. > > Am I understanding this correctly? You mentioned [1] in the CL comment, is there > a document I'm missing? Ah, sorry I forgot to add it. I meant https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI... Will update the description.
Description was changed from ========== Make memory pressure notifier configurable In memory coordinator v0 we try to replace MemoryPressureListener with MemoryCoordinatorClient as described in [1]. This CL adds SetNotifier() method which takes a callback for dispatching memory pressure notifications. This allows us to replace MemoryPressureListener::NotifyMemoryPressure() with an arbitrary function. BUG=617492 ========== to ========== Make memory pressure notifier configurable In memory coordinator v0 we try to replace MemoryPressureListener with MemoryCoordinatorClient as described in [1]. This CL adds SetNotifier() method which takes a callback for dispatching memory pressure notifications. This allows us to replace MemoryPressureListener::NotifyMemoryPressure() with an arbitrary function. [1] https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI... BUG=617492 ==========
After reading the design doc and discussing with haraken, lgtm!
bashi@chromium.org changed reviewers: + alokp@chromium.org, danakj@chromium.org
Thanks! Asking owners review. +danakj@ for base/ owners review. +alokp@ for chromecast/ owners review.
On 2016/06/30 00:53:38, bashi1 wrote: > Thanks! Asking owners review. > > +danakj@ for base/ owners review. > +alokp@ for chromecast/ owners review. chromecast/ lgtm
https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:15: // TODO(chrisha): Make this a concrete class with per-OS implementations rather Is this something that will be done as part of this? Because on the list of things that make me uncomfortable about code, near the top is classes that are half abstract and half concrete. And that's where this goes now. https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:36: // Sets a notification callback. Once a callback is set, I tried to get this from the doc, but I think maybe it's hidden below the surface of the larger design: - Why is it a callback instead of a listener interface? - Why is there only a single listener instead of a set? https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:39: using Notifier = base::Callback<void(MemoryPressureLevel level)>; "Notifier" is the wrong direction here. The notifier is the monitor. This would be a Listener or an Observer or the like, it receives notifications.
https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:15: // TODO(chrisha): Make this a concrete class with per-OS implementations rather On 2016/06/30 18:48:05, danakj wrote: > Is this something that will be done as part of this? Because on the list of > things that make me uncomfortable about code, near the top is classes that are > half abstract and half concrete. And that's where this goes now. I don't plan to do that. I'm not sure this still applies because we need to have a way to change notifier from MemoryPressureListener to MemoryCoordinator which we are going to implement. https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:36: // Sets a notification callback. Once a callback is set, On 2016/06/30 18:48:05, danakj wrote: > I tried to get this from the doc, but I think maybe it's hidden below the > surface of the larger design: > - Why is it a callback instead of a listener interface? We are implementing MemoryCoordinator which will replace MemoryPressureListener. Currently MemoryPressureMonitor lives in base/ and MemoryCoordinator will live in components/ so I use callback to avoid dependency violation. > - Why is there only a single listener instead of a set? I'm afraid you may misunderstand what I'm going to do here. There are some listeners actually. MemoryPressureListener::NotifyMemoryPressure() is a static method and it dispatches listeners owned by a singleton called g_observer. And what I'm going to do in this CL is to replace it with a callback. https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:39: using Notifier = base::Callback<void(MemoryPressureLevel level)>; On 2016/06/30 18:48:05, danakj wrote: > "Notifier" is the wrong direction here. The notifier is the monitor. This would > be a Listener or an Observer or the like, it receives notifications. The callback replaces MemoryPressureListener::NotifyMemoryPressure(), which notifies pressure level.
https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:15: // TODO(chrisha): Make this a concrete class with per-OS implementations rather On 2016/06/30 23:33:10, bashi1 wrote: > On 2016/06/30 18:48:05, danakj wrote: > > Is this something that will be done as part of this? Because on the list of > > things that make me uncomfortable about code, near the top is classes that are > > half abstract and half concrete. And that's where this goes now. > > I don't plan to do that. I'm not sure this still applies because we need to have > a way to change notifier from MemoryPressureListener to MemoryCoordinator which > we are going to implement. I think this comment is saying GetCurrentPressureLevel should not be virtual, we should just have it as a concrete method, and the definition is simply in different platform-specific places. I don't really want to leave us with this half-abstract class forever. So what should be the plan? https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:36: // Sets a notification callback. Once a callback is set, On 2016/06/30 23:33:10, bashi1 wrote: > On 2016/06/30 18:48:05, danakj wrote: > > I tried to get this from the doc, but I think maybe it's hidden below the > > surface of the larger design: > > - Why is it a callback instead of a listener interface? > We are implementing MemoryCoordinator which will replace MemoryPressureListener. > Currently MemoryPressureMonitor lives in base/ and MemoryCoordinator will live > in components/ so I use callback to avoid dependency violation. A virtual interface would do the same, where MemoryPressureMonitorObserver is an abstract class in base, and something in components implements it and passes that to the SetObserver method here. I prefer virtual interfaces to callbacks for things like this when they make sense (you don't need to bind state into it) because they are easier to find/understand in codesearch. > > - Why is there only a single listener instead of a set? > I'm afraid you may misunderstand what I'm going to do here. There are some > listeners actually. MemoryPressureListener::NotifyMemoryPressure() is a static > method and it dispatches listeners owned by a singleton called g_observer. And > what I'm going to do in this CL is to replace it with a callback. I see, so there may be multiple observers, but they would register with the thing that's listening to this class here. https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:39: using Notifier = base::Callback<void(MemoryPressureLevel level)>; On 2016/06/30 23:33:10, bashi1 wrote: > On 2016/06/30 18:48:05, danakj wrote: > > "Notifier" is the wrong direction here. The notifier is the monitor. This > would > > be a Listener or an Observer or the like, it receives notifications. > > The callback replaces MemoryPressureListener::NotifyMemoryPressure(), which > notifies pressure level. Yes, and the monitor notifies it by that, so it *is* the notifier. The Set here is coming from someone who wants to be notified, to receive notifications. So the relationship needs to be inverted in the naming. https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:40: void SetNotifier(const Notifier& notifier) { notifier_ = notifier; } Can we DCHECK that this is only set once? Or only set to/from null, but not clobbering a non-null with a non-null? Or do we need to support that?
https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:15: // TODO(chrisha): Make this a concrete class with per-OS implementations rather On 2016/07/01 20:49:28, danakj wrote: > On 2016/06/30 23:33:10, bashi1 wrote: > > On 2016/06/30 18:48:05, danakj wrote: > > > Is this something that will be done as part of this? Because on the list of > > > things that make me uncomfortable about code, near the top is classes that > are > > > half abstract and half concrete. And that's where this goes now. > > > > I don't plan to do that. I'm not sure this still applies because we need to > have > > a way to change notifier from MemoryPressureListener to MemoryCoordinator > which > > we are going to implement. > > I think this comment is saying GetCurrentPressureLevel should not be virtual, we > should just have it as a concrete method, and the definition is simply in > different platform-specific places. I don't really want to leave us with this > half-abstract class forever. So what should be the plan? An alternate could be to make the setter abstract and have each OS implement it.
Thank you for review and sorry for delay. https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:15: // TODO(chrisha): Make this a concrete class with per-OS implementations rather On 2016/07/01 20:50:09, danakj wrote: > On 2016/07/01 20:49:28, danakj wrote: > > On 2016/06/30 23:33:10, bashi1 wrote: > > > On 2016/06/30 18:48:05, danakj wrote: > > > > Is this something that will be done as part of this? Because on the list > of > > > > things that make me uncomfortable about code, near the top is classes that > > are > > > > half abstract and half concrete. And that's where this goes now. > > > > > > I don't plan to do that. I'm not sure this still applies because we need to > > have > > > a way to change notifier from MemoryPressureListener to MemoryCoordinator > > which > > > we are going to implement. > > > > I think this comment is saying GetCurrentPressureLevel should not be virtual, > we > > should just have it as a concrete method, and the definition is simply in > > different platform-specific places. I don't really want to leave us with this > > half-abstract class forever. So what should be the plan? > > An alternate could be to make the setter abstract and have each OS implement it. Could you elaborate what's the benefit of doing that and avoiding half abstraction? Do you want me to do refactoring before doing this kind of change? I'm not really interested in refactoring this class but want to have a way to change receiver of pressure notification. https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:36: // Sets a notification callback. Once a callback is set, On 2016/07/01 20:49:28, danakj wrote: > On 2016/06/30 23:33:10, bashi1 wrote: > > On 2016/06/30 18:48:05, danakj wrote: > > > I tried to get this from the doc, but I think maybe it's hidden below the > > > surface of the larger design: > > > - Why is it a callback instead of a listener interface? > > We are implementing MemoryCoordinator which will replace > MemoryPressureListener. > > Currently MemoryPressureMonitor lives in base/ and MemoryCoordinator will live > > in components/ so I use callback to avoid dependency violation. > > A virtual interface would do the same, where MemoryPressureMonitorObserver is an > abstract class in base, and something in components implements it and passes > that to the SetObserver method here. I prefer virtual interfaces to callbacks > for things like this when they make sense (you don't need to bind state into it) > because they are easier to find/understand in codesearch. That makes sense. Done. > > > > - Why is there only a single listener instead of a set? > > I'm afraid you may misunderstand what I'm going to do here. There are some > > listeners actually. MemoryPressureListener::NotifyMemoryPressure() is a static > > method and it dispatches listeners owned by a singleton called g_observer. > And > > what I'm going to do in this CL is to replace it with a callback. > > I see, so there may be multiple observers, but they would register with the > thing that's listening to this class here. https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:40: void SetNotifier(const Notifier& notifier) { notifier_ = notifier; } On 2016/07/01 20:49:28, danakj wrote: > Can we DCHECK that this is only set once? Or only set to/from null, but not > clobbering a non-null with a non-null? Or do we need to support that? Added DCHECK.
https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:15: // TODO(chrisha): Make this a concrete class with per-OS implementations rather On 2016/07/03 23:54:47, bashi1 wrote: > On 2016/07/01 20:50:09, danakj wrote: > > On 2016/07/01 20:49:28, danakj wrote: > > > On 2016/06/30 23:33:10, bashi1 wrote: > > > > On 2016/06/30 18:48:05, danakj wrote: > > > > > Is this something that will be done as part of this? Because on the list > > of > > > > > things that make me uncomfortable about code, near the top is classes > that > > > are > > > > > half abstract and half concrete. And that's where this goes now. > > > > > > > > I don't plan to do that. I'm not sure this still applies because we need > to > > > have > > > > a way to change notifier from MemoryPressureListener to MemoryCoordinator > > > which > > > > we are going to implement. > > > > > > I think this comment is saying GetCurrentPressureLevel should not be > virtual, > > we > > > should just have it as a concrete method, and the definition is simply in > > > different platform-specific places. I don't really want to leave us with > this > > > half-abstract class forever. So what should be the plan? > > > > An alternate could be to make the setter abstract and have each OS implement > it. > > Could you elaborate what's the benefit of doing that and avoiding half > abstraction? Do you want me to do refactoring before doing this kind of change? > I'm not really interested in refactoring this class but want to have a way to > change receiver of pressure notification. Half abstraction looks fine in the first CL, but as a class evolves over time, it gets increasingly murky where code should live, and trying to understand how it works. I've watched this happen to a few classes in chromium over time, and it has become a red flag for me. It would be a pretty simply change in this CL to avoid it by just making SetNotifier be a pure virtual method, and just write a defn for it in each subclass of this.
https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:15: // TODO(chrisha): Make this a concrete class with per-OS implementations rather On 2016/07/06 18:57:23, danakj wrote: > On 2016/07/03 23:54:47, bashi1 wrote: > > On 2016/07/01 20:50:09, danakj wrote: > > > On 2016/07/01 20:49:28, danakj wrote: > > > > On 2016/06/30 23:33:10, bashi1 wrote: > > > > > On 2016/06/30 18:48:05, danakj wrote: > > > > > > Is this something that will be done as part of this? Because on the > list > > > of > > > > > > things that make me uncomfortable about code, near the top is classes > > that > > > > are > > > > > > half abstract and half concrete. And that's where this goes now. > > > > > > > > > > I don't plan to do that. I'm not sure this still applies because we need > > to > > > > have > > > > > a way to change notifier from MemoryPressureListener to > MemoryCoordinator > > > > which > > > > > we are going to implement. > > > > > > > > I think this comment is saying GetCurrentPressureLevel should not be > > virtual, > > > we > > > > should just have it as a concrete method, and the definition is simply in > > > > different platform-specific places. I don't really want to leave us with > > this > > > > half-abstract class forever. So what should be the plan? > > > > > > An alternate could be to make the setter abstract and have each OS implement > > it. > > > > Could you elaborate what's the benefit of doing that and avoiding half > > abstraction? Do you want me to do refactoring before doing this kind of > change? > > I'm not really interested in refactoring this class but want to have a way to > > change receiver of pressure notification. > > Half abstraction looks fine in the first CL, but as a class evolves over time, > it gets increasingly murky where code should live, and trying to understand how > it works. I've watched this happen to a few classes in chromium over time, and > it has become a red flag for me. > > It would be a pretty simply change in this CL to avoid it by just making > SetNotifier be a pure virtual method, and just write a defn for it in each > subclass of this. The Notify() method could just be a concrete method in each subclass and would not need to be part of this API (an example of how mixing concrete and abstract goes wrong IMO, the real public API gets unclear). And it would only need to be public for the Mac version?
Patchset #3 (id:100001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:15: // TODO(chrisha): Make this a concrete class with per-OS implementations rather I'm not fully convinced but updated the CL as you suggested (dup of code for non-planned future extension seems premature to me). I personally feel the previous CL looks better but I want to land this as soon as possible.
Yeah, I'm not convinced this is any cleaner. The whole class hierarchy is effectively broken, as it actually doesn't *need* to be an abstract base class, with different implementations. It actually should be a single concrete class with some different implementation per platform, and the mechanism that bashi is trying to add should be part of that concrete class (and shared across all platforms). Since this is currently being rejigged in the memory_coordinator client, and the mechanism introduced by this CL is only temporary in order to permit testing and experimentation, maybe allowing the mixed implementation/interface is fine for now? I wouldn't want anyone to spend a bunch of time cleaning something up that is going to disappear. I'm also not 100% convinced of the 'Observer' naming. It's more of a delegate for dispatching the notifications. Maybe NotificationDispatchDelegate? (I also dislike the conditional logic that sends to the 'observer' if present, otherwise calls the existing notification mechanism. A delegate would simplify this logic.) https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:15: // TODO(chrisha): Make this a concrete class with per-OS implementations rather On 2016/07/01 20:49:28, danakj wrote: > On 2016/06/30 23:33:10, bashi1 wrote: > > On 2016/06/30 18:48:05, danakj wrote: > > > Is this something that will be done as part of this? Because on the list of > > > things that make me uncomfortable about code, near the top is classes that > are > > > half abstract and half concrete. And that's where this goes now. > > > > I don't plan to do that. I'm not sure this still applies because we need to > have > > a way to change notifier from MemoryPressureListener to MemoryCoordinator > which > > we are going to implement. > > I think this comment is saying GetCurrentPressureLevel should not be virtual, we > should just have it as a concrete method, and the definition is simply in > different platform-specific places. I don't really want to leave us with this > half-abstract class forever. So what should be the plan? That's exactly what the comment is saying. MemoryPressureMonitor is simply going to disappear in its current incarnation, being replaced with equivalent functionality in the memory_coordinator component. This CL is mostly about putting in place a seam to intercept the existing MP messages and to route them via the new MC system, which is in progress. I don't think it's worth addressing the comment in this code, as the component refactor will take care of it over there. https://codereview.chromium.org/2097753002/diff/160001/base/memory/memory_pre... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/160001/base/memory/memory_pre... base/memory/memory_pressure_monitor.h:39: virtual void SetObserver(MemoryPressureMonitorObserver* observer) = 0; This is more than just an observer. It actually takes the role of the dispatch, and causes the dispatch mechanism to change. Maybe simply define a DispatchCallback type, and provide a 'set_dispatch_callback', which defaults as being bound to MPL::NotifyMemoryPressure? It feels like an observer is the wrong term, and a dedicated class a little overkill.
https://codereview.chromium.org/2097753002/diff/160001/base/memory/memory_pre... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/160001/base/memory/memory_pre... base/memory/memory_pressure_monitor.h:39: virtual void SetObserver(MemoryPressureMonitorObserver* observer) = 0; On 2016/07/07 03:09:19, chrisha (slow) wrote: > This is more than just an observer. It actually takes the role of the dispatch, > and causes the dispatch mechanism to change. > > Maybe simply define a DispatchCallback type, and provide a > 'set_dispatch_callback', which defaults as being bound to > MPL::NotifyMemoryPressure? > > It feels like an observer is the wrong term, and a dedicated class a little > overkill. Makes sense. Will do that if danakj@ is fine.
> Yeah, I'm not convinced this is any cleaner. The whole class hierarchy is > effectively broken, as it actually doesn't *need* to be an abstract base class, > with different implementations. It actually should be a single concrete class > with some different implementation per platform, and the mechanism that bashi is > trying to add should be part of that concrete class (and shared across all > platforms). I don't disagree, it should not be an abstract base class. But it currently is, and if we're not making it a concrete class, I don't want to mix abstract and concrete APIs in the same class. I totally would like us to make this not virtual entirely as the TODO points toward in the top of the header file. So, I prefer the way it is unless we're also doing the work to make the rest of the API not virtual. https://codereview.chromium.org/2097753002/diff/160001/base/memory/memory_pre... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/160001/base/memory/memory_pre... base/memory/memory_pressure_monitor.h:39: virtual void SetObserver(MemoryPressureMonitorObserver* observer) = 0; On 2016/07/07 03:32:32, bashi1 wrote: > On 2016/07/07 03:09:19, chrisha (slow) wrote: > > This is more than just an observer. It actually takes the role of the > dispatch, > > and causes the dispatch mechanism to change. > > > > Maybe simply define a DispatchCallback type, and provide a > > 'set_dispatch_callback', which defaults as being bound to > > MPL::NotifyMemoryPressure? > > > > It feels like an observer is the wrong term, and a dedicated class a little > > overkill. > > Makes sense. Will do that if danakj@ is fine. Ya that sounds good to me, and is a good reason for using a Callback instead of a virtual interface then.
Thanks for feedback. Revised the CL. PTAL. https://codereview.chromium.org/2097753002/diff/160001/base/memory/memory_pre... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/160001/base/memory/memory_pre... base/memory/memory_pressure_monitor.h:39: virtual void SetObserver(MemoryPressureMonitorObserver* observer) = 0; On 2016/07/07 23:12:06, danakj wrote: > On 2016/07/07 03:32:32, bashi1 wrote: > > On 2016/07/07 03:09:19, chrisha (slow) wrote: > > > This is more than just an observer. It actually takes the role of the > > dispatch, > > > and causes the dispatch mechanism to change. > > > > > > Maybe simply define a DispatchCallback type, and provide a > > > 'set_dispatch_callback', which defaults as being bound to > > > MPL::NotifyMemoryPressure? > > > > > > It feels like an observer is the wrong term, and a dedicated class a little > > > overkill. > > > > Makes sense. Will do that if danakj@ is fine. > > Ya that sounds good to me, and is a good reason for using a Callback instead of > a virtual interface then. Done.
LGTM except maybe a bug in mac? https://codereview.chromium.org/2097753002/diff/180001/base/memory/memory_pre... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/180001/base/memory/memory_pre... base/memory/memory_pressure_monitor.h:26: using DispatchCallback = void(*)(MemoryPressureLevel level); oh I was expecting a base::Callback, but if you don't need that for the new thing then this is fine too. https://codereview.chromium.org/2097753002/diff/180001/base/memory/memory_pre... File base/memory/memory_pressure_monitor_chromeos.h (right): https://codereview.chromium.org/2097753002/diff/180001/base/memory/memory_pre... base/memory/memory_pressure_monitor_chromeos.h:64: void SetDispatchCallback(DispatchCallback callback) override; nit: normally all overrides for a class are grouped into one block (no whitespace), with a "ThisOtherClass implementation." comment or the like on the top of them. https://codereview.chromium.org/2097753002/diff/180001/base/memory/memory_pre... File base/memory/memory_pressure_monitor_mac.cc (right): https://codereview.chromium.org/2097753002/diff/180001/base/memory/memory_pre... base/memory/memory_pressure_monitor_mac.cc:55: dispatch_callback_); does this not bind the current callback forever? maybe you want to bind a pointer to the callback? not sure how ^{} works.
https://codereview.chromium.org/2097753002/diff/180001/base/memory/memory_pre... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/2097753002/diff/180001/base/memory/memory_pre... base/memory/memory_pressure_monitor.h:26: using DispatchCallback = void(*)(MemoryPressureLevel level); On 2016/07/08 00:03:53, danakj wrote: > oh I was expecting a base::Callback, but if you don't need that for the new > thing then this is fine too. Ah, good point. I have a WIP CL locally and a function pointer is enough for the CL, but I noticed that it would need to be a callback after some changes. I'll change DispatchCallback to a base::Callback. https://codereview.chromium.org/2097753002/diff/180001/base/memory/memory_pre... File base/memory/memory_pressure_monitor_chromeos.h (right): https://codereview.chromium.org/2097753002/diff/180001/base/memory/memory_pre... base/memory/memory_pressure_monitor_chromeos.h:64: void SetDispatchCallback(DispatchCallback callback) override; On 2016/07/08 00:03:53, danakj wrote: > nit: normally all overrides for a class are grouped into one block (no > whitespace), with a "ThisOtherClass implementation." comment or the like on the > top of them. Acknowledged. https://codereview.chromium.org/2097753002/diff/180001/base/memory/memory_pre... File base/memory/memory_pressure_monitor_mac.cc (right): https://codereview.chromium.org/2097753002/diff/180001/base/memory/memory_pre... base/memory/memory_pressure_monitor_mac.cc:55: dispatch_callback_); On 2016/07/08 00:03:53, danakj wrote: > does this not bind the current callback forever? maybe you want to bind a > pointer to the callback? not sure how ^{} works. Not sure how blocks work too but seems a bug. I'll check it.
Bots seem happy. Let me land this. https://codereview.chromium.org/2097753002/diff/240001/base/memory/memory_pre... File base/memory/memory_pressure_monitor_mac.cc (right): https://codereview.chromium.org/2097753002/diff/240001/base/memory/memory_pre... base/memory/memory_pressure_monitor_mac.cc:56: dispatch_source_set_event_handler(memory_level_event_source_, ^{ I read [1] and it seems that we can use member variables here as the same as in a member function. I think this is OK. [1] http://clang.llvm.org/docs/BlockLanguageSpec.html
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org, halliwell@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2097753002/#ps240001 (title: "Fix another error")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, chrisha@chromium.org, halliwell@chromium.org Link to the patchset: https://codereview.chromium.org/2097753002/#ps260001 (title: "Fix win")
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.
Description was changed from ========== Make memory pressure notifier configurable In memory coordinator v0 we try to replace MemoryPressureListener with MemoryCoordinatorClient as described in [1]. This CL adds SetNotifier() method which takes a callback for dispatching memory pressure notifications. This allows us to replace MemoryPressureListener::NotifyMemoryPressure() with an arbitrary function. [1] https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI... BUG=617492 ========== to ========== Make memory pressure notifier configurable In memory coordinator v0 we try to replace MemoryPressureListener with MemoryCoordinatorClient as described in [1]. This CL adds SetNotifier() method which takes a callback for dispatching memory pressure notifications. This allows us to replace MemoryPressureListener::NotifyMemoryPressure() with an arbitrary function. [1] https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI... BUG=617492 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:260001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Make memory pressure notifier configurable In memory coordinator v0 we try to replace MemoryPressureListener with MemoryCoordinatorClient as described in [1]. This CL adds SetNotifier() method which takes a callback for dispatching memory pressure notifications. This allows us to replace MemoryPressureListener::NotifyMemoryPressure() with an arbitrary function. [1] https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI... BUG=617492 ========== to ========== Make memory pressure notifier configurable In memory coordinator v0 we try to replace MemoryPressureListener with MemoryCoordinatorClient as described in [1]. This CL adds SetNotifier() method which takes a callback for dispatching memory pressure notifications. This allows us to replace MemoryPressureListener::NotifyMemoryPressure() with an arbitrary function. [1] https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI... BUG=617492 Committed: https://crrev.com/456eef44d60c8cddc72f53d4e3d13ffd80158df3 Cr-Commit-Position: refs/heads/master@{#404327} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/456eef44d60c8cddc72f53d4e3d13ffd80158df3 Cr-Commit-Position: refs/heads/master@{#404327}
Message was sent while issue was closed.
On Thu, Jul 7, 2016 at 6:58 PM, <bashi@chromium.org> wrote: > Bots seem happy. Let me land this. > > > > https://codereview.chromium.org/2097753002/diff/240001/base/memory/memory_pre... > File base/memory/memory_pressure_monitor_mac.cc (right): > > > https://codereview.chromium.org/2097753002/diff/240001/base/memory/memory_pre... > base/memory/memory_pressure_monitor_mac.cc:56: > dispatch_source_set_event_handler(memory_level_event_source_, ^{ > I read [1] and it seems that we can use member variables here as the > same as in a member function. I think this is OK. > > [1] http://clang.llvm.org/docs/BlockLanguageSpec.html > Thanksfor the pointer. "References to this, as well as references to non-static members of any enclosing class, are evaluated by capturing this just like a normal variable of C pointer type." does sound like it's okay! > > > https://codereview.chromium.org/2097753002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
