Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1312163003:
Add method for suppressing MemoryPressureListener notifications (Closed)
|
Created:
5 years, 4 months ago by petrcermak Modified:
5 years, 3 months ago CC:
chromium-reviews, gavinp+memory_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd method for suppressing MemoryPressureListener notifications
This patch adds a new static method to MemoryPressureListener which can
be used to start/stop suppressing memory pressure notifications:
// Start suppressing memory pressure notifications.
MemoryPressureListener::SetNotificationsSuppressed(true);
// Stop suppressing memory pressure notifications.
MemoryPressureListener::SetNotificationsSuppressed(false);
This patch represents the first step towards implementing a DevTools API
for suppressing and simulating memory pressure signals in Chrome. The
main use case for this feature is to enforce consistent conditions
across memory measurements. See https://goo.gl/cZFdH3 for more details.
BUG=516776
Committed: https://crrev.com/da4982addfa8b6a5e25b794565c6c7158c1ec8cc
Cr-Commit-Position: refs/heads/master@{#347622}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Use Atomic32 instead of Lock #
Total comments: 12
Patch Set 3 : Address rmcilroy's comments #
Total comments: 6
Patch Set 4 : Address primiano's comments #
Total comments: 6
Patch Set 5 : Address thestig's comments #
Messages
Total messages: 27 (6 generated)
petrcermak@chromium.org changed reviewers: + primiano@chromium.org
Please have a quick look at this before I send it out for a full review. Thanks, Petr
https://codereview.chromium.org/1312163003/diff/1/base/memory/memory_pressure... File base/memory/memory_pressure_listener.cc (right): https://codereview.chromium.org/1312163003/diff/1/base/memory/memory_pressure... base/memory/memory_pressure_listener.cc:64: AutoLock lock(g_suppressed_lock.Get()); Hmm looking now at the code the code looks really overkill. Check with rmcilroy who, IIRC, is an OWNER here. I think that it's fine to keep it a plain bool. In theory you could see a memory pressure signal if it happens immediately after the Set. Realistically it won't ever happen and if it happens won't make a huge difference.
petrcermak@chromium.org changed reviewers: + rmcilroy@chromium.org
rmcilroy: Could you please have a look at this? Thanks, Petr
https://codereview.chromium.org/1312163003/diff/1/base/memory/memory_pressure... File base/memory/memory_pressure_listener.cc (right): https://codereview.chromium.org/1312163003/diff/1/base/memory/memory_pressure... base/memory/memory_pressure_listener.cc:64: AutoLock lock(g_suppressed_lock.Get()); On 2015/08/26 09:52:23, Primiano Tucci wrote: > Hmm looking now at the code the code looks really overkill. > Check with rmcilroy who, IIRC, is an OWNER here. > I think that it's fine to keep it a plain bool. > In theory you could see a memory pressure signal if it happens immediately after > the Set. Realistically it won't ever happen and if it happens won't make a huge > difference. I don't think it's fine to have it as a plain bool if it's going to be accessed from multiple threads, but I do agree the lock here is not ideal. Does this really need to be set from any thread? Couldn't you just post a task to the same thread which performs the memory pressure notifications to update this flag when required, such that it's always updated / accessed from the same thread?
https://codereview.chromium.org/1312163003/diff/1/base/memory/memory_pressure... File base/memory/memory_pressure_listener.cc (right): https://codereview.chromium.org/1312163003/diff/1/base/memory/memory_pressure... base/memory/memory_pressure_listener.cc:64: AutoLock lock(g_suppressed_lock.Get()); On 2015/08/26 10:27:19, rmcilroy wrote: > On 2015/08/26 09:52:23, Primiano Tucci wrote: > > Hmm looking now at the code the code looks really overkill. > > Check with rmcilroy who, IIRC, is an OWNER here. > > I think that it's fine to keep it a plain bool. > > In theory you could see a memory pressure signal if it happens immediately > after > > the Set. Realistically it won't ever happen and if it happens won't make a > huge > > difference. > > I don't think it's fine to have it as a plain bool if it's going to be accessed > from multiple threads, but I do agree the lock here is not ideal. > > Does this really need to be set from any thread? Couldn't you just post a task > to the same thread which performs the memory pressure notifications to update > this flag when required, such that it's always updated / accessed from the same > thread? That seems like an even more heavyweight solution. Moreover, the notifications can come from any thread within the process (that's why the observers are in a global ObserverListThreadSafe object).
On 2015/08/26 12:47:03, petrcermak wrote: > https://codereview.chromium.org/1312163003/diff/1/base/memory/memory_pressure... > File base/memory/memory_pressure_listener.cc (right): > > https://codereview.chromium.org/1312163003/diff/1/base/memory/memory_pressure... > base/memory/memory_pressure_listener.cc:64: AutoLock > lock(g_suppressed_lock.Get()); > On 2015/08/26 10:27:19, rmcilroy wrote: > > On 2015/08/26 09:52:23, Primiano Tucci wrote: > > > Hmm looking now at the code the code looks really overkill. > > > Check with rmcilroy who, IIRC, is an OWNER here. > > > I think that it's fine to keep it a plain bool. > > > In theory you could see a memory pressure signal if it happens immediately > > after > > > the Set. Realistically it won't ever happen and if it happens won't make a > > huge > > > difference. > > > > I don't think it's fine to have it as a plain bool if it's going to be > accessed > > from multiple threads, but I do agree the lock here is not ideal. > > > > Does this really need to be set from any thread? Couldn't you just post a task > > to the same thread which performs the memory pressure notifications to update > > this flag when required, such that it's always updated / accessed from the > same > > thread? > > That seems like an even more heavyweight solution. Moreover, the notifications > can come from any thread within the process (that's why the observers are in a > global ObserverListThreadSafe object). I don't think that's the case. The observers are in ObserverListThreadSafe because the listeners could be on any thread and that ensures that the listeners get notified on the thread which they registered on. The notifications always come from the same thread I believe (although it might be a different thread on Android than it is on other platforms). My original question still stands - do you need to be able to call this from any thread? If not you can just ensure it is always called from the same thread as the notifications are and you won't need the postTask either.
As discussed offline, I changed the code to use Atomic32 instead of Lock. PTAL. Thanks, Petr
https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... File base/memory/memory_pressure_listener.cc (right): https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.cc:61: if (subtle::NoBarrier_Load(&g_suppressed) == 1) It probably doesn't matter here given the surrounding code, but for the semantics you are trying to get I think you want Acquire_Load here. https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.cc:61: if (subtle::NoBarrier_Load(&g_suppressed) == 1) Could you pull this out into a separate IsNotificationsSuppressed function please. https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.cc:68: void MemoryPressureListener::SetNotificationsSuppressed(bool suppressed) { SetSuppressNotifications https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.cc:69: subtle::NoBarrier_Store(&g_suppressed, suppressed ? 1 : 0); And Release_Store here. https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.h:75: // Intended to maintain stable conditions across memory measurements. Please add a warning that this shouldn't be called by any code except for the Memory performance measurement code. Could we also enforce it in some way (e.g., DCHECK on a particular command-line flag being set in SetNotificationsSuppressed)?
Thanks for your comments. PTAL. Petr https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... File base/memory/memory_pressure_listener.cc (right): https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.cc:61: if (subtle::NoBarrier_Load(&g_suppressed) == 1) On 2015/08/28 15:08:06, rmcilroy wrote: > It probably doesn't matter here given the surrounding code, but for the > semantics you are trying to get I think you want Acquire_Load here. Done. I called the method "IsSuppressNotifications" to match "SetSuppressNotifications", but the name is kind of strange. Let me know if you have any better suggestions. https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.cc:61: if (subtle::NoBarrier_Load(&g_suppressed) == 1) On 2015/08/28 15:08:06, rmcilroy wrote: > It probably doesn't matter here given the surrounding code, but for the > semantics you are trying to get I think you want Acquire_Load here. Done. https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.cc:68: void MemoryPressureListener::SetNotificationsSuppressed(bool suppressed) { On 2015/08/28 15:08:06, rmcilroy wrote: > SetSuppressNotifications Done. https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.cc:69: subtle::NoBarrier_Store(&g_suppressed, suppressed ? 1 : 0); On 2015/08/28 15:08:07, rmcilroy wrote: > And Release_Store here. Done. https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.h:75: // Intended to maintain stable conditions across memory measurements. On 2015/08/28 15:08:07, rmcilroy wrote: > Please add a warning that this shouldn't be called by any code except for the > Memory performance measurement code. Could we also enforce it in some way (e.g., > DCHECK on a particular command-line flag being set in > SetNotificationsSuppressed)? Done (warning). We currently use "--enable-memory-benchmarking" in a few places, but it's a temporary hack (it's a content flag and we are using it in base). We don't have any other flags at the moment.
https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.h:75: // Intended to maintain stable conditions across memory measurements. Did you do this, or did you mean you can't do it because "--enable-memory-benchmarking" is going away?
https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/1312163003/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.h:75: // Intended to maintain stable conditions across memory measurements. On 2015/08/28 16:40:29, rmcilroy wrote: > Did you do this, or did you mean you can't do it because > "--enable-memory-benchmarking" is going away? I added the warning, but I didn't add the flag. We use it only as a temporary hack that will become unnecessary in the near future. We generally try to not depend on command-line flags in memory infra. Nevertheless, I can add it now if you want and we can decide about it later.
https://codereview.chromium.org/1312163003/diff/40001/base/memory/memory_pres... File base/memory/memory_pressure_listener.cc (right): https://codereview.chromium.org/1312163003/diff/40001/base/memory/memory_pres... base/memory/memory_pressure_listener.cc:38: g_suppress_notifications ? https://codereview.chromium.org/1312163003/diff/40001/base/memory/memory_pres... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/1312163003/diff/40001/base/memory/memory_pres... base/memory/memory_pressure_listener.h:79: static void SetSuppressNotifications(bool suppressed); Not sure whether this was discussed in the previous patchsets but, if there is no other opinion, I'd vote for AreNotificationsSuppressed() here :) Ignore if already discussed in another patchset (apologies for the laziness, reviewing from a tablet is hard++) https://codereview.chromium.org/1312163003/diff/40001/base/memory/memory_pres... File base/memory/memory_pressure_listener_unittest.cc (right): https://codereview.chromium.org/1312163003/diff/40001/base/memory/memory_pres... base/memory/memory_pressure_listener_unittest.cc:32: Instead of waiting for idle (which can become unreliable if the something in the production code decides to delay the notification) you could PostTask a message_loop.QuitClosure() when the mock method is invoked (see .Invoke() from Gtest, see recent ruuda@ patchsets where he posted the task from the mock). In this way, you either: -get the callback AND the then exit the messageloop if everything goes right -timeout (no callback == no quitclosure posted), hence fail, if the callback does not arrive. IMHO that would make the test a bit more reliable and future proof
Thanks for your comments. PTAL. Petr https://codereview.chromium.org/1312163003/diff/40001/base/memory/memory_pres... File base/memory/memory_pressure_listener.cc (right): https://codereview.chromium.org/1312163003/diff/40001/base/memory/memory_pres... base/memory/memory_pressure_listener.cc:38: On 2015/08/30 18:40:51, Primiano Tucci wrote: > g_suppress_notifications ? Renamed to g_notifications_suppressed to be in line with the getter/setter names. https://codereview.chromium.org/1312163003/diff/40001/base/memory/memory_pres... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/1312163003/diff/40001/base/memory/memory_pres... base/memory/memory_pressure_listener.h:79: static void SetSuppressNotifications(bool suppressed); On 2015/08/30 18:40:51, Primiano Tucci wrote: > Not sure whether this was discussed in the previous patchsets but, if there is > no other opinion, I'd vote for AreNotificationsSuppressed() here :) > Ignore if already discussed in another patchset (apologies for the laziness, > reviewing from a tablet is hard++) Done. https://codereview.chromium.org/1312163003/diff/40001/base/memory/memory_pres... File base/memory/memory_pressure_listener_unittest.cc (right): https://codereview.chromium.org/1312163003/diff/40001/base/memory/memory_pres... base/memory/memory_pressure_listener_unittest.cc:32: On 2015/08/30 18:40:52, Primiano Tucci wrote: > Instead of waiting for idle (which can become unreliable if the something in the > production code decides to delay the notification) you could PostTask a > message_loop.QuitClosure() when the mock method is invoked (see .Invoke() from > Gtest, see recent ruuda@ patchsets where he posted the task from the mock). > In this way, you either: > -get the callback AND the then exit the messageloop if everything goes right > -timeout (no callback == no quitclosure posted), hence fail, if the callback > does not arrive. > > IMHO that would make the test a bit more reliable and future proof As discussed offline, we'll stick with the current implementation for the time being because it is more readable and can be used to check lack of notifications without a timeout.
lgtm (you'll need to get someone who own's base/ though, I only own base/android).
petrcermak@chromium.org changed reviewers: + mark@chromium.org
Hi Mark, could you please review this patch? Thanks, Petr
primiano@chromium.org changed reviewers: + thestig@chromium.org - mark@chromium.org
+thestig for base/ OWNERS stamp
lgtm https://codereview.chromium.org/1312163003/diff/60001/base/memory/memory_pres... File base/memory/memory_pressure_listener_unittest.cc (right): https://codereview.chromium.org/1312163003/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_listener_unittest.cc:12: namespace { nit: not needed for using statements? https://codereview.chromium.org/1312163003/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_listener_unittest.cc:22: scoped_ptr<MemoryPressureListener> listener(new MemoryPressureListener( |listener| has a reference to |callback| above it, but |callback| will get destroyed first. It's probably ok as is, but if you explicitly reset() |listener| before it goes out of scope, it'll feel safer. https://codereview.chromium.org/1312163003/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_listener_unittest.cc:23: base::Bind(&MockCallback::Call, base::Unretained(&callback)))); nit: no need for base:: inside namespace base.
Thanks for your comments. Petr https://codereview.chromium.org/1312163003/diff/60001/base/memory/memory_pres... File base/memory/memory_pressure_listener_unittest.cc (right): https://codereview.chromium.org/1312163003/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_listener_unittest.cc:12: namespace { On 2015/09/03 17:46:33, Lei Zhang wrote: > nit: not needed for using statements? Done. https://codereview.chromium.org/1312163003/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_listener_unittest.cc:22: scoped_ptr<MemoryPressureListener> listener(new MemoryPressureListener( On 2015/09/03 17:46:33, Lei Zhang wrote: > |listener| has a reference to |callback| above it, but |callback| will get > destroyed first. It's probably ok as is, but if you explicitly reset() > |listener| before it goes out of scope, it'll feel safer. Done. https://codereview.chromium.org/1312163003/diff/60001/base/memory/memory_pres... base/memory/memory_pressure_listener_unittest.cc:23: base::Bind(&MockCallback::Call, base::Unretained(&callback)))); On 2015/09/03 17:46:33, Lei Zhang wrote: > nit: no need for base:: inside namespace base. Done.
The CQ bit was checked by petrcermak@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1312163003/#ps80001 (title: "Address thestig's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1312163003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1312163003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/da4982addfa8b6a5e25b794565c6c7158c1ec8cc Cr-Commit-Position: refs/heads/master@{#347622} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
