Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2335933003:
Make DOMStorageContextWrapper a client of memory coordinator (Closed)
|
Created:
4 years, 3 months ago by hajimehoshi Modified:
4 years, 3 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake DOMStorageContextWrapper a client of memory coordinator
When the memory coordinator is enabled, DOMStorageContextWrapper becomes
a client of the memory coordinator instead of installing
MemoryPressureListener.
Design Doc: https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8Uo/edit?ts=57d7968b#
BUG=639700
Committed: https://crrev.com/a517a220c9961d15c4f045ff3cabe03fa8e9af2c
Cr-Commit-Position: refs/heads/master@{#419692}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address on reviews #
Total comments: 8
Patch Set 3 : Address on reviews #Patch Set 4 : (rebase) #
Total comments: 1
Patch Set 5 : Use FeatureList::IsEnabled #
Total comments: 5
Patch Set 6 : Address on ssid's review #
Depends on Patchset: Messages
Total messages: 37 (19 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: + bashi@google.com, chrisha@chromium.org, michaeln@chromium.org
PTAL
bashi@chromium.org changed reviewers: + bashi@chromium.org, ssid@chromium.org
+ssid@ who added a MPL to DOMStrageContextWrapper.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_wrapper.cc:397: PurgeMemory(base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL); Why are we always purging aggressively here? shouldn't this pass the correct pressure level? Also PurgeMemory takes a boolean now right? https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_wrapper.cc:402: PurgeMemory(state == memory_coordinator::MemoryState::SUSPENDED); The memory coordinator document states that the "suspended" state can only be applied to backgrounded renderer. How does a suspended browser process make sense? The doc does not explain about browser as far as I see. Can this method be called with "Discarded" or "Throttled"? Why do we purge non-aggressively when called with "Discarded"? Also why are we purging for a state of "Unthrottled"?
https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_wrapper.cc:297: if (memory_coordinator::MemoryCoordinator::GetInstance()) { Register() / Unregister() need to be called on the same thread but the dtor is not guaranteed to be called on the same thread as the ctor. Please move the call to Unregister() into the Shutdown() method so everything happens on the UI thread. https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_wrapper.cc:401: memory_coordinator::MemoryState state) { enum class MemoryState : int32_t { UNKNOWN = -1, NORMAL = 0, THROTTLED = 1, SUSPENDED = 2, }; What do those states mean? Is there a .mojom or .h file somewhere that defines describes that. https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_wrapper.cc:405: void DOMStorageContextWrapper::PurgeMemory(bool aggressively) might be cleaner to have the helper take a DOMStorageContextImpl::PurgeOption as input and for each of the OnMemory methods to translate to one of the three options https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_context_wrapper.h (right): https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_wrapper.h:83: // memory_coordinator::MemoryCoordinatorClient implementation: while your here, can u make all these OnMemory related methods private?
https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_wrapper.cc:401: memory_coordinator::MemoryState state) { On 2016/09/14 01:34:41, michaeln wrote: > enum class MemoryState : int32_t { > UNKNOWN = -1, > NORMAL = 0, > THROTTLED = 1, > SUSPENDED = 2, > }; > > What do those states mean? Is there a .mojom or .h file somewhere that defines > describes that. Sorry for lack of explanation. I'll add comments in .mojom to describe what they mean.
Thank you! https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_wrapper.cc:297: if (memory_coordinator::MemoryCoordinator::GetInstance()) { On 2016/09/14 01:34:41, michaeln wrote: > Register() / Unregister() need to be called on the same thread but the dtor is > not guaranteed to be called on the same thread as the ctor. > > Please move the call to Unregister() into the Shutdown() method so everything > happens on the UI thread. Done. https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_wrapper.cc:397: PurgeMemory(base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL); On 2016/09/13 18:29:23, ssid wrote: > Why are we always purging aggressively here? shouldn't this pass the correct > pressure level? > Also PurgeMemory takes a boolean now right? Done. https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_wrapper.cc:402: PurgeMemory(state == memory_coordinator::MemoryState::SUSPENDED); On 2016/09/13 18:29:23, ssid wrote: > The memory coordinator document states that the "suspended" state can only be > applied to backgrounded renderer. How does a suspended browser process make > sense? The doc does not explain about browser as far as I see. I realized 'suspended' state can't come to browser components so far. I added assertions to verify this. > > Can this method be called with "Discarded" or "Throttled"? Why do we purge > non-aggressively when called with "Discarded"? There is not 'Discarded' state enum value. I added code to purge memory for 'Throttled'. > > Also why are we purging for a state of "Unthrottled"? Fixed. https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_wrapper.cc:405: void DOMStorageContextWrapper::PurgeMemory(bool aggressively) On 2016/09/14 01:34:41, michaeln wrote: > might be cleaner to have the helper take a DOMStorageContextImpl::PurgeOption as > input and for each of the OnMemory methods to translate to one of the three > options I don't think that is possible since DOMStorageContext.h can't refer DOMStorageContextImpl.h. https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_context_wrapper.h (right): https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_wrapper.h:83: // memory_coordinator::MemoryCoordinatorClient implementation: On 2016/09/14 01:34:41, michaeln wrote: > while your here, can u make all these OnMemory related methods private? Done.
https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_wrapper.cc:405: void DOMStorageContextWrapper::PurgeMemory(bool aggressively) On 2016/09/14 11:49:48, hajimehoshi wrote: > On 2016/09/14 01:34:41, michaeln wrote: > > might be cleaner to have the helper take a DOMStorageContextImpl::PurgeOption > as > > input and for each of the OnMemory methods to translate to one of the three > > options > > I don't think that is possible since DOMStorageContext.h can't refer > DOMStorageContextImpl.h. Maybe i'm missing something? This method is a private method of DOMStorageContextWrapper, the class is not in the content/public api and the method is not an override of a content/public api. So i think content/browser/dom_storage_content_wrapper.h can include content/browser/dom_storage_content_impl.h no problem. https://codereview.chromium.org/2335933003/diff/20001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2335933003/diff/20001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.cc:407: case memory_coordinator::MemoryState::NORMAL: style nit: indent cases by 2 spaces https://codereview.chromium.org/2335933003/diff/20001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.cc:417: NOTREACHED(); maybe comment that SUSPENDED doesn't occur in the main browser process https://codereview.chromium.org/2335933003/diff/20001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.cc:423: { style nit: move { up to the end of the previous line https://codereview.chromium.org/2335933003/diff/20001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_context_wrapper.h (right): https://codereview.chromium.org/2335933003/diff/20001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.h:87: friend class DOMStorageMessageFilter; // for access to context() nit: it'd be good to keep the two OnMem and PurgeMem methods together, also plz keep the friend dcls up at the top of the private section, thnx
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...
Thank you! https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_wrapper.cc:405: void DOMStorageContextWrapper::PurgeMemory(bool aggressively) On 2016/09/14 19:40:48, michaeln wrote: > On 2016/09/14 11:49:48, hajimehoshi wrote: > > On 2016/09/14 01:34:41, michaeln wrote: > > > might be cleaner to have the helper take a > DOMStorageContextImpl::PurgeOption > > as > > > input and for each of the OnMemory methods to translate to one of the three > > > options > > > > I don't think that is possible since DOMStorageContext.h can't refer > > DOMStorageContextImpl.h. > > Maybe i'm missing something? This method is a private method of > DOMStorageContextWrapper, the class is not in the content/public api and the > method is not an override of a content/public api. > > So i think content/browser/dom_storage_content_wrapper.h can include > content/browser/dom_storage_content_impl.h no problem. Sorry but I was misunderstanding. Instead of adding a helper function, I've just changed the argument from bool to PurgeOption. https://codereview.chromium.org/2335933003/diff/20001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2335933003/diff/20001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.cc:407: case memory_coordinator::MemoryState::NORMAL: On 2016/09/14 19:40:49, michaeln wrote: > style nit: indent cases by 2 spaces Done. https://codereview.chromium.org/2335933003/diff/20001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.cc:417: NOTREACHED(); On 2016/09/14 19:40:49, michaeln wrote: > maybe comment that SUSPENDED doesn't occur in the main browser process Done. https://codereview.chromium.org/2335933003/diff/20001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.cc:423: { On 2016/09/14 19:40:49, michaeln wrote: > style nit: move { up to the end of the previous line Done. https://codereview.chromium.org/2335933003/diff/20001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_context_wrapper.h (right): https://codereview.chromium.org/2335933003/diff/20001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.h:87: friend class DOMStorageMessageFilter; // for access to context() On 2016/09/14 19:40:49, michaeln wrote: > nit: it'd be good to keep the two OnMem and PurgeMem methods together, also plz > keep the friend dcls up at the top of the private section, thnx Done. https://codereview.chromium.org/2335933003/diff/60001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2335933003/diff/60001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.cc:289: if (MemoryCoordinator::GetInstance()) { bashi: Is this a correct usage to check whether MemoryCoordinator is enabled or not?
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.
this lgtm if it lgt ssid https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2335933003/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_context_wrapper.cc:405: void DOMStorageContextWrapper::PurgeMemory(bool aggressively) On 2016/09/15 09:14:46, hajimehoshi wrote: > On 2016/09/14 19:40:48, michaeln wrote: > > On 2016/09/14 11:49:48, hajimehoshi wrote: > > > On 2016/09/14 01:34:41, michaeln wrote: > > > > might be cleaner to have the helper take a > > DOMStorageContextImpl::PurgeOption > > > as > > > > input and for each of the OnMemory methods to translate to one of the > three > > > > options > > > > > > I don't think that is possible since DOMStorageContext.h can't refer > > > DOMStorageContextImpl.h. > > > > Maybe i'm missing something? This method is a private method of > > DOMStorageContextWrapper, the class is not in the content/public api and the > > method is not an override of a content/public api. > > > > So i think content/browser/dom_storage_content_wrapper.h can include > > content/browser/dom_storage_content_impl.h no problem. > > Sorry but I was misunderstanding. Instead of adding a helper function, I've just > changed the argument from bool to PurgeOption. Thnx, that's exactly what i was suggesting :) The PurgeMemory() method IS the helper i was referring to.
Sorry for the late reply. I was trying to read the doc and got lost. https://codereview.chromium.org/2335933003/diff/80001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2335933003/diff/80001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.cc:415: case base::MemoryState::THROTTLED: I fail to understand when THROTTLED gets called for browser. So, without memory coordinator the pressure levels were moderate and critical. The DOMStorage does non-aggressive and less aggressive purging in these respective cases. What happens in browser when using memory coordinator and we hit moderate and critical signals? I am worried that if we hit a critical pressure signal and if we only purge unopened, and not do an aggressive purge, then we might not be purging enough here. None of the levels could be used to differentiate between moderate and critical, whereas it seems to make sense for renderers very well. Also, I think in case where the foreground renderer is going to be throttled an aggressive purge should be called. This would try to commit the databases that foreground renderer has opened.
https://codereview.chromium.org/2335933003/diff/80001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2335933003/diff/80001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.cc:415: case base::MemoryState::THROTTLED: On 2016/09/15 21:58:54, ssid wrote: > I fail to understand when THROTTLED gets called for browser. So, without memory > coordinator the pressure levels were moderate and critical. The DOMStorage does > non-aggressive and less aggressive purging in these respective cases. chrisha@ wrote a design doc for when to change memory state and I think he is implementing the first version of memory coordinator in browser side. https://docs.google.com/document/d/1GMSkij4qpbkK6T7MtCX9FcPlv39Q9J2lmW8qY8Wmf... > > What happens in browser when using memory coordinator and we hit moderate and > critical signals? > I am worried that if we hit a critical pressure signal and if we only purge > unopened, and not do an aggressive purge, then we might not be purging enough > here. When memory coordinator is enabled, MemoryPressureMonitor will be disabled, which means there is no critical/moderate notifications. Instead, MemoryCoordinator will notify memory state changes to MemoryCoordinatorClient (DOMStorageContextWrapper in this CL). As hajimehoshi@ described in comments, we only have normal/throttled states in the browser side and there is no levels in throttling for now. It might not be enough but I think we can start here then improve this if this could be a problem. (In my ideal world we should actually throttle memory allocation if possible, maybe adding ThrottleMemory()) > > None of the levels could be used to differentiate between moderate and critical, > whereas it seems to make sense for renderers very well. > Also, I think in case where the foreground renderer is going to be throttled an > aggressive purge should be called. This would try to commit the databases that > foreground renderer has opened.
https://codereview.chromium.org/2335933003/diff/80001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2335933003/diff/80001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.cc:415: case base::MemoryState::THROTTLED: On 2016/09/15 23:09:29, bashi1 wrote: > On 2016/09/15 21:58:54, ssid wrote: > > I fail to understand when THROTTLED gets called for browser. So, without > memory > > coordinator the pressure levels were moderate and critical. The DOMStorage > does > > non-aggressive and less aggressive purging in these respective cases. > chrisha@ wrote a design doc for when to change memory state and I think he is > implementing the first version of memory coordinator in browser side. > https://docs.google.com/document/d/1GMSkij4qpbkK6T7MtCX9FcPlv39Q9J2lmW8qY8Wmf... > > > > What happens in browser when using memory coordinator and we hit moderate and > > critical signals? > > I am worried that if we hit a critical pressure signal and if we only purge > > unopened, and not do an aggressive purge, then we might not be purging enough > > here. > When memory coordinator is enabled, MemoryPressureMonitor will be disabled, > which means there is no critical/moderate notifications. Instead, > MemoryCoordinator will notify memory state changes to MemoryCoordinatorClient > (DOMStorageContextWrapper in this CL). Yes, but on Android we get critical and moderate signals from the OS and for both these cases the coordinator would be sending a state of throttled. So, we would never do an aggressive purge. > As hajimehoshi@ described in comments, we only have normal/throttled states in > the browser side and there is no levels in throttling for now. It might not be > enough but I think we can start here then improve this if this could be a > problem. > > (In my ideal world we should actually throttle memory allocation if possible, > maybe adding ThrottleMemory()) I am afraid if we don't add a level now, we might not notice that later we could have released more memory on critical signals from OS. So, this would go unnoticed. Why not always purge aggressively and if we notice an issue, lets introduce lower levels?
https://codereview.chromium.org/2335933003/diff/80001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2335933003/diff/80001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.cc:415: case base::MemoryState::THROTTLED: On 2016/09/15 23:39:56, ssid wrote: > On 2016/09/15 23:09:29, bashi1 wrote: > > On 2016/09/15 21:58:54, ssid wrote: > > > I fail to understand when THROTTLED gets called for browser. So, without > > memory > > > coordinator the pressure levels were moderate and critical. The DOMStorage > > does > > > non-aggressive and less aggressive purging in these respective cases. > > chrisha@ wrote a design doc for when to change memory state and I think he is > > implementing the first version of memory coordinator in browser side. > > > https://docs.google.com/document/d/1GMSkij4qpbkK6T7MtCX9FcPlv39Q9J2lmW8qY8Wmf... > > > > > > What happens in browser when using memory coordinator and we hit moderate > and > > > critical signals? > > > I am worried that if we hit a critical pressure signal and if we only purge > > > unopened, and not do an aggressive purge, then we might not be purging > enough > > > here. > > When memory coordinator is enabled, MemoryPressureMonitor will be disabled, > > which means there is no critical/moderate notifications. Instead, > > MemoryCoordinator will notify memory state changes to MemoryCoordinatorClient > > (DOMStorageContextWrapper in this CL). > > Yes, but on Android we get critical and moderate signals from the OS and for > both these cases the coordinator would be sending a state of throttled. So, we > would never do an aggressive purge. > > > As hajimehoshi@ described in comments, we only have normal/throttled states in > > the browser side and there is no levels in throttling for now. It might not be > > enough but I think we can start here then improve this if this could be a > > problem. > > > > (In my ideal world we should actually throttle memory allocation if possible, > > maybe adding ThrottleMemory()) > > > I am afraid if we don't add a level now, we might not notice that later we could > have released more memory on critical signals from OS. So, this would go > unnoticed. > > Why not always purge aggressively and if we notice an issue, lets introduce > lower levels? +1 to this idea. For now, purging aggressively when THROTTLED is received makes sense to me.
Thank you! https://codereview.chromium.org/2335933003/diff/80001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2335933003/diff/80001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.cc:415: case base::MemoryState::THROTTLED: On 2016/09/15 23:57:42, bashi1 (slow til Sep 26) wrote: > On 2016/09/15 23:39:56, ssid wrote: > > On 2016/09/15 23:09:29, bashi1 wrote: > > > On 2016/09/15 21:58:54, ssid wrote: > > > > I fail to understand when THROTTLED gets called for browser. So, without > > > memory > > > > coordinator the pressure levels were moderate and critical. The DOMStorage > > > does > > > > non-aggressive and less aggressive purging in these respective cases. > > > chrisha@ wrote a design doc for when to change memory state and I think he > is > > > implementing the first version of memory coordinator in browser side. > > > > > > https://docs.google.com/document/d/1GMSkij4qpbkK6T7MtCX9FcPlv39Q9J2lmW8qY8Wmf... > > > > > > > > What happens in browser when using memory coordinator and we hit moderate > > and > > > > critical signals? > > > > I am worried that if we hit a critical pressure signal and if we only > purge > > > > unopened, and not do an aggressive purge, then we might not be purging > > enough > > > > here. > > > When memory coordinator is enabled, MemoryPressureMonitor will be disabled, > > > which means there is no critical/moderate notifications. Instead, > > > MemoryCoordinator will notify memory state changes to > MemoryCoordinatorClient > > > (DOMStorageContextWrapper in this CL). > > > > Yes, but on Android we get critical and moderate signals from the OS and for > > both these cases the coordinator would be sending a state of throttled. So, we > > would never do an aggressive purge. > > > > > As hajimehoshi@ described in comments, we only have normal/throttled states > in > > > the browser side and there is no levels in throttling for now. It might not > be > > > enough but I think we can start here then improve this if this could be a > > > problem. > > > > > > (In my ideal world we should actually throttle memory allocation if > possible, > > > maybe adding ThrottleMemory()) > > > > > > I am afraid if we don't add a level now, we might not notice that later we > could > > have released more memory on critical signals from OS. So, this would go > > unnoticed. > > > > Why not always purge aggressively and if we notice an issue, lets introduce > > lower levels? > > +1 to this idea. For now, purging aggressively when THROTTLED is received makes > sense to me. +1. Done.
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...
non-owner lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org Link to the patchset: https://codereview.chromium.org/2335933003/#ps100001 (title: "Address on ssid'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 #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Make DOMStorageContextWrapper a client of memory coordinator When the memory coordinator is enabled, DOMStorageContextWrapper becomes a client of the memory coordinator instead of installing MemoryPressureListener. Design Doc: https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8... BUG=639700 ========== to ========== Make DOMStorageContextWrapper a client of memory coordinator When the memory coordinator is enabled, DOMStorageContextWrapper becomes a client of the memory coordinator instead of installing MemoryPressureListener. Design Doc: https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8... BUG=639700 Committed: https://crrev.com/a517a220c9961d15c4f045ff3cabe03fa8e9af2c Cr-Commit-Position: refs/heads/master@{#419692} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a517a220c9961d15c4f045ff3cabe03fa8e9af2c Cr-Commit-Position: refs/heads/master@{#419692} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
