Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17106004:
Add discardable memory emulation for non-android/mac platforms (Closed)
|
Created:
7 years, 6 months ago by Ian Vollick Modified:
7 years, 1 month ago Reviewers:
jamesr, Avi (use Gerrit), Philippe, reveman, willchan no longer on Chromium, tony, cpu_(ooo_6.6-7.5), Tom Hudson, tomhudson, reed1 CC:
chromium-reviews, erikwright+watch_chromium.org, gavinp+memory_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd discardable memory emulation for non-android/mac platforms
Adds support for emulated discardable memory. The memory is managed by a
provider which listens for memory pressure notifications from the platform.
Currently, only android pushes these notifications, but in future patches, we
will apply pressure on other platforms in certain situations (e.g., when a tab
gets backgrounded).
BUG=237681
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231845
Patch Set 1 : . #
Total comments: 7
Patch Set 2 : . #
Total comments: 5
Patch Set 3 : . #
Total comments: 4
Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #
Total comments: 12
Patch Set 7 : . #Patch Set 8 : . #Patch Set 9 : . #
Total comments: 1
Patch Set 10 : . #Patch Set 11 : DiscardableMemory cannot inherit from NonThreadSafe. Also unnecessary: skia implements its own sync… #Patch Set 12 : . #Patch Set 13 : . #
Total comments: 24
Patch Set 14 : Addressing review. #
Total comments: 24
Patch Set 15 : refactor #Patch Set 16 : add test that use discardable memory on thread #Patch Set 17 : #Patch Set 18 : s/ASSERT_/EXPECT_/ #
Total comments: 10
Patch Set 19 : address review feedback #
Total comments: 14
Patch Set 20 : rebase and address review feedback #
Total comments: 2
Patch Set 21 : parameterize some tests #Patch Set 22 : Fix MemoryAfterUnlock test #
Total comments: 1
Messages
Total messages: 53 (0 generated)
I've started to split the original patch (https://codereview.chromium.org/15650016/) up into pieces. This patch is the base-only bit and it hopefully addresses your comments on the old patch. PTAL. There will be two other patches upcoming: 1. A patch to add code to respond to render widget visibility and call MemoryPressureListener::NotifyMemoryPressure as appropriate (i.e., it will notify of critical memory pressure when backgrounded). 2. A patch to add code to periodically poll system memory pressure and apply pressure as above. On 2013/06/12 14:56:48, Avi wrote: > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > File base/memory/discardable_memory.h (right): > > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > base/memory/discardable_memory.h:26: class DiscardableMemoryProvider; > Why is this class forward declared here? It's only used below if > !DISCARDABLE_MEMORY_SUPPORTED_NATIVELY, so this declaration seems useless. Removed. > > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > base/memory/discardable_memory.h:101: #endif > I would re-order this up two functions, so it doesn't appear to be covered by > the "testing utility calls" header. Or give it a header of its own. Moved to another header. > > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > File base/memory/discardable_memory_emulated.cc (right): > > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > base/memory/discardable_memory_emulated.cc:35: kSystemMemoryLowerBound; > Way too magical. Does this function belong in SysInfo? I'm borderline here, but > would be OK if you pushed hard. Gone. > > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > base/memory/discardable_memory_emulated.cc:38: typedef > HashingMRUCache<DiscardableMemory*, bool> AllocationMap; > bool? If that's just a placeholder, say so. It's a placeholder. Added a comment. > > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > base/memory/discardable_memory_emulated.cc:49: class DiscardableMemoryProvider { > Give the member functions explanatory comments. Member functions love > explanatory comments. Done :) > > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > base/memory/discardable_memory_emulated.cc:49: class DiscardableMemoryProvider { > DiscardableMemoryProvider desperately needs a unit test. The current unittest > file tests basic functionality of a system out of our control, but if we control > DiscardableMemoryProvider, we should be able to put it through a much more > thorough evaluation. You're right. Made a unit test. > > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > base/memory/discardable_memory_emulated.cc:69: Unregister(discardable); > Why the Unregister? Register is only called from > DiscardableMemory::InitializeAndLock below, which is an error to call more than > once, so it shouldn't ever be in the allocation cache. Definitely DCHECK if you > want, but Unregistering seems useless. True. Added a DCHECK, removed the call to Unregister. > > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > base/memory/discardable_memory_emulated.cc:83: discardable->Deallocate(); > Eh... > > Unregister is only called by ~DiscardableMemory, and it's weird that the > destructor of the class calls into a different class, which then calls back into > the class to dealloc. Can we move this call to DiscardableMemory::Deallocate() > into ~DiscardableMemory? Yeah, that was awkward. Once I fixed up and clarified the locking, this was unnecessary. Things are now, I think, the way you'd like them to be. > > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > base/memory/discardable_memory_emulated.cc:148: Lock lock_; > Member variables need comment love too. In particular, what is lock_ locking? I've now got two locks, one for the allocation list, and one for the bytes_allocated_ member and related limits. We often reenter the provider while iterating through the list and the locks may not be reacquired on the same thread, so having two locks simplified things considerably. > > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > base/memory/discardable_memory_emulated.cc:160: return Lock() != > DISCARDABLE_MEMORY_FAILED; > LockDiscardableMemoryStatus status = Lock(); > DCHECK_NE(DISCARDABLE_MEMORY_SUCCESS, status); // InitializeAndLock should only > be called once; why is there already memory? > return status == DISCARDABLE_MEMORY_PURGED; Done. > > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > base/memory/discardable_memory_emulated.cc:185: memory_ = malloc(size_ * > sizeof(char)); > sizeof(char) is defined by the standard to be 1 (C++98 §5.3.3¶1, expr.sizeof), > so just do |malloc(size_)|. Done. On 2013/06/13 20:16:06, cpu wrote: > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > File base/memory/discardable_memory_emulated.cc (right): > > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > base/memory/discardable_memory_emulated.cc:132: if (bytes_allocated_ < > kCacheLimitPurgeBound) > I was expecting freeing N megs, not freeing until N megs total Sure, sg. Done. > > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > base/memory/discardable_memory_emulated.cc:139: PurgeAll(); > again, surprised on the full purge. The provider is now a memory pressure listener, and we're freeing things up as recommended in memory_pressure_listener.h (i.e., we free up convenient stuff when under moderate pressure, and everything when we're under critical pressure). On 2013/06/14 11:09:33, Avi atgoogle (DO NOT USE) wrote: > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > File base/memory/discardable_memory.h (right): > > https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m... > base/memory/discardable_memory.h:25: #define > DISCARDABLE_MEMORY_SUPPORTED_NATIVELY > BTW it's likely that we will need to turn this into a runtime function rather > than a #define. The reason I had DiscardableMemory::Supported was that the > concept showed up in Win8, so if we go native on Win32 it won't be compile time. I've brought back ::Supported.
looking nice, I am going to be ooo monday. Thanks for your patience.
https://codereview.chromium.org/17106004/diff/8001/base/containers/mru_cache.h File base/containers/mru_cache.h (right): https://codereview.chromium.org/17106004/diff/8001/base/containers/mru_cache.... base/containers/mru_cache.h:127: // TODO(brettw) We may want a const version of this function in the future. remove this comment since you did it https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... File base/memory/discardable_memory.cc (right): https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... base/memory/discardable_memory.cc:24: CHECK(false); Braces if the if spans more than one line. (Even if that second line is a comment.) https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... File base/memory/discardable_memory.h (right): https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... base/memory/discardable_memory.h:96: #if !defined(DISCARDABLE_MEMORY_SUPPORTED_NATIVELY) How will this work on a platform like Win32 where we know if discardable memory is available only at runtime? Will this turn into DISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY? https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... File base/memory/discardable_memory_emulated.cc (right): https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... base/memory/discardable_memory_emulated.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. New files don't get "(c)". (this is for all new files in this CL) https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... File base/memory/discardable_memory_provider.h (right): https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... base/memory/discardable_memory_provider.h:15: class DiscardableMemory; no indent https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... base/memory/discardable_memory_provider.h:47: static void NotifyMemoryPressure(MemoryPressureListener::MemoryPressureLevel); you need to name your param https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... File base/memory/discardable_memory_provider_unittest.cc (right): https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... base/memory/discardable_memory_provider_unittest.cc:19: DiscardableMemoryProvider::SetInstanceForTest(provider_.get()); I'm lost; why are we keeping our own provider here and setting it as the instance?
On 2013/06/18 00:00:46, Avi wrote: > https://codereview.chromium.org/17106004/diff/8001/base/containers/mru_cache.h > File base/containers/mru_cache.h (right): > > https://codereview.chromium.org/17106004/diff/8001/base/containers/mru_cache.... > base/containers/mru_cache.h:127: // TODO(brettw) We may want a const version of > this function in the future. > remove this comment since you did it Done. > > https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... > File base/memory/discardable_memory.cc (right): > > https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... > base/memory/discardable_memory.cc:24: CHECK(false); > Braces if the if spans more than one line. (Even if that second line is a > comment.) Done. > > https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... > File base/memory/discardable_memory.h (right): > > https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... > base/memory/discardable_memory.h:96: #if > !defined(DISCARDABLE_MEMORY_SUPPORTED_NATIVELY) > How will this work on a platform like Win32 where we know if discardable memory > is available only at runtime? Will this turn into > DISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY? Yes, that sounds right. Currently, the 'emulated' discardable memory instances talk to the global provider, but I was thinking that when we add win support, it might make sense to have the discardable memory instances own a pointer to a provider and you can swap in a provider that gives native support when you detect that this is possible at runtime. > > https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... > File base/memory/discardable_memory_emulated.cc (right): > > https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... > base/memory/discardable_memory_emulated.cc:1: // Copyright (c) 2013 The Chromium > Authors. All rights reserved. > New files don't get "(c)". > > (this is for all new files in this CL) Done. > > https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... > File base/memory/discardable_memory_provider.h (right): > > https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... > base/memory/discardable_memory_provider.h:15: class DiscardableMemory; > no indent Done. > > https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... > base/memory/discardable_memory_provider.h:47: static void > NotifyMemoryPressure(MemoryPressureListener::MemoryPressureLevel); > you need to name your param Done. > > https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... > File base/memory/discardable_memory_provider_unittest.cc (right): > > https://codereview.chromium.org/17106004/diff/8001/base/memory/discardable_me... > base/memory/discardable_memory_provider_unittest.cc:19: > DiscardableMemoryProvider::SetInstanceForTest(provider_.get()); > I'm lost; why are we keeping our own provider here and setting it as the > instance? I've added a comment explaining the reasons for this, since they won't be obvious to future readers of the code. We create an instance here for two reasons: 1. To ensure that one test does not affect the next, and 2. Since the provider listens for notifications on the thread it was created on (issued via PostTask), if we create the provider on the test thread, we can run the test thread's message loop until idle when we need to process one of these notifications.
On 2013/06/19 21:13:58, vollick wrote: > I was thinking that when we add win support, it > might make sense to have the discardable memory instances own a pointer to a > provider and you can swap in a provider that gives native support when you > detect that this is possible at runtime. Who is "you" in this scenario? A pointer back to the provider makes sense if there is more than one provider active, but I don't see why we would want to do that. If we're taking the approach of using a Win8 provider, shouldn't that be installed first thing and the emulation provider never used?
On 2013/06/20 20:29:31, Avi wrote: > On 2013/06/19 21:13:58, vollick wrote: > > I was thinking that when we add win support, it > > might make sense to have the discardable memory instances own a pointer to a > > provider and you can swap in a provider that gives native support when you > > detect that this is possible at runtime. > > Who is "you" in this scenario? A pointer back to the provider makes sense if > there is more than one provider active, but I don't see why we would want to do > that. If we're taking the approach of using a Win8 provider, shouldn't that be > installed first thing and the emulation provider never used? BTW, I wouldn't want to hold this over speculation of what we would do for Windows platforms; I'd rather land something reasonable (that is on the level of "might" to "probably" workable) and figure out the details once we actually implement Windows.
On 2013/06/20 20:29:31, Avi wrote: > On 2013/06/19 21:13:58, vollick wrote: > > I was thinking that when we add win support, it > > might make sense to have the discardable memory instances own a pointer to a > > provider and you can swap in a provider that gives native support when you > > detect that this is possible at runtime. > > Who is "you" in this scenario? A pointer back to the provider makes sense if > there is more than one provider active, but I don't see why we would want to do > that. If we're taking the approach of using a Win8 provider, shouldn't that be > installed first thing and the emulation provider never used? You're right -- installing the right provider is a better route. In that case, I'm imagining that we'll have discardable_memory_provider_win.h which defines DiscardableMemoryProviderWin which is a DiscardableMemoryProvider (whose methods will have been virtualized). In discardable_memory_provider_win.cc, we'll have an implementation of DiscardableMemoryProvider::GetInstance that either returns a DiscardableMemoryProviderWin or a DiscardableMemoryProvider instance depending on the capabilities of the system. The definition of DiscardableMemoryProvider::GetInstance in discardable_memory_provider.cc will have to be #ifdef'd, of course.
On 2013/06/20 20:32:17, Avi wrote: > On 2013/06/20 20:29:31, Avi wrote: > > On 2013/06/19 21:13:58, vollick wrote: > > > I was thinking that when we add win support, it > > > might make sense to have the discardable memory instances own a pointer to a > > > provider and you can swap in a provider that gives native support when you > > > detect that this is possible at runtime. > > > > Who is "you" in this scenario? A pointer back to the provider makes sense if > > there is more than one provider active, but I don't see why we would want to > do > > that. If we're taking the approach of using a Win8 provider, shouldn't that be > > installed first thing and the emulation provider never used? > > BTW, I wouldn't want to hold this over speculation of what we would do for > Windows platforms; I'd rather land something reasonable (that is on the level of > "might" to "probably" workable) and figure out the details once we actually > implement Windows. K, cool. I just didn't want to land a design that made Win8's implementation overly difficult :) Does this seem reasonable, then?
https://codereview.chromium.org/17106004/diff/16001/base/memory/discardable_m... File base/memory/discardable_memory_emulated.cc (right): https://codereview.chromium.org/17106004/diff/16001/base/memory/discardable_m... base/memory/discardable_memory_emulated.cc:13: return true; :/ so this always returns true. For Windows, we'll eventually need this in the flavor of "is this natively supported?" but right now, having a function like this that always returns true isn't helpful. I guess I'm changing my stance here; we can drop this. https://codereview.chromium.org/17106004/diff/16001/base/memory/discardable_m... File base/memory/discardable_memory_provider.cc (right): https://codereview.chromium.org/17106004/diff/16001/base/memory/discardable_m... base/memory/discardable_memory_provider.cc:41: DiscardableMemoryProvider::~DiscardableMemoryProvider() { What is the scenario in which this gets called? I'd imagine that a provider like this would be installed and live forever. (shrug) https://codereview.chromium.org/17106004/diff/16001/base/memory/discardable_m... base/memory/discardable_memory_provider.cc:87: const_cast<DiscardableMemoryProvider*>(this)->bytes_allocated_lock_); Yikes. Do we want to make the lock mutable then?
On 2013/06/20 20:47:41, Avi wrote: > https://codereview.chromium.org/17106004/diff/16001/base/memory/discardable_m... > File base/memory/discardable_memory_emulated.cc (right): > > https://codereview.chromium.org/17106004/diff/16001/base/memory/discardable_m... > base/memory/discardable_memory_emulated.cc:13: return true; > :/ so this always returns true. > > For Windows, we'll eventually need this in the flavor of "is this natively > supported?" but right now, having a function like this that always returns true > isn't helpful. I guess I'm changing my stance here; we can drop this. Good point. I think that "SupportedNatively" could be useful, so I've swapped out "Supported" for that. On Win, its return value will depend on your runtime check. WDYT? > > https://codereview.chromium.org/17106004/diff/16001/base/memory/discardable_m... > File base/memory/discardable_memory_provider.cc (right): > > https://codereview.chromium.org/17106004/diff/16001/base/memory/discardable_m... > base/memory/discardable_memory_provider.cc:41: > DiscardableMemoryProvider::~DiscardableMemoryProvider() { > What is the scenario in which this gets called? I'd imagine that a provider like > this would be installed and live forever. (shrug) Only when the process is terminated and between unittests. > > https://codereview.chromium.org/17106004/diff/16001/base/memory/discardable_m... > base/memory/discardable_memory_provider.cc:87: > const_cast<DiscardableMemoryProvider*>(this)->bytes_allocated_lock_); > Yikes. Do we want to make the lock mutable then? It's pretty gross, eh? :) Sure, mutable looks a little nicer. Done.
https://codereview.chromium.org/17106004/diff/27001/base/memory/discardable_m... File base/memory/discardable_memory.h (right): https://codereview.chromium.org/17106004/diff/27001/base/memory/discardable_m... base/memory/discardable_memory.h:21: #define DISCARDABLE_MEMORY_SUPPORTED_NATIVELY If there is now a runtime call, do we need this #define? https://codereview.chromium.org/17106004/diff/27001/base/memory/discardable_m... File base/memory/discardable_memory_unittest.cc (right): https://codereview.chromium.org/17106004/diff/27001/base/memory/discardable_m... base/memory/discardable_memory_unittest.cc:12: // case here. ... especially here; the idea that the #define would be false, but the runtime check would be true...
https://codereview.chromium.org/17106004/diff/16001/base/memory/discardable_m... File base/memory/discardable_memory.h (right): https://codereview.chromium.org/17106004/diff/16001/base/memory/discardable_m... base/memory/discardable_memory.h:29: // flexibility as to which objects get discarded. I don't see the mention of the word 'thread' in the whole file. It would be nice to specify the thread safety of this thing.
https://codereview.chromium.org/17106004/diff/16001/base/memory/discardable_m... File base/memory/discardable_memory.h (right): https://codereview.chromium.org/17106004/diff/16001/base/memory/discardable_m... base/memory/discardable_memory.h:29: // flexibility as to which objects get discarded. On 2013/06/20 21:30:17, cpu wrote: > I don't see the mention of the word 'thread' in the whole file. It would be nice > to specify the thread safety of this thing. > Done. https://codereview.chromium.org/17106004/diff/27001/base/memory/discardable_m... File base/memory/discardable_memory.h (right): https://codereview.chromium.org/17106004/diff/27001/base/memory/discardable_m... base/memory/discardable_memory.h:21: #define DISCARDABLE_MEMORY_SUPPORTED_NATIVELY On 2013/06/20 21:16:41, Avi wrote: > If there is now a runtime call, do we need this #define? Yup, but it needs a better name. We need to conditionally compile code if there's any chance we need emulation. I've changed this to DISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY. For the unit test, I've also added DISCARDABLE_MEMORY_NEVER_SUPPORTED_NATIVELY. For the win implementation, neither would be defined.
LGTM https://codereview.chromium.org/17106004/diff/27001/base/memory/discardable_m... File base/memory/discardable_memory.h (right): https://codereview.chromium.org/17106004/diff/27001/base/memory/discardable_m... base/memory/discardable_memory.h:21: #define DISCARDABLE_MEMORY_SUPPORTED_NATIVELY On 2013/06/20 23:27:09, vollick wrote: > On 2013/06/20 21:16:41, Avi wrote: > > If there is now a runtime call, do we need this #define? > > Yup, but it needs a better name. We need to conditionally compile code if > there's any chance we need emulation. I've changed this to > DISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY. For the unit test, I've also added > DISCARDABLE_MEMORY_NEVER_SUPPORTED_NATIVELY. For the win implementation, neither > would be defined. Meh. I'm not thrilled, but I don't care that much.
LGTM I still feel fuzzy about the threading situation, for future CLs consider deriving from NonThreadSafe or even better making some facet of the interface thread safe, in particular the OS implementation is safe in the lock and unlock. My concern comes from the fact that 'memory managent' is something that in all OS is thread safe so I can see a potential for brewing bugs on the users of this. But I let the use cases drive the rest.
+willchan for base/ OWNERS +darin for webkit/ OWNERS On 2013/06/21 00:12:40, cpu wrote: > LGTM > > I still feel fuzzy about the threading situation, for future CLs consider > deriving from NonThreadSafe or even better making some facet of the interface > thread safe, in particular the OS implementation is safe in the lock and > unlock. My concern comes from the fact that 'memory managent' is something that > in all OS is thread safe so I can see a potential for brewing bugs on the users > of this. > > But I let the use cases drive the rest. On 2013/06/21 00:12:40, cpu wrote: > LGTM > > I still feel fuzzy about the threading situation, for future CLs consider > deriving from NonThreadSafe or even better making some facet of the interface > thread safe, in particular the OS implementation is safe in the lock and > unlock. My concern comes from the fact that 'memory managent' is something that > in all OS is thread safe so I can see a potential for brewing bugs on the users > of this. > > But I let the use cases drive the rest. I was worried that deriving from NonThreadSafe would cause us problems; that we would tie DiscardableMemory instances to the thread that created them, even if that wasn't the thread that ultimately used them. This turns out not to be the case, currently, so I've derived from NonThreadSafe as you suggested. I've added a comment in DiscardableMemory describing what should be done if we eventually need to pass ownership of instances from one thread to another.
lgtm
https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... File base/memory/discardable_memory.cc (right): https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... base/memory/discardable_memory.cc:24: CHECK(false); Weird, why not just do CHECK(is_locked_);? https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... File base/memory/discardable_memory_provider.h (right): https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:31: // The DiscardableMemoryProvider is used on platforms that do not support It's not clear if DiscardableMemoryProvider is supposed to be directly accessed by consumers of base, or if this is an implementation detail. https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:44: static DiscardableMemoryProvider* GetInstance(); Who calls this? Why is it public? It looks like it's only used for the emulated DiscardableMemory, which I think is friended. Why is this kept public? https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:47: static void NotifyMemoryPressure( Why is this public too? Who is supposed to be calling this? https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:48: MemoryPressureListener::MemoryPressureLevel pressureLevel); pressure_level https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:127: scoped_ptr<MemoryPressureListener> memory_pressure_listener_; Why bother with a scoped_ptr?
Darin, can you PTAL? https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... File base/memory/discardable_memory.cc (right): https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... base/memory/discardable_memory.cc:24: CHECK(false); On 2013/06/25 21:06:50, willchan wrote: > Weird, why not just do CHECK(is_locked_);? Done. https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... File base/memory/discardable_memory_provider.h (right): https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:31: // The DiscardableMemoryProvider is used on platforms that do not support On 2013/06/25 21:06:50, willchan wrote: > It's not clear if DiscardableMemoryProvider is supposed to be directly accessed > by consumers of base, or if this is an implementation detail. It's an implementation detail. It's only got a header so I can unit test it. Added a clarifying comment. The visibility changes you requested below emphasize this, too. https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:44: static DiscardableMemoryProvider* GetInstance(); On 2013/06/25 21:06:50, willchan wrote: > Who calls this? Why is it public? It looks like it's only used for the emulated > DiscardableMemory, which I think is friended. Why is this kept public? Made it private. https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:47: static void NotifyMemoryPressure( On 2013/06/25 21:06:50, willchan wrote: > Why is this public too? Who is supposed to be calling this? It's only called by the MemoryPressureListener. It can be private. Updated the visibility. https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:48: MemoryPressureListener::MemoryPressureLevel pressureLevel); On 2013/06/25 21:06:50, willchan wrote: > pressure_level Done. https://codereview.chromium.org/17106004/diff/40001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:127: scoped_ptr<MemoryPressureListener> memory_pressure_listener_; On 2013/06/25 21:06:50, willchan wrote: > Why bother with a scoped_ptr? Got rid of the scoped_ptr.
-darin (unavailable) Tony, Will -- are you able to do an OWNERS review for this CL? +tony for webkit/ +willchan for base/
rubberstamp webkit LGTM.
I'm sorry, I'm going out on another trip tomorrow and will return on the 15th, so I fear I can't do this review.
Hi Brett, Will's not able to do this review. Would you be able to take a look?
https://codereview.chromium.org/17106004/diff/50001/base/memory/discardable_m... File base/memory/discardable_memory.h (right): https://codereview.chromium.org/17106004/diff/50001/base/memory/discardable_m... base/memory/discardable_memory.h:25: #endif Just to point out that if https://codereview.chromium.org/18910002/ lands this define is set by the build system based on platform, so this probably shouldn't be set here.
https://codereview.chromium.org/17106004/diff/79001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/17106004/diff/79001/build/common.gypi#newcode... build/common.gypi:3984: 'defines': ['DISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY'], I don't quite understand why we need the word ALWAYS in here, other than Android precedent. Are there systems that sometimes support discardable memory natively, and sometimes don't? i.e, "How is DISCARDABLE_MEMORY_SUPPORTED_NATIVELY insufficient?"
On 2013/08/08 22:29:57, Peter Mayo wrote: > Are there systems that sometimes support discardable memory > natively, and sometimes don't? Win32.
On 2013/08/08 22:43:17, Avi wrote: > On 2013/08/08 22:29:57, Peter Mayo wrote: > > Are there systems that sometimes support discardable memory > > natively, and sometimes don't? > > Win32. Darin, I think this is ready to go. The failures seem like flake. Can you PTAL at the base/ stuff when you have a moment?
https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... File base/memory/discardable_memory.h (right): https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory.h:102: bool Allocate(); Please document the return value. https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... File base/memory/discardable_memory_emulated.cc (right): https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_emulated.cc:36: return DISCARDABLE_MEMORY_FAILED; Shouldn't this have a NOTREACHED()? Anytime this happens is a programmer error, right? https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_emulated.cc:42: if (Allocate()) I note here that Allocate() does not seem to attempt to purge unlocked memory to make available for allocation. Is this by design? Is that what ashmem does? https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... File base/memory/discardable_memory_provider.cc (right): https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.cc:23: // 2560x1600 images. One thing I wonder is if we'll have a good way of catching new users of this API which might lead to a resident set that exceeds the available memory and cause too much thrashing. Will performance tests catch this? Or do we need more controls here? Do you have any idea about how serious it is if we get this wrong, and if we have good automated ways of catching this? https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.cc:135: DCHECK(bytes <= bytes_allocated_); DCHECK_LE? https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.cc:143: AllocationMap::iterator it = allocations_.Get(discardable); This is non-obvious to readers who don't realize the side effect of the Get() leading to updating the MRUCache, and it's not obvious when reading AllocationMap that it's a MRUCache if you're just skimming the code. Can you explain with a comment? https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... File base/memory/discardable_memory_provider.h (right): https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:5: #ifndef BASE_MEMORY_DISCARDABLE_MEMORY_PROVIDER_H_ I don't like to bikeshed too much on naming, but when I first saw this, it wasn't clear to me that this was only for emulated memory. Maybe name DiscardableMemoryEmulatedProvider? If you strongly prefer as is, I don't mind. https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:6: #define BASE_MEMORY_DISCARDABLE_MEMORY_PROVIDER_H_ It'd be great if there was a file level comment describing what this is actually doing under the hood. This is an implementation detail class, and some higher level explanation of the purge policy and what not would be good. There seems to be a fair amount of fine grained usage of locks in EnforcePolicy(), which results in ignoring multiple EnforcePolicy() calls while one is in effect. And combined with Lock() not actually purging unlocked discardable memory, this seems to allow for many situations where DiscardableMemory cannot be acquired. What do callers do in this case? https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:37: // NB - this class is an implementation detail. It has been exposed for testing Please throw this class into the base::internal namespace. That's one of the main uses for it. See https://code.google.com/p/chromium/codesearch#search/&q=internal::%20test.cc&... for examples of this practice elsewhere in our code base. https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:42: virtual ~DiscardableMemoryProvider(); Why is this virtual? https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:46: typedef HashingMRUCache<DiscardableMemory*, bool> AllocationMap; Types go before constructors/destructor in the class definition according to the style guide. https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:132: MemoryPressureListener memory_pressure_listener_; This is only implemented on Android right now, right? And Android doesn't use emulated discardable memory, right? So is this code never going to be exercised, or will be unused when you commit?
Thanks for the review! https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... File base/memory/discardable_memory.h (right): https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory.h:102: bool Allocate(); On 2013/08/27 03:23:15, willchan wrote: > Please document the return value. Done. https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... File base/memory/discardable_memory_emulated.cc (right): https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_emulated.cc:36: return DISCARDABLE_MEMORY_FAILED; On 2013/08/27 03:23:15, willchan wrote: > Shouldn't this have a NOTREACHED()? Anytime this happens is a programmer error, > right? Yeah, this should be a NOTREACHED(). Updated. https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_emulated.cc:42: if (Allocate()) On 2013/08/27 03:23:15, willchan wrote: > I note here that Allocate() does not seem to attempt to purge unlocked memory to > make available for allocation. Is this by design? Is that what ashmem does? Ah good point! I've updated the code to purge if we hit this situation. https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... File base/memory/discardable_memory_provider.cc (right): https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.cc:23: // 2560x1600 images. On 2013/08/27 03:23:15, willchan wrote: > One thing I wonder is if we'll have a good way of catching new users of this API > which might lead to a resident set that exceeds the available memory and cause > too much thrashing. Will performance tests catch this? Or do we need more > controls here? Do you have any idea about how serious it is if we get this > wrong, and if we have good automated ways of catching this? Consumers of the DiscardableMemory already need to be prepared for requests to fail, so if we get this wrong, we shouldn't be worse than our current behavior. As for detecting when we're doing badly, I've instrumented the purge calls with trace events. It should be simple to build automated tests around them if this turns out to be a pain point. https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.cc:135: DCHECK(bytes <= bytes_allocated_); On 2013/08/27 03:23:15, willchan wrote: > DCHECK_LE? Done. https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.cc:143: AllocationMap::iterator it = allocations_.Get(discardable); On 2013/08/27 03:23:15, willchan wrote: > This is non-obvious to readers who don't realize the side effect of the Get() > leading to updating the MRUCache, and it's not obvious when reading > AllocationMap that it's a MRUCache if you're just skimming the code. Can you > explain with a comment? Done. https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... File base/memory/discardable_memory_provider.h (right): https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:5: #ifndef BASE_MEMORY_DISCARDABLE_MEMORY_PROVIDER_H_ On 2013/08/27 03:23:15, willchan wrote: > I don't like to bikeshed too much on naming, but when I first saw this, it > wasn't clear to me that this was only for emulated memory. Maybe name > DiscardableMemoryEmulatedProvider? If you strongly prefer as is, I don't mind. I've updated the class level comment. I'm hoping it more clearly articulates that what the provider provides is emulated discardable memory. If you're willing to deal with it, I'd really prefer to keep the current name. I appreciate that it's a bit ambiguous, but I find the name to be a bit less clunky and I'm hoping that since it's an implementation detail, folks won't interact with this class frequently and that it won't be a confusion factory. https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:6: #define BASE_MEMORY_DISCARDABLE_MEMORY_PROVIDER_H_ On 2013/08/27 03:23:15, willchan wrote: > It'd be great if there was a file level comment describing what this is actually > doing under the hood. This is an implementation detail class, and some higher > level explanation of the purge policy and what not would be good. I've beefed up the big comment above the class definition. Is this better, or should I write a larger piece up here? > There seems to > be a fair amount of fine grained usage of locks in EnforcePolicy(), which > results in ignoring multiple EnforcePolicy() calls while one is in effect. And > combined with Lock() not actually purging unlocked discardable memory, this > seems to allow for many situations where DiscardableMemory cannot be acquired. > What do callers do in this case? Consumers of DiscardableMemory need to be prepared for their requests to fail (they fall back to conventional allocation in this case). There are some existing consumers and they're set up this way. That said, your point about not purging in Lock was a great one. Hopefully the purge I've added there will make this a less common occurrence. https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:37: // NB - this class is an implementation detail. It has been exposed for testing On 2013/08/27 03:23:15, willchan wrote: > Please throw this class into the base::internal namespace. That's one of the > main uses for it. See > https://code.google.com/p/chromium/codesearch#search/&q=internal::%2520test.c... > for examples of this practice elsewhere in our code base. Done. https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:42: virtual ~DiscardableMemoryProvider(); On 2013/08/27 03:23:15, willchan wrote: > Why is this virtual? No good reason. Removed. https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:46: typedef HashingMRUCache<DiscardableMemory*, bool> AllocationMap; On 2013/08/27 03:23:15, willchan wrote: > Types go before constructors/destructor in the class definition according to the > style guide. Done. https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... base/memory/discardable_memory_provider.h:132: MemoryPressureListener memory_pressure_listener_; On 2013/08/27 03:23:15, willchan wrote: > This is only implemented on Android right now, right? And Android doesn't use > emulated discardable memory, right? So is this code never going to be exercised, > or will be unused when you commit? Android is the only platform that listens to real system memory pressure, but we apply artificial memory pressure on other platforms, too (see render_thread_impl.cc).
On 2013/09/24 17:15:24, Ian Vollick wrote: > Thanks for the review! > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > File base/memory/discardable_memory.h (right): > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > base/memory/discardable_memory.h:102: bool Allocate(); > On 2013/08/27 03:23:15, willchan wrote: > > Please document the return value. > > Done. > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > File base/memory/discardable_memory_emulated.cc (right): > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > base/memory/discardable_memory_emulated.cc:36: return DISCARDABLE_MEMORY_FAILED; > On 2013/08/27 03:23:15, willchan wrote: > > Shouldn't this have a NOTREACHED()? Anytime this happens is a programmer > error, > > right? > Yeah, this should be a NOTREACHED(). Updated. > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > base/memory/discardable_memory_emulated.cc:42: if (Allocate()) > On 2013/08/27 03:23:15, willchan wrote: > > I note here that Allocate() does not seem to attempt to purge unlocked memory > to > > make available for allocation. Is this by design? Is that what ashmem does? > > Ah good point! I've updated the code to purge if we hit this situation. > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > File base/memory/discardable_memory_provider.cc (right): > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > base/memory/discardable_memory_provider.cc:23: // 2560x1600 images. > On 2013/08/27 03:23:15, willchan wrote: > > One thing I wonder is if we'll have a good way of catching new users of this > API > > which might lead to a resident set that exceeds the available memory and cause > > too much thrashing. Will performance tests catch this? Or do we need more > > controls here? Do you have any idea about how serious it is if we get this > > wrong, and if we have good automated ways of catching this? > > Consumers of the DiscardableMemory already need to be prepared for requests to > fail, so if we get this wrong, we shouldn't be worse than our current behavior. > > As for detecting when we're doing badly, I've instrumented the purge calls with > trace events. It should be simple to build automated tests around them if this > turns out to be a pain point. > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > base/memory/discardable_memory_provider.cc:135: DCHECK(bytes <= > bytes_allocated_); > On 2013/08/27 03:23:15, willchan wrote: > > DCHECK_LE? > > Done. > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > base/memory/discardable_memory_provider.cc:143: AllocationMap::iterator it = > allocations_.Get(discardable); > On 2013/08/27 03:23:15, willchan wrote: > > This is non-obvious to readers who don't realize the side effect of the Get() > > leading to updating the MRUCache, and it's not obvious when reading > > AllocationMap that it's a MRUCache if you're just skimming the code. Can you > > explain with a comment? > > Done. > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > File base/memory/discardable_memory_provider.h (right): > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > base/memory/discardable_memory_provider.h:5: #ifndef > BASE_MEMORY_DISCARDABLE_MEMORY_PROVIDER_H_ > On 2013/08/27 03:23:15, willchan wrote: > > I don't like to bikeshed too much on naming, but when I first saw this, it > > wasn't clear to me that this was only for emulated memory. Maybe name > > DiscardableMemoryEmulatedProvider? If you strongly prefer as is, I don't mind. > > I've updated the class level comment. I'm hoping it more clearly articulates > that what the provider provides is emulated discardable memory. If you're > willing to deal with it, I'd really prefer to keep the current name. I > appreciate that it's a bit ambiguous, but I find the name to be a bit less > clunky and I'm hoping that since it's an implementation detail, folks won't > interact with this class frequently and that it won't be a confusion factory. > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > base/memory/discardable_memory_provider.h:6: #define > BASE_MEMORY_DISCARDABLE_MEMORY_PROVIDER_H_ > On 2013/08/27 03:23:15, willchan wrote: > > It'd be great if there was a file level comment describing what this is > actually > > doing under the hood. This is an implementation detail class, and some higher > > level explanation of the purge policy and what not would be good. > I've beefed up the big comment above the class definition. Is this better, or > should I write a larger piece up here? > > > There seems to > > be a fair amount of fine grained usage of locks in EnforcePolicy(), which > > results in ignoring multiple EnforcePolicy() calls while one is in effect. And > > combined with Lock() not actually purging unlocked discardable memory, this > > seems to allow for many situations where DiscardableMemory cannot be acquired. > > What do callers do in this case? > > Consumers of DiscardableMemory need to be prepared for their requests to fail > (they fall back to conventional allocation in this case). There are some > existing consumers and they're set up this way. > > That said, your point about not purging in Lock was a great one. Hopefully the > purge I've added there will make this a less common occurrence. > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > base/memory/discardable_memory_provider.h:37: // NB - this class is an > implementation detail. It has been exposed for testing > On 2013/08/27 03:23:15, willchan wrote: > > Please throw this class into the base::internal namespace. That's one of the > > main uses for it. See > > > https://code.google.com/p/chromium/codesearch#search/&q=internal::%252520test... > > for examples of this practice elsewhere in our code base. > > Done. > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > base/memory/discardable_memory_provider.h:42: virtual > ~DiscardableMemoryProvider(); > On 2013/08/27 03:23:15, willchan wrote: > > Why is this virtual? > No good reason. Removed. > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > base/memory/discardable_memory_provider.h:46: typedef > HashingMRUCache<DiscardableMemory*, bool> AllocationMap; > On 2013/08/27 03:23:15, willchan wrote: > > Types go before constructors/destructor in the class definition according to > the > > style guide. > > Done. > > https://codereview.chromium.org/17106004/diff/89001/base/memory/discardable_m... > base/memory/discardable_memory_provider.h:132: MemoryPressureListener > memory_pressure_listener_; > On 2013/08/27 03:23:15, willchan wrote: > > This is only implemented on Android right now, right? And Android doesn't use > > emulated discardable memory, right? So is this code never going to be > exercised, > > or will be unused when you commit? > > Android is the only platform that listens to real system memory pressure, but we > apply artificial memory pressure on other platforms, too (see > render_thread_impl.cc). Hi All. This bug seems to have been closed several times. I've just reopened it. If that was a mistake and this patch should be abandoned, please reply with a not looks good to me.
Still reviewing the provider code, but had some comments beforehand. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... File base/memory/discardable_memory.h (right): https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory.h:16: class DiscardableMemoryProvider; No indentation https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory.h:47: // contention for the instances. It's more technically correct to say there are no races, rather than there is no contention. Contention would be true if you actually used a mutex. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory.h:102: friend class base::internal::DiscardableMemoryProvider; You can ditch the base::. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... File base/memory/discardable_memory_emulated.cc (right): https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_emulated.cc:50: DiscardableMemoryProvider::GetInstance()->PurgeAll(); This is kind of a big hammer. No need to prematurely optimize, but perhaps leave a note to say this might be worth optimizing later on rather than purging all instances? https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_emulated.cc:69: if (size_ == 0) Interesting. I hadn't considered this case before. Is it a bug to try to initialize a DiscardableMemory with size 0? The interesting implication of this here is that Allocate() will return false, which then can lead to purging all of the DiscardableMemory instances. I see that you've added a test for this, but I sorta wonder if we should treat allocating zero bytes as a bug instead. Is there a good reason to allow this behavior?
https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... File base/memory/discardable_memory_provider.cc (right): https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:46: for (; it != allocations_.end(); ++it) Mind putting braces around this? I know it works, but some people unfamiliar to C++ don't know how these line/statement rules work, especially when there's a 2-line statement within a loop statement. Braces make it more clear. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:65: void DiscardableMemoryProvider::NotifyMemoryPressure( OK, there's a bug here. NotifyMemoryPressure will always be called on whatever thread the DiscardableMemoryProvider is created on, which is undefined due to the lazy creation when the first DiscardableMemoryEmulated is created. Now, all these Purge*() calls happen on the notification thread, not necessarily the DiscardableMemoryEmulated creation thread. DiscardableMemoryEmulated is not threadsafe. This means memory pressure notifications can cause races. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... File base/memory/discardable_memory_provider.h (right): https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_provider.h:46: class BASE_EXPORT DiscardableMemoryProvider { BASE_EXPORT_PRIVATE? https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_provider.h:70: friend class base::DiscardableMemory; Can you just move the necessary private member functions to the public section and ditch the friends? This is already listed in ::internal and should be BASE_EXPORT_PRIVATE, so no one should be able to access this. This way it's more obvious to me what private members DiscardableMemoryEmulated needs to access. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_provider.h:82: // provider is retained by the caller. You should note that this should be called prior to any DiscardableMemory being used, since this setter is not threadsafe.
https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... File base/memory/discardable_memory_provider_unittest.cc (right): https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_provider_unittest.cc:78: ASSERT_NE(static_cast<void*>(NULL), Memory(discardable)); You should use EXPECT_*() unless the following statements would crash otherwise, or it would trigger way too many errors and not be useful. Since these are checking the various accessors which are independent, it's good to EXPECT_* on them separately. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_provider_unittest.cc:137: MemoryPressureListener::NotifyMemoryPressure( You should add a test with a DiscardableMemory on a different thread.
Hi all! I'm picking up this patch as it's needed for the new image cache system I'm working on and vollick@ doesn't have cycles for this at the moment. I've uploaded a new patch that I think address prior review feedback. It's a significant refactor of the previous patch. The main difference is that the provider is no longer making callbacks into the DiscardableMemory instances. The memory associated with a DiscardableMemory instance is also owned by the DM instance when locked and by the provider when it's unlocked and can be purged. That's supposed to make it clear what memory can be purged but it might be cleaner if the provider just always owns the memory. Let me know what you prefer. Please take a look. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... File base/memory/discardable_memory.h (right): https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory.h:16: class DiscardableMemoryProvider; On 2013/10/01 18:12:05, willchan wrote: > No indentation Done. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory.h:47: // contention for the instances. On 2013/10/01 18:12:05, willchan wrote: > It's more technically correct to say there are no races, rather than there is no > contention. Contention would be true if you actually used a mutex. Done. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory.h:102: friend class base::internal::DiscardableMemoryProvider; On 2013/10/01 18:12:05, willchan wrote: > You can ditch the base::. Done. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... File base/memory/discardable_memory_emulated.cc (right): https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_emulated.cc:50: DiscardableMemoryProvider::GetInstance()->PurgeAll(); On 2013/10/01 18:12:05, willchan wrote: > This is kind of a big hammer. No need to prematurely optimize, but perhaps leave > a note to say this might be worth optimizing later on rather than purging all > instances? My latest patch is purging the minimal amount of LRU memory in these cases. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... File base/memory/discardable_memory_provider.cc (right): https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:46: for (; it != allocations_.end(); ++it) On 2013/10/01 18:47:25, willchan wrote: > Mind putting braces around this? I know it works, but some people unfamiliar to > C++ don't know how these line/statement rules work, especially when there's a > 2-line statement within a loop statement. Braces make it more clear. Done. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:65: void DiscardableMemoryProvider::NotifyMemoryPressure( On 2013/10/01 18:47:25, willchan wrote: > OK, there's a bug here. NotifyMemoryPressure will always be called on whatever > thread the DiscardableMemoryProvider is created on, which is undefined due to > the lazy creation when the first DiscardableMemoryEmulated is created. > > Now, all these Purge*() calls happen on the notification thread, not necessarily > the DiscardableMemoryEmulated creation thread. DiscardableMemoryEmulated is not > threadsafe. This means memory pressure notifications can cause races. This is supposed to be fixed in my latest patch. PTAL. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:75: NOTREACHED(); Note: I removed the default case and moved the NOTREACHED out of switch statement so the compiler will force us to update this code if we ever add new MEMORY_PRESSURE_* values. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... File base/memory/discardable_memory_provider.h (right): https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_provider.h:46: class BASE_EXPORT DiscardableMemoryProvider { On 2013/10/01 18:47:25, willchan wrote: > BASE_EXPORT_PRIVATE? Done. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_provider.h:70: friend class base::DiscardableMemory; On 2013/10/01 18:47:25, willchan wrote: > Can you just move the necessary private member functions to the public section > and ditch the friends? This is already listed in ::internal and should be > BASE_EXPORT_PRIVATE, so no one should be able to access this. > > This way it's more obvious to me what private members DiscardableMemoryEmulated > needs to access. Done. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_provider.h:82: // provider is retained by the caller. On 2013/10/01 18:47:25, willchan wrote: > You should note that this should be called prior to any DiscardableMemory being > used, since this setter is not threadsafe. Done. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... File base/memory/discardable_memory_provider_unittest.cc (right): https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_provider_unittest.cc:78: ASSERT_NE(static_cast<void*>(NULL), Memory(discardable)); On 2013/10/01 19:05:05, willchan wrote: > You should use EXPECT_*() unless the following statements would crash otherwise, > or it would trigger way too many errors and not be useful. Since these are > checking the various accessors which are independent, it's good to EXPECT_* on > them separately. Done. https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_... base/memory/discardable_memory_provider_unittest.cc:137: MemoryPressureListener::NotifyMemoryPressure( On 2013/10/01 19:05:05, willchan wrote: > You should add a test with a DiscardableMemory on a different thread. I added a basic test that uses a DiscardableMemory on a different thread.
FYI, I'm at a conference for this week, so I'll be slow. On Wed, Oct 9, 2013 at 3:40 PM, <reveman@chromium.org> wrote: > Hi all! I'm picking up this patch as it's needed for the new image cache > system > I'm working on and vollick@ doesn't have cycles for this at the moment. > > I've uploaded a new patch that I think address prior review feedback. It's > a > significant refactor of the previous patch. The main difference is that the > provider is no longer making callbacks into the DiscardableMemory > instances. The > memory associated with a DiscardableMemory instance is also owned by the DM > instance when locked and by the provider when it's unlocked and can be > purged. > That's supposed to make it clear what memory can be purged but it might be > cleaner if the provider just always owns the memory. Let me know what you > prefer. > > Please take a look. > > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory.h<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory.h> > File base/memory/discardable_**memory.h (right): > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory.h#**newcode16<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory.h#newcode16> > base/memory/discardable_**memory.h:16: class DiscardableMemoryProvider; > On 2013/10/01 18:12:05, willchan wrote: > >> No indentation >> > > Done. > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory.h#**newcode47<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory.h#newcode47> > base/memory/discardable_**memory.h:47: // contention for the > instances. > On 2013/10/01 18:12:05, willchan wrote: > >> It's more technically correct to say there are no races, rather than >> > there is no > >> contention. Contention would be true if you actually used a mutex. >> > > Done. > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory.h#**newcode102<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory.h#newcode102> > base/memory/discardable_**memory.h:102: friend class > base::internal::**DiscardableMemoryProvider; > On 2013/10/01 18:12:05, willchan wrote: > >> You can ditch the base::. >> > > Done. > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory_**emulated.cc<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory_emulated.cc> > File base/memory/discardable_**memory_emulated.cc (right): > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory_**emulated.cc#newcode50<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory_emulated.cc#newcode50> > base/memory/discardable_**memory_emulated.cc:50: > DiscardableMemoryProvider::**GetInstance()->PurgeAll(); > On 2013/10/01 18:12:05, willchan wrote: > >> This is kind of a big hammer. No need to prematurely optimize, but >> > perhaps leave > >> a note to say this might be worth optimizing later on rather than >> > purging all > >> instances? >> > > My latest patch is purging the minimal amount of LRU memory in these > cases. > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory_**provider.cc<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory_provider.cc> > File base/memory/discardable_**memory_provider.cc (right): > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory_**provider.cc#newcode46<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory_provider.cc#newcode46> > base/memory/discardable_**memory_provider.cc:46: for (; it != > allocations_.end(); ++it) > On 2013/10/01 18:47:25, willchan wrote: > >> Mind putting braces around this? I know it works, but some people >> > unfamiliar to > >> C++ don't know how these line/statement rules work, especially when >> > there's a > >> 2-line statement within a loop statement. Braces make it more clear. >> > > Done. > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory_**provider.cc#newcode65<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory_provider.cc#newcode65> > base/memory/discardable_**memory_provider.cc:65: void > DiscardableMemoryProvider::**NotifyMemoryPressure( > On 2013/10/01 18:47:25, willchan wrote: > >> OK, there's a bug here. NotifyMemoryPressure will always be called on >> > whatever > >> thread the DiscardableMemoryProvider is created on, which is undefined >> > due to > >> the lazy creation when the first DiscardableMemoryEmulated is created. >> > > Now, all these Purge*() calls happen on the notification thread, not >> > necessarily > >> the DiscardableMemoryEmulated creation thread. >> > DiscardableMemoryEmulated is not > >> threadsafe. This means memory pressure notifications can cause races. >> > > This is supposed to be fixed in my latest patch. PTAL. > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory_**provider.cc#newcode75<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory_provider.cc#newcode75> > base/memory/discardable_**memory_provider.cc:75: NOTREACHED(); > Note: I removed the default case and moved the NOTREACHED out of switch > statement so the compiler will force us to update this code if we ever > add new MEMORY_PRESSURE_* values. > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory_**provider.h<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory_provider.h> > File base/memory/discardable_**memory_provider.h (right): > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory_**provider.h#newcode46<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory_provider.h#newcode46> > base/memory/discardable_**memory_provider.h:46: class BASE_EXPORT > DiscardableMemoryProvider { > On 2013/10/01 18:47:25, willchan wrote: > >> BASE_EXPORT_PRIVATE? >> > > Done. > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory_**provider.h#newcode70<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory_provider.h#newcode70> > base/memory/discardable_**memory_provider.h:70: friend class > base::DiscardableMemory; > On 2013/10/01 18:47:25, willchan wrote: > >> Can you just move the necessary private member functions to the public >> > section > >> and ditch the friends? This is already listed in ::internal and should >> > be > >> BASE_EXPORT_PRIVATE, so no one should be able to access this. >> > > This way it's more obvious to me what private members >> > DiscardableMemoryEmulated > >> needs to access. >> > > Done. > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory_**provider.h#newcode82<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory_provider.h#newcode82> > base/memory/discardable_**memory_provider.h:82: // provider is retained by > the caller. > On 2013/10/01 18:47:25, willchan wrote: > >> You should note that this should be called prior to any >> > DiscardableMemory being > >> used, since this setter is not threadsafe. >> > > Done. > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory_**provider_unittest.cc<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory_provider_unittest.cc> > File base/memory/discardable_**memory_provider_unittest.cc (right): > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory_**provider_unittest.cc#newcode78<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory_provider_unittest.cc#newcode78> > base/memory/discardable_**memory_provider_unittest.cc:**78: > ASSERT_NE(static_cast<void*>(**NULL), Memory(discardable)); > On 2013/10/01 19:05:05, willchan wrote: > >> You should use EXPECT_*() unless the following statements would crash >> > otherwise, > >> or it would trigger way too many errors and not be useful. Since these >> > are > >> checking the various accessors which are independent, it's good to >> > EXPECT_* on > >> them separately. >> > > Done. > > https://codereview.chromium.**org/17106004/diff/102001/base/** > memory/discardable_memory_**provider_unittest.cc#**newcode137<https://codereview.chromium.org/17106004/diff/102001/base/memory/discardable_memory_provider_unittest.cc#newcode137> > base/memory/discardable_**memory_provider_unittest.cc:**137: > MemoryPressureListener::**NotifyMemoryPressure( > On 2013/10/01 19:05:05, willchan wrote: > >> You should add a test with a DiscardableMemory on a different thread. >> > > I added a basic test that uses a DiscardableMemory on a different > thread. > > https://codereview.chromium.**org/17106004/<https://codereview.chromium.org/1... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hey David, I started looking at this. Sorry for the delay. pliard is near the end of his review and will be committing soon, and I'm curious if you've talked to him. This is the review: https://codereview.chromium.org/24988003/. I just want to make sure this new design doesn't conflict too badly. I think you'll definitely get some rebase conflicts. I'm going to continue reviewing the design just to unbottleneck that long tail part, but I won't review the nitty gritty since I'm worried it might change after the rebase.
On 2013/10/17 01:29:40, willchan wrote: > Hey David, I started looking at this. Sorry for the delay. pliard is near the > end of his review and will be committing soon, and I'm curious if you've talked > to him. This is the review: https://codereview.chromium.org/24988003/. I just > want to make sure this new design doesn't conflict too badly. I think you'll > definitely get some rebase conflicts. I'm going to continue reviewing the design > just to unbottleneck that long tail part, but I won't review the nitty gritty > since I'm worried it might change after the rebase. Sounds good. pliard and I have been talking and I don't think the rebase will be too bad. Not sure it makes sense yet but I might want to look at consolidating some of the provider code and pliard's allocator code before landing this. Some feedback on the design of the current patch would be much appreciated.
If you have a design doc on the new image cache architecture, I'd love to read it. Let me know if I misunderstood the locking here. I think you always have to lock in the same order if you want to avoid deadlock, and it looks like sometimes there are different orderings. The removal of the callbacks does seem to eliminate a big source of issues. I'm still working my head through the locking. I kinda wonder if it'd be simpler just to use one lock. Do we anticipate use of so many DiscardableMemory instances that the finer grained locking is worth it? https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... File base/memory/discardable_memory_provider.cc (right): https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:52: return Singleton<DiscardableMemoryProvider>::get(); WDYT about using a LeakyLazyInstance instead? Is there a good reason to delete stuff on shutdown? https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:102: AutoLock lock(allocations_lock_); You acquire this lock here first. Later on, you may try to acquire bytes_allocated_lock_ at line 108. Purge() grabs bytes_allocated_lock_ first. And then allocations_lock_. What happens when Unregister() and Purge() execute concurrently? Do we deadlock? https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... File base/memory/discardable_memory_provider.h (right): https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... base/memory/discardable_memory_provider.h:89: bool IsRegisteredForTest(const DiscardableMemory* discardable); Make these test functions const if they don't mutate state? https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... base/memory/discardable_memory_provider.h:103: Allocation(size_t bytes) explicit
Thanks for the review. Small design doc for new image cache here: https://docs.google.com/a/chromium.org/document/d/1QmNOMRLFJoLp0s-VkIGu2d-2UU... https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... File base/memory/discardable_memory_provider.cc (right): https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:52: return Singleton<DiscardableMemoryProvider>::get(); On 2013/10/17 02:06:13, willchan wrote: > WDYT about using a LeakyLazyInstance instead? Is there a good reason to delete > stuff on shutdown? Not really. I guess we need to use a LeakyLazyInstance to allow the use of Singleton<DiscardableMemory> but not sure that's necessary. Seems like there would be a bit more code to use LeakyLazyInstance but I'll switch to that if preferred? https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:102: AutoLock lock(allocations_lock_); On 2013/10/17 02:06:13, willchan wrote: > You acquire this lock here first. Later on, you may try to acquire > bytes_allocated_lock_ at line 108. > > Purge() grabs bytes_allocated_lock_ first. And then allocations_lock_. > > What happens when Unregister() and Purge() execute concurrently? Do we deadlock? Yes, the use of multiple locks here is broken. Reduced it to just one lock in latest patch, which makes everything much simpler and hopefully that's good enough. vollick@, I assume the use of two locks in your original patch was for performance reasons, correct? Did you do some testing to determine that this was needed? https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... File base/memory/discardable_memory_provider.h (right): https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... base/memory/discardable_memory_provider.h:89: bool IsRegisteredForTest(const DiscardableMemory* discardable); On 2013/10/17 02:06:13, willchan wrote: > Make these test functions const if they don't mutate state? Done. https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... base/memory/discardable_memory_provider.h:103: Allocation(size_t bytes) On 2013/10/17 02:06:13, willchan wrote: > explicit Done.
https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... File base/memory/discardable_memory_provider.cc (right): https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:52: return Singleton<DiscardableMemoryProvider>::get(); On 2013/10/19 21:17:04, David Reveman wrote: > On 2013/10/17 02:06:13, willchan wrote: > > WDYT about using a LeakyLazyInstance instead? Is there a good reason to delete > > stuff on shutdown? > > Not really. I guess we need to use a LeakyLazyInstance to allow the use of > Singleton<DiscardableMemory> but not sure that's necessary. Seems like there > would be a bit more code to use LeakyLazyInstance but I'll switch to that if > preferred? Not deleting on shutdown is preferable. It avoids shutdown crashes (when not all threads are joined) and makes shutdown quicker (less deletion means less work on shutdown). If you need something done on shutdown, then by all means, use LazyInstance/Singleton, but otherwise, make it leaky. LazyInstance also helps avoids fragmentation by allocating in a global segment rather than in heap. https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... File base/memory/discardable_memory_provider.cc (right): https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:43: DCHECK_EQ(0u, allocations_.size()); DCHECK(allocations_.empty()); size() is linear in the container's size, as per http://www.sgi.com/tech/stl/Container.html.
https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... File base/memory/discardable_memory_provider.cc (right): https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:132: if (bytes > discardable_memory_limit_) I'm confused. The comment for this variable says: // The maximum number of bytes of discardable memory that may be allocated // before we assume moderate memory pressure. Why does this mean that we should fail to acquire DiscardableMemory? https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:162: bool DiscardableMemoryProvider::IsRegisteredForTest( Please order functions in the .cc file to match the .h file. This is listed in the Chromium style guide: http://www.chromium.org/developers/coding-style#Code_Formatting. https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:184: lock_.AssertAcquired(); Sometimes people use a naming convention to make it more obvious to readers which functions expect "the" lock to already be held. Locked() or _Locked() as the suffix can accomplish this. This makes it easier when reading a function body to see that a lock is held prior to calling a Locked() function. Right now, I have to read ahead to the called function body to see what its expectations are. Up to you though. https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... File base/memory/discardable_memory_provider.h (right): https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... base/memory/discardable_memory_provider.h:75: // NULL if an error occurred or discardable memory limit has been reached. This comment does not discuss what happens to |purged| when returning NULL, although it should be reasonably obvious when you think about it. I suggest rewording to clarify though. """ Returns NULL if an error occurred or the discardable memory limit has been reached. Otherwise, returns the backing buffer and sets |purged| to indicate whether or not the backing buffer has been purged since last use. """ https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... base/memory/discardable_memory_provider.h:80: // to the provider where it can be purged it necessary. if necessary https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... base/memory/discardable_memory_provider.h:119: void PurgeLRU(size_t limit); It's not obvious from the function name what |limit| is for. You explain it here in the comment though. At the risk of being overly verbose, what do you think about a more descriptive function name that hints at what |limit| is for?
Rebased and addressed review feedback. PTAL. https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... File base/memory/discardable_memory_provider.cc (right): https://codereview.chromium.org/17106004/diff/121001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:52: return Singleton<DiscardableMemoryProvider>::get(); On 2013/10/21 18:30:27, willchan wrote: > On 2013/10/19 21:17:04, David Reveman wrote: > > On 2013/10/17 02:06:13, willchan wrote: > > > WDYT about using a LeakyLazyInstance instead? Is there a good reason to > delete > > > stuff on shutdown? > > > > Not really. I guess we need to use a LeakyLazyInstance to allow the use of > > Singleton<DiscardableMemory> but not sure that's necessary. Seems like there > > would be a bit more code to use LeakyLazyInstance but I'll switch to that if > > preferred? > > Not deleting on shutdown is preferable. It avoids shutdown crashes (when not all > threads are joined) and makes shutdown quicker (less deletion means less work on > shutdown). If you need something done on shutdown, then by all means, use > LazyInstance/Singleton, but otherwise, make it leaky. > > LazyInstance also helps avoids fragmentation by allocating in a global segment > rather than in heap. Thanks for the explanation. That makes sense. It's now a leaky LazyInstance. https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... File base/memory/discardable_memory_provider.cc (right): https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:43: DCHECK_EQ(0u, allocations_.size()); On 2013/10/21 18:30:27, willchan wrote: > DCHECK(allocations_.empty()); size() is linear in the container's size, as per > http://www.sgi.com/tech/stl/Container.html. Done. https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:132: if (bytes > discardable_memory_limit_) On 2013/10/21 21:29:12, willchan wrote: > I'm confused. The comment for this variable says: > // The maximum number of bytes of discardable memory that may be allocated > // before we assume moderate memory pressure. > > Why does this mean that we should fail to acquire DiscardableMemory? Talked to vollick@ and I don't think we should have this fail unless malloc fails. Latest patch tries to reduce the memory usage if needed but doesn't fail. https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:162: bool DiscardableMemoryProvider::IsRegisteredForTest( On 2013/10/21 21:29:12, willchan wrote: > Please order functions in the .cc file to match the .h file. This is listed in > the Chromium style guide: > http://www.chromium.org/developers/coding-style#Code_Formatting. Done. https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... base/memory/discardable_memory_provider.cc:184: lock_.AssertAcquired(); On 2013/10/21 21:29:12, willchan wrote: > Sometimes people use a naming convention to make it more obvious to readers > which functions expect "the" lock to already be held. Locked() or _Locked() as > the suffix can accomplish this. This makes it easier when reading a function > body to see that a lock is held prior to calling a Locked() function. Right now, > I have to read ahead to the called function body to see what its expectations > are. Up to you though. Added "WithLockAcquired" to the name of the functions that require the lock to be held when called. https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... File base/memory/discardable_memory_provider.h (right): https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... base/memory/discardable_memory_provider.h:75: // NULL if an error occurred or discardable memory limit has been reached. On 2013/10/21 21:29:12, willchan wrote: > This comment does not discuss what happens to |purged| when returning NULL, > although it should be reasonably obvious when you think about it. I suggest > rewording to clarify though. > > """ > Returns NULL if an error occurred or the discardable memory limit has been > reached. > Otherwise, returns the backing buffer and sets |purged| to indicate whether or > not > the backing buffer has been purged since last use. > """ Done. https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... base/memory/discardable_memory_provider.h:80: // to the provider where it can be purged it necessary. On 2013/10/21 21:29:12, willchan wrote: > if necessary Done. https://codereview.chromium.org/17106004/diff/130001/base/memory/discardable_... base/memory/discardable_memory_provider.h:119: void PurgeLRU(size_t limit); On 2013/10/21 21:29:12, willchan wrote: > It's not obvious from the function name what |limit| is for. You explain it here > in the comment though. At the risk of being overly verbose, what do you think > about a more descriptive function name that hints at what |limit| is for? At the risk of making it too long, it's now "PurgeLRUWithLockAcquiredUntilUsageIsWithin(size_t limit)".
OK, I'm happy with this CL, mod testing feedback from jyasskin. LGTM, but please wait for him to OK the value parameterized test or suggest a better way of doing it. https://codereview.chromium.org/17106004/diff/220001/base/memory/discardable_... File base/memory/discardable_memory_provider_unittest.cc (right): https://codereview.chromium.org/17106004/diff/220001/base/memory/discardable_... base/memory/discardable_memory_provider_unittest.cc:185: #define DiscardableMemoryProviderPermutions(name, pressure) \ My instinct is that you can replace this with value parameterized tests (https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...), but I don't know for sure. I'm adding jyasskin for his advice here.
+jamesr for webkit/ +reed for skia/ https://codereview.chromium.org/17106004/diff/220001/base/memory/discardable_... File base/memory/discardable_memory_provider_unittest.cc (right): https://codereview.chromium.org/17106004/diff/220001/base/memory/discardable_... base/memory/discardable_memory_provider_unittest.cc:185: #define DiscardableMemoryProviderPermutions(name, pressure) \ On 2013/10/29 01:44:59, willchan wrote: > My instinct is that you can replace this with value parameterized tests > (https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...), > but I don't know for sure. I'm adding jyasskin for his advice here. I went ahead and parameterized these tests in latest patch as that makes a lot of sense. I'll still wait for feedback from jyasskin in case he has some additional advice.
NM, you figured it out, and I forgot to add jyasskin :) Go ahead and submit when you get the other reviewers' lgtms.
webkit lgtm (although I think you've already got a stamp for it)
Skia LGTM
Skia LGTM from the other account.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vollick@chromium.org/17106004/690001
Message was sent while issue was closed.
Change committed as 231845
Message was sent while issue was closed.
https://codereview.chromium.org/17106004/diff/690001/base/memory/discardable_... File base/memory/discardable_memory_emulated.cc (right): https://codereview.chromium.org/17106004/diff/690001/base/memory/discardable_... base/memory/discardable_memory_emulated.cc:70: if (memory->Lock() != DISCARDABLE_MEMORY_PURGED) I was playing on Linux with discardable memory (thanks to this CL :)) and came across this line. Is the use of DISCARDABLE_MEMORY_PURGED here (instead of DISCARDABLE_MEMORY_SUCCESS) intentional?
Message was sent while issue was closed.
On 2013/11/05 12:41:38, Philippe wrote: > https://codereview.chromium.org/17106004/diff/690001/base/memory/discardable_... > File base/memory/discardable_memory_emulated.cc (right): > > https://codereview.chromium.org/17106004/diff/690001/base/memory/discardable_... > base/memory/discardable_memory_emulated.cc:70: if (memory->Lock() != > DISCARDABLE_MEMORY_PURGED) > I was playing on Linux with discardable memory (thanks to this CL :)) and came > across this line. Is the use of DISCARDABLE_MEMORY_PURGED here (instead of > DISCARDABLE_MEMORY_SUCCESS) intentional? Yep, it is intentional, though perhaps in need of a comment. When Acquire returns 'PURGED' it means, "yes, I was able to obtain locked memory for you, but it could contain anything." This should be the case when we create a new discardable since this is the first time we allocate its memory. It may be the case that this will return 'FAILED' (because allocation doesn't succeed), but it should never return 'SUCCESS' on the first acquire.
Message was sent while issue was closed.
On 2013/11/05 13:16:18, Ian Vollick wrote: > On 2013/11/05 12:41:38, Philippe wrote: > > > https://codereview.chromium.org/17106004/diff/690001/base/memory/discardable_... > > File base/memory/discardable_memory_emulated.cc (right): > > > > > https://codereview.chromium.org/17106004/diff/690001/base/memory/discardable_... > > base/memory/discardable_memory_emulated.cc:70: if (memory->Lock() != > > DISCARDABLE_MEMORY_PURGED) > > I was playing on Linux with discardable memory (thanks to this CL :)) and came > > across this line. Is the use of DISCARDABLE_MEMORY_PURGED here (instead of > > DISCARDABLE_MEMORY_SUCCESS) intentional? > > Yep, it is intentional, though perhaps in need of a comment. When Acquire > returns 'PURGED' it means, "yes, I was able to obtain locked memory for you, but > it could contain anything." This should be the case when we create a new > discardable since this is the first time we allocate its memory. It may be the > case that this will return 'FAILED' (because allocation doesn't succeed), but it > should never return 'SUCCESS' on the first acquire. Great, thanks Ian. |
