Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9289052:
Adding GpuMemoryManager to track GpuCommandBufferStub visibility and last_used_time and dictate mem… (Closed)
|
Created:
8 years, 11 months ago by mmocny Modified:
8 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdding GpuMemoryManager to track GpuCommandBufferStub visibility and last_used_time and dictate memory allocations
This is initial work using partial information to start making decisions on memory allocation for various RenderWidgets/GraphicsContexts.
Subsequent work will fill in some of the missing information, and add do-somethings where we currently do-nothing -- namely implementing GpuCommandBufferSub::SetMemoryAllocation(...).\
BUG=111967
TEST=content_unittests GpuMemoryManager tests added
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120339
Patch Set 1 #Patch Set 2 : Changing to use gpu_memory_allocation.h file so it lands first #
Total comments: 33
Patch Set 3 : Changing to use surface_id instead of render_widget_id, and made testable #Patch Set 4 : Minor updates, working on tests #
Total comments: 83
Patch Set 5 : Fixing issues identified by reviewers. Tests are not complete yet. #Patch Set 6 : Fixing a few more chromium style issues I missed, and making the WeakPtr factory change. #Patch Set 7 : Adding a bunch of tests. Helped find a bug introduced with a recent refactor. #Patch Set 8 : Removing lines of slashes as that is not the convention in gpu #Patch Set 9 : Changing tests to use a test harness for setup, and splitting big test into several smaller. #
Total comments: 17
Patch Set 10 : . #
Total comments: 26
Patch Set 11 : Moving the affected_surface_ids structure to subsequent patch since it isnt used anyway. #Patch Set 12 : . #Patch Set 13 : . #Patch Set 14 : Changing surface_state pointer to const-ref with a bool has_*() #
Total comments: 6
Patch Set 15 : . #Patch Set 16 : Moving inlined functions back out since they were virtuals, which is against chromium style. #Patch Set 17 : Adding CONTENT_EXPORT to SurfaceComparer to fix link error on win #Patch Set 18 : . #Messages
Total messages: 24 (0 generated)
I believe this is ready for review. Part 2 and 3 are referenced in the description of this CL. I have more testing to do for cases like session restore and gpu process crashing (both have been tested at least a bit), but I would love to get early feedback if I am veering in the wrong direction anywhere.
We need a 1:1 to talk about testability. The manage function needs to be created with tests from the get go. http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_channel.cc File content/common/gpu/gpu_channel.cc (right): http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_channel... content/common/gpu/gpu_channel.cc:214: std::vector<GpuCommandBufferStub*> GpuChannel::GetCommandBuffers() { Better to return the stubmap and let the code upstream do the iteration. You're doing a copy here. :( http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_channel... File content/common/gpu/gpu_channel_manager.cc (right): http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_channel... content/common/gpu/gpu_channel_manager.cc:21: ALLOW_THIS_IN_INITIALIZER_LIST(gpu_memory_manager_( not sure if this is the ideal way to do this... you've got scoped ptr so just default initialize then in the actual constructor body gpu_memory_manager_.reset(this). alternatively, make the channelmanager have the memory manager by value rather than a pointer. At that point you'd need this trick. Why's it a pointer/ http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_channel... content/common/gpu/gpu_channel_manager.cc:59: std::vector<GpuChannel*> GpuChannelManager::GetChannels() const { Return the map? You're incurring a copy here. http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_channel... File content/common/gpu/gpu_channel_manager.h (right): http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_channel... content/common/gpu/gpu_channel_manager.h:71: GpuMemoryManager* GetGpuMemoryManager() { return gpu_memory_manager_.get(); } Is this our convention in chrome? I thought it was gpu_memory_manager() for member accessors. http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_command... File content/common/gpu/gpu_command_buffer_stub.cc (right): http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_command... content/common/gpu/gpu_command_buffer_stub.cc:512: channel_->gpu_channel_manager()->GetGpuMemoryManager()->Manage(); Not liking this. I would rather you modify the upper level plumbing to hit manage() after every PutChanged, for instance. Or, add a method on GpuMemoryManager saying ScheduleManage() that itself contains the logic about when to manage. http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_command... content/common/gpu/gpu_command_buffer_stub.cc:530: return std::vector<int>(1,int(route_id_)); woah, what? You can wire this up already if you think about it. When you create a stub, you set whether it is rendering to a render_view_ --- I think that if thats nonzero, it should be immediately added to the affected_render_widget_ids_ vector. I made a similar comment about this in the chrome-side extensions patch. http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_command... content/common/gpu/gpu_command_buffer_stub.cc:531: } Shouldn't you be using render_view_id, not route_id_? Is this why your merge stuff is wonky? http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_command... content/common/gpu/gpu_command_buffer_stub.cc:536: // 2. Move drop backbuffer hook from SetSurfaceVisibile to here Woah, this will have side effects other than just sending? I think you should blow this up into a Send which you should do right away. 2-4 should be a different function call on this class. its fine if the memory manager makes two calls to this class. Its a huge win to make your functions do clear and well defined things rather than have 4 side effects. You'll remove these printfs right? http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_command... File content/common/gpu/gpu_command_buffer_stub.h (right): http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_command... content/common/gpu/gpu_command_buffer_stub.h:77: // visibility state of associated render widgets. Visibility, upper cap http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_command... content/common/gpu/gpu_command_buffer_stub.h:79: bool visible() const { return visible_; } Please make a visibility_known accessor and update comment accordingly so that this validity is encoded in the accessor rather than a doc. Make visible() fail (i forget the chorme equivalent of ASSERT_NOT_REACHED if its unknown.) http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_command... content/common/gpu/gpu_command_buffer_stub.h:81: // last_used_time is updated during visibility changes, and can be used for // Comments are sentences and thus the first word is capitalized. :) http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_command... content/common/gpu/gpu_command_buffer_stub.h:82: // LRU ordering in combination with visibility state Not sure the lru comment is useful here. Why are you using int64 instead of base::TimeTicks? http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_command... content/common/gpu/gpu_command_buffer_stub.h:94: std::vector<int> render_widget_ids() const; Consistency! affected_render_widget_ids. Then you dont need a comment since its obvious http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_command... content/common/gpu/gpu_command_buffer_stub.h:101: void SetMemoryAllocation(GpuMemoryAllocation const& allocation); This sends not sets, right? Call things by what they do. http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_memory_... File content/common/gpu/gpu_memory_manager.cc (right): http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_memory_... content/common/gpu/gpu_memory_manager.cc:26: RenderWidgetDescriptor(int render_widget_id, This comment applies to this entire patch. but we need to confirm the naming here and convention in the gpu proces for these ids. I notice command_buffer_stub is calling these render_view_ids. Even though by this point, they're actually render_widgets. This isn't per se your fault, but we're adding to the mess and should clean as we go. Ping @kbr, establish a convention in a hurry, and normalize. http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_memory_... content/common/gpu/gpu_memory_manager.cc:42: // This is a helper function to learn from another descriptor for the same ??? when do you merge? This feels like some serious voodoo that makes me think something is wrong further up. http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_memory_... content/common/gpu/gpu_memory_manager.cc:84: }; Unit tests, or file a bug and write unit tests after m18 crisis ends. Seriously. Let me know if you've not used gtest before and I'll help you through the first few. Its stupid easy once you've cut your teeth. http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_memory_... content/common/gpu/gpu_memory_manager.cc:107: std::vector<RenderWidgetDescriptor*> ComputeRenderWidgetDescriptorsFromStubs( Why are you doing this? Why not just get a vector of stubs, sort that directly, make the sorter above be a sorter of the stubs? I dont see any value you're creating in converting stubs into something else... http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_memory_... content/common/gpu/gpu_memory_manager.cc:221: No tests? That is scary. It loosk like we dont have mock gpu command buffer stubs. Shame on us! For this, I want this tested from the get go. Not optional. Lets do a 1:1 ASAP to find a way to get unit testability here. http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_memory_... File content/common/gpu/gpu_memory_manager.h (right): http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_memory_... content/common/gpu/gpu_memory_manager.h:23: I would like it if this had a ScheduleManage and Manage, and that the ScheduleManage used a postTask technique to actually schedule a manage call. e.g. manage_scheduled_ = false; And if (!manage_scheduled_) MessageLoop::current()->PostTask( FROM_HERE, base::Bind(&GpuMemorYManager::manage, this));
http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_channel.cc File content/common/gpu/gpu_channel.cc (right): http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_channel... content/common/gpu/gpu_channel.cc:214: std::vector<GpuCommandBufferStub*> GpuChannel::GetCommandBuffers() { On 2012/01/27 10:10:13, nduca wrote: > Better to return the stubmap and let the code upstream do the iteration. You're > doing a copy here. :( Returning the StubMap would expose more implementation details of the GpuChannel. If this isn't performance critical code then returning a vector seems fine to me. http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_memory_... File content/common/gpu/gpu_memory_manager.cc (right): http://codereview.chromium.org/9289052/diff/11/content/common/gpu/gpu_memory_... content/common/gpu/gpu_memory_manager.cc:221: On 2012/01/27 10:10:13, nduca wrote: > No tests? That is scary. > > It loosk like we dont have mock gpu command buffer stubs. Shame on us! > > For this, I want this tested from the get go. Not optional. Lets do a 1:1 ASAP > to find a way to get unit testability here. +1 to testing this from the start. I haven't been paying attention to the design discussions in this area, but upon first glance this doesn't look to me like the right way of going about this. This method is starting from scratch every time it runs, and iterating through a potentially large number of objects, potentially frequently. There's really no excuse for doing this. There are concrete events that occur when tabs are foregrounded, backgrounded, created, destroyed, etc., and we should be hooking in to those and doing a small, incremental amount of work each time. There should be assertions that our memory tracking code knows what is going on; for example, the allocated memory should go to zero when all of the tabs have been closed. There are similar assertions in the command buffer code and associated unit tests. I would suggest you look at that code and talk with Gregg if you have questions about how strict this tracking framework should be.
The patch will come soon, but here are my replies to comments. https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... File content/common/gpu/gpu_channel.cc (right): https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... content/common/gpu/gpu_channel.cc:214: std::vector<GpuCommandBufferStub*> GpuChannel::GetCommandBuffers() { I had it that way at first, but this was cleaner. The map is a nasty data structure for external clients to know about.. And can't we rely on return value optimization here? Its an easy change so I won't argue too much. On 2012/01/27 10:10:13, nduca wrote: > Better to return the stubmap and let the code upstream do the iteration. You're > doing a copy here. :( https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... File content/common/gpu/gpu_channel_manager.cc (right): https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... content/common/gpu/gpu_channel_manager.cc:21: ALLOW_THIS_IN_INITIALIZER_LIST(gpu_memory_manager_( Changed to store by value. On 2012/01/27 10:10:13, nduca wrote: > not sure if this is the ideal way to do this... you've got scoped ptr so just > default initialize then in the actual constructor body > gpu_memory_manager_.reset(this). > > alternatively, make the channelmanager have the memory manager by value rather > than a pointer. At that point you'd need this trick. Why's it a pointer/ https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... content/common/gpu/gpu_command_buffer_stub.cc:512: channel_->gpu_channel_manager()->GetGpuMemoryManager()->Manage(); Added ScheduleManage. On 2012/01/27 10:10:13, nduca wrote: > Not liking this. I would rather you modify the upper level plumbing to hit > manage() after every PutChanged, for instance. Or, add a method on > GpuMemoryManager saying ScheduleManage() that itself contains the logic about > when to manage. https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... content/common/gpu/gpu_command_buffer_stub.cc:530: return std::vector<int>(1,int(route_id_)); I cannot find any reference to render_view during stub creation. This return value was used as a temporary placeholder during testing, I will change it to return a reference to an empty vector. On 2012/01/27 10:10:13, nduca wrote: > woah, what? You can wire this up already if you think about it. When you create > a stub, you set whether it is rendering to a render_view_ --- I think that if > thats nonzero, it should be immediately added to the affected_render_widget_ids_ > vector. I made a similar comment about this in the chrome-side extensions patch. https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... content/common/gpu/gpu_command_buffer_stub.cc:536: // 2. Move drop backbuffer hook from SetSurfaceVisibile to here You are correct on all points, this was leftover debugging stuff that should have been removed On 2012/01/27 10:10:13, nduca wrote: > Woah, this will have side effects other than just sending? I think you should > blow this up into a Send which you should do right away. > > 2-4 should be a different function call on this class. its fine if the memory > manager makes two calls to this class. Its a huge win to make your functions do > clear and well defined things rather than have 4 side effects. > > You'll remove these printfs right? https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... File content/common/gpu/gpu_command_buffer_stub.h (right): https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... content/common/gpu/gpu_command_buffer_stub.h:82: // LRU ordering in combination with visibility state For no good reason whatsoever. Changed, thanks. On 2012/01/27 10:10:13, nduca wrote: > Not sure the lru comment is useful here. > > Why are you using int64 instead of base::TimeTicks? https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... File content/common/gpu/gpu_memory_manager.cc (right): https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... content/common/gpu/gpu_memory_manager.cc:42: // This is a helper function to learn from another descriptor for the same This is a convenience function which I will move out of this class. It is used since more than one stub/context may be affecting a given render widget, and we want to merge each of their "descriptors" into a single one. On 2012/01/27 10:10:13, nduca wrote: > ??? when do you merge? This feels like some serious voodoo that makes me think > something is wrong further up. https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... content/common/gpu/gpu_memory_manager.cc:84: }; This should be easy to test. On 2012/01/27 10:10:13, nduca wrote: > Unit tests, or file a bug and write unit tests after m18 crisis ends. Seriously. > Let me know if you've not used gtest before and I'll help you through the first > few. Its stupid easy once you've cut your teeth. https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... content/common/gpu/gpu_memory_manager.cc:107: std::vector<RenderWidgetDescriptor*> ComputeRenderWidgetDescriptorsFromStubs( I promise there was a method to this madness but I think I've come up with a way to accomplish what you want, and it may indeed be cleaner. On 2012/01/27 10:10:13, nduca wrote: > Why are you doing this? Why not just get a vector of stubs, sort that directly, > make the sorter above be a sorter of the stubs? I dont see any value you're > creating in converting stubs into something else... https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... content/common/gpu/gpu_memory_manager.cc:221: ok. On 2012/01/27 10:10:13, nduca wrote: > No tests? That is scary. > > It loosk like we dont have mock gpu command buffer stubs. Shame on us! > > For this, I want this tested from the get go. Not optional. Lets do a 1:1 ASAP > to find a way to get unit testability here. https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... File content/common/gpu/gpu_memory_manager.h (right): https://chromiumcodereview.appspot.com/9289052/diff/11/content/common/gpu/gpu... content/common/gpu/gpu_memory_manager.h:23: Done. On 2012/01/27 10:10:13, nduca wrote: > I would like it if this had a ScheduleManage and Manage, and that the > ScheduleManage used a postTask technique to actually schedule a manage call. > > e.g. > manage_scheduled_ = false; > And > if (!manage_scheduled_) > MessageLoop::current()->PostTask( > FROM_HERE, base::Bind(&GpuMemorYManager::manage, this));
http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_chan... File content/common/gpu/gpu_channel.h (right): http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_chan... content/common/gpu/gpu_channel.h:79: virtual void AppendAllCommandBufferStubs( Overkill. Just append them all. Then filter then as a second pass inside the consumer. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_comm... File content/common/gpu/gpu_command_buffer_stub.cc (right): http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.cc:48: surface_state_(), once you get this sorted, I think you should initialize this directly with surface_id in the constructor. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.cc:531: return affected_surface_ids_; does affected_surface_ids for a view-command-buffer-stub affect its own surface id? E.g. if this->surface.surface_id = 7, then is affected_surface_ids() = [] or affected_surface_ids() = [7] Add a comment explaining clearly which it is in the .h for this method. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.cc:536: // TODO(mmocny): Send callback once gl extensions are added sentences end with . http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_comm... File content/common/gpu/gpu_command_buffer_stub.h (right): http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.h:152: GpuSurfaceState surface_state_; Isn't there a surface_id_ on this class already? Lets avoid having duplication between surface_state_ and fields on the stub http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.h:152: GpuSurfaceState surface_state_; The whole point of this structure is to have it be null when there's no surface... E.g. this should be a scoped_ptr<SurfaceState> Also, I'm a little confused about the GLSurface* --- that maybe should just be on the surface state. E.g. Stub { Surface { GLSurface* gl_surface; int surface_id; .... } scoped_ptr<Surface> surface_; } I.e. no duplication on the stub. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_allocation.h (right): http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_allocation.h:13: // GpuIdealMemoryAllocation values (see below). I think the ", and ... values " doesn't really add value. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_allocation.h:14: // Exceeding these limits for an unreasonable amount of time will cause kill. will cause the context to be lost. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_allocation.h:16: static const int kResourceSizeForegroundTab = 100 * 1024 * 1024; Confused why this is in the header. I would have expected this to be only in gpu_memory_manger.cc http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_allocation.h:39: // Contexts should give ideal memory allocation hints to the GpuMemoryManager Do we like "ideal" or do we want to try on "goal" as an alternative word? E.g. GpuMemoryAllocationGoal Dunno http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_allocation.h:41: // If memory is constrained and/or this context has lower priority than others, this context -> a context. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_allocation.h:42: // these requests will be overruled (see actual allocation limits above). these requests will be overruled -> the allocated values may be lower than the stated goals. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_management.h (right): http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_management.h:6: #define CONTENT_COMMON_GPU_GPU_MEMORY_MANAGEMENT_H_ OK , so this file shouldn't exist. These individual classes should be classes in their associated class' files. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_management.h:20: struct GpuSurfaceState { This I think should be a public inner struct called SurfaceState inside GpuCommandBufferShim http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_management.h:28: class GpuMemoryManageableCommandBufferStub { This should be a class in GpuCommandBufferStub and should be either a) GpuCommandBufferStubBase or b) GpuCommandBufferStub and we rename the existing class to be CommandBufferStubImplmpl. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_management.h:34: virtual void SendMemoryAllocation(const GpuMemoryAllocation& allocation) = 0; SendMemoryAllocationToProxy? http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_management.h:37: class GpuMemoryManagerClient { This class should be defined in gpu_memory_manager.h, just before the actual GpuMemoryManager class. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_management.h:52: inline bool IsMoreImportant(GpuMemoryManageableCommandBufferStub* lhs, Why both a comparator and a function? Pick one or the other? http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_management.h:66: struct StubComparator { class; struct feels awkward here. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_management.h:66: struct StubComparator { I think this should be an inner class in the GpuMemoryManager header file. Its only really exposed for the purposes of testing. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_manager.cc (right): http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.cc:33: FROM_HERE, base::Bind(&GpuMemoryManager::Manage, base::Unretained(this))); Hmmm isn't this going to crash if the task is posted when the manager dies and the message loop keeps spinning? The trick here is weak references. Look at GpuChannel's weak_factory_ member and its OnScheduled function and copy that. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.cc:36: void GpuMemoryManager::Manage() { I think you should take some of the comments from your allocation classes about the actual 3 limits and also add some text that explains this algorithm. "The current managing algorithm simply classifies tabs as "foreground", "background" and "hibernated." For each of those three categories, there are predefined limits. It proceeds in X steps, as follows.... http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.cc:40: std::vector<GpuMemoryManageableCommandBufferStub*> stubs_with_surface; this is where I was saying you should just get all the stubs, then build two vectors from them inside this function. Its really not gonna be a big deal perf wise. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.cc:50: // Create allocations This comment isn't adding a lot of value. You're better doing something like // Set up the three predefined allocation values http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.cc:69: // Separate stubs with surfaces into three sets and send memory allocation This should really be put up in the overall comment for the manage function. The three numbered bullets here are a bit information dense. Use a few more lines and/or prose to make this clearer? Its good information. :) http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.cc:115: // TODO(mmocny): Either (a) no affected surfaces, or Fix this so you put an if(affected_surface_ids().size() == 0) continue up top and some sort of real failure here. DLOG(CRITICAL) << "OMGWTFBBQ" for example. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_manager.h (right): http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.h:13: //////////////////////////////////////////////////////////////////////////////// not sure we do /// delimiters? http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.h:23: void Manage(); You might put a comment on Manage() like "// Public for test" Or, just make it private and use friending. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_manager_unittest.cc (right): http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager_unittest.cc:6: #include "testing/gtest/include/gtest/gtest.h" i think we put a space between the "class header file" (gpu_memory_manager) and the other includes http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager_unittest.cc:8: //////////////////////////////////////////////////////////////////////////////// I dont see the /// pattern being used in the gpu codebase... its in other bits of the browser, though. When in doubt, go with the style in your local directory. Or if you feel strongly, ask @kbr or @apatrick http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager_unittest.cc:9: // Fake Overrides for testing not sure this comment adds value http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager_unittest.cc:11: struct FakeCommandBufferStub : GpuMemoryManageableCommandBufferStub { why struct? http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager_unittest.cc:27: struct FakeClient : GpuMemoryManagerClient { class http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager_unittest.cc:49: FakeCommandBufferStub ret; ret -> stub http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager_unittest.cc:56: FakeCommandBufferStub CreateFakeCommandBufferStubWithoutSurface() { you know, I'm thinking you might be better served by not having this helper funciton. Instead, just create the surface and set all its fields explicitly every time. I find it easier to read test code this way... you'll do a little bit of typing to get it written out but youll be able to read the test itself very very clearly at that point. http://codereview.chromium.org/9289052/diff/11002/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager_unittest.cc:69: FakeCommandBufferStub fake_stub_with_surface1 = i think you're killing yourself with these names. fake_client -> client fake_stub_with_surface1 -> stub1, stub2, etc --- dont bother giving the names to say whether they're with or without surface. If your setup is easy to read [described earlier], you wont have to look hard to figure out what the different stub numbers do. Just to help the newcomer in reading a test, I usually put a comment above the TEST(xxx) macro saying the objective of a test. E.g. // Verifies that the Send method is called on all stubs. You're gonna need more tests than this, I hope you know. At minimum: 1. At least 3 or 4 tests on the comparator. In each individual test, set up pairs of stubs and verify that the comparator orders them correcty. E.g. FakeCommandBufferStub stub1(); stub1.visible = true ... FakeCommndBufferStub stub2(); stub2.visible = false; GpuMemoryManager::StubComparator cmp; EXPECT_TRUE(cmp(stub1,stub2) < 0); http://codereview.chromium.org/9289052/diff/11002/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/9289052/diff/11002/content/content_tests.gypi#... content/content_tests.gypi:262: 'common/gpu/gpu_memory_manager_unittest.cc', you might need to run ./build/gyp_chromium to get this to take
https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... File content/common/gpu/gpu_channel.h (right): https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_channel.h:40: public GpuMemoryManagerClient, Necessary? https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.cc:285: surface_state_.visible = true; If I open up a tab in the background, what happens here? And what about last_used_time_? https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... File content/common/gpu/gpu_memory_management.h (right): https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_management.h:48: I have a feeling that these comparators are only used in GpuMemoryManager.cc. If so, why not define them there? https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... File content/common/gpu/gpu_memory_manager.cc (right): https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager.cc:20: , manage_scheduled_(false) { nit: sorry, but I think this is the WK way, not the Chromium way... (commas trail not lead) https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager.cc:68: AFAICT, these never change. Define them as constants? Perhaps in an anon namespace at the beginning of the file? https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager.cc:80: stub->SendMemoryAllocation(all_buffers_allocation); Is this necessary given the code below? Aren't we sending the allocation twice?
https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... File content/common/gpu/gpu_channel.h (right): https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_channel.h:40: public GpuMemoryManagerClient, Indeed, no. On 2012/01/31 18:13:58, jonathan.backer wrote: > Necessary? https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_channel.h:79: virtual void AppendAllCommandBufferStubs( On 2012/01/31 06:53:47, nduca wrote: > Overkill. Just append them all. Then filter then as a second pass inside the > consumer. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.cc:48: surface_state_(), On 2012/01/31 06:53:47, nduca wrote: > once you get this sorted, I think you should initialize this directly with > surface_id in the constructor. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.cc:285: surface_state_.visible = true; Tabs open in the background get a "WasHidden" message sent, and the state will be update during SetSurfaceVisibility. Tabs open in the foreground are not sent a signal, as they are meant to default to visible. I shouldn't be setting this here, though, the default should be true during construction. As for last_used_time, I should set it along with visibility for consistency. I wasn't setting it because (a) its only used for background tabs and (b) I wanted to differentiate between tabs that have had visibility state changed explicitly and those that did not. Now we have other ways to track that. On 2012/01/31 18:13:58, jonathan.backer wrote: > If I open up a tab in the background, what happens here? And what about > last_used_time_? https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.cc:531: return affected_surface_ids_; On 2012/01/31 06:53:47, nduca wrote: > does affected_surface_ids for a view-command-buffer-stub affect its own surface > id? > > E.g. if this->surface.surface_id = 7, then is > affected_surface_ids() = [] > or > affected_surface_ids() = [7] > > Add a comment explaining clearly which it is in the .h for this method. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.cc:536: // TODO(mmocny): Send callback once gl extensions are added On 2012/01/31 06:53:47, nduca wrote: > sentences end with . Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... File content/common/gpu/gpu_command_buffer_stub.h (right): https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.h:152: GpuSurfaceState surface_state_; On 2012/01/31 06:53:47, nduca wrote: > Isn't there a surface_id_ on this class already? Lets avoid having duplication > between surface_state_ and fields on the stub Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.h:152: GpuSurfaceState surface_state_; GpuMemoryManager doesn't need to know about GLSurface and also, what other members should be changed if GLSurface* is moved.. I don't disagree that this may be correct, but I think it should be out of scope for this CL. On 2012/01/31 06:53:47, nduca wrote: > The whole point of this structure is to have it be null when there's no > surface... E.g. this should be a scoped_ptr<SurfaceState> > > Also, I'm a little confused about the GLSurface* --- that maybe should just be > on the surface state. E.g. > > Stub { > Surface { > GLSurface* gl_surface; > int surface_id; > .... > } > scoped_ptr<Surface> surface_; > } > > I.e. no duplication on the stub. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... File content/common/gpu/gpu_memory_allocation.h (right): https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_allocation.h:13: // GpuIdealMemoryAllocation values (see below). On 2012/01/31 06:53:47, nduca wrote: > I think the ", and ... values " doesn't really add value. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_allocation.h:14: // Exceeding these limits for an unreasonable amount of time will cause kill. On 2012/01/31 06:53:47, nduca wrote: > will cause the context to be lost. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_allocation.h:16: static const int kResourceSizeForegroundTab = 100 * 1024 * 1024; On 2012/01/31 06:53:47, nduca wrote: > Confused why this is in the header. I would have expected this to be only in > gpu_memory_manger.cc Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_allocation.h:39: // Contexts should give ideal memory allocation hints to the GpuMemoryManager I find goal confusing, since I think of the goal to be to use as little memory as possible. This struct is used to request as much as possible. Maybe 'wish' is more appropriate in that sense ;) Perhaps IdealMemoryAllocationRequest to make it more obvious that this is coming from the renderer? On 2012/01/31 06:53:47, nduca wrote: > Do we like "ideal" or do we want to try on "goal" as an alternative word? > E.g. > > GpuMemoryAllocationGoal > > Dunno https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_allocation.h:41: // If memory is constrained and/or this context has lower priority than others, On 2012/01/31 06:53:47, nduca wrote: > this context -> a context. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_allocation.h:42: // these requests will be overruled (see actual allocation limits above). On 2012/01/31 06:53:47, nduca wrote: > these requests will be overruled -> the allocated values may be lower than the > stated goals. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... File content/common/gpu/gpu_memory_management.h (right): https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_management.h:6: #define CONTENT_COMMON_GPU_GPU_MEMORY_MANAGEMENT_H_ On 2012/01/31 06:53:47, nduca wrote: > OK , so this file shouldn't exist. These individual classes should be classes in > their associated class' files. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_management.h:20: struct GpuSurfaceState { On 2012/01/31 06:53:47, nduca wrote: > This I think should be a public inner struct called SurfaceState inside > GpuCommandBufferShim Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_management.h:28: class GpuMemoryManageableCommandBufferStub { On 2012/01/31 06:53:47, nduca wrote: > This should be a class in GpuCommandBufferStub and should be either > a) GpuCommandBufferStubBase or > b) GpuCommandBufferStub and we rename the existing class to be > CommandBufferStubImplmpl. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_management.h:34: virtual void SendMemoryAllocation(const GpuMemoryAllocation& allocation) = 0; On 2012/01/31 06:53:47, nduca wrote: > SendMemoryAllocationToProxy? Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_management.h:37: class GpuMemoryManagerClient { On 2012/01/31 06:53:47, nduca wrote: > This class should be defined in gpu_memory_manager.h, just before the actual > GpuMemoryManager class. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_management.h:48: Also used in tests, but they are moved to GpuMemoryManager header file. On 2012/01/31 18:13:58, jonathan.backer wrote: > I have a feeling that these comparators are only used in GpuMemoryManager.cc. If > so, why not define them there? https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_management.h:52: inline bool IsMoreImportant(GpuMemoryManageableCommandBufferStub* lhs, On 2012/01/31 06:53:47, nduca wrote: > Why both a comparator and a function? Pick one or the other? Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_management.h:66: struct StubComparator { On 2012/01/31 06:53:47, nduca wrote: > I think this should be an inner class in the GpuMemoryManager header file. Its > only really exposed for the purposes of testing. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_management.h:66: struct StubComparator { On 2012/01/31 06:53:47, nduca wrote: > class; struct feels awkward here. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... File content/common/gpu/gpu_memory_manager.cc (right): https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager.cc:20: , manage_scheduled_(false) { On 2012/01/31 18:13:58, jonathan.backer wrote: > nit: sorry, but I think this is the WK way, not the Chromium way... (commas > trail not lead) Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager.cc:40: std::vector<GpuMemoryManageableCommandBufferStub*> stubs_with_surface; On 2012/01/31 06:53:47, nduca wrote: > this is where I was saying you should just get all the stubs, then build two > vectors from them inside this function. Its really not gonna be a big deal perf > wise. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager.cc:50: // Create allocations On 2012/01/31 06:53:47, nduca wrote: > This comment isn't adding a lot of value. You're better doing something like > // Set up the three predefined allocation values Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager.cc:68: On 2012/01/31 18:13:58, jonathan.backer wrote: > AFAICT, these never change. Define them as constants? Perhaps in an anon > namespace at the beginning of the file? Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager.cc:80: stub->SendMemoryAllocation(all_buffers_allocation); The first loop is over stubs_with_surfaces, the second loop is over stubs_without_surface. Sorry, its more confusing than it needs to be. I've updated the comment at the top of this function to explain whats going on here. I can separate this out into two helper functions to make it extra explicit, but perhaps that is overkill. On 2012/01/31 18:13:58, jonathan.backer wrote: > Is this necessary given the code below? Aren't we sending the allocation twice? https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager.cc:115: // TODO(mmocny): Either (a) no affected surfaces, or On 2012/01/31 06:53:47, nduca wrote: > Fix this so you put an if(affected_surface_ids().size() == 0) continue up top > and some sort of real failure here. DLOG(CRITICAL) << "OMGWTFBBQ" for example. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... File content/common/gpu/gpu_memory_manager.h (right): https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager.h:13: //////////////////////////////////////////////////////////////////////////////// I've seen it used, even inside content/common source files. On 2012/01/31 06:53:47, nduca wrote: > not sure we do /// delimiters? https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager.h:23: void Manage(); On 2012/01/31 06:53:47, nduca wrote: > You might put a comment on Manage() like "// Public for test" > > Or, just make it private and use friending. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... File content/common/gpu/gpu_memory_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager_unittest.cc:6: #include "testing/gtest/include/gtest/gtest.h" On 2012/01/31 06:53:47, nduca wrote: > i think we put a space between the "class header file" (gpu_memory_manager) and > the other includes Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager_unittest.cc:8: //////////////////////////////////////////////////////////////////////////////// I saw usage all over content/common.. I didn't realize we had micro standards in each subdirectory. Someone has to introduce the trend, no? @backer is also OWNER, so I'll ask his opinion. On 2012/01/31 06:53:47, nduca wrote: > I dont see the /// pattern being used in the gpu codebase... its in other bits > of the browser, though. When in doubt, go with the style in your local > directory. Or if you feel strongly, ask @kbr or @apatrick https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager_unittest.cc:9: // Fake Overrides for testing On 2012/01/31 06:53:47, nduca wrote: > not sure this comment adds value Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager_unittest.cc:11: struct FakeCommandBufferStub : GpuMemoryManageableCommandBufferStub { On 2012/01/31 06:53:47, nduca wrote: > why struct? Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager_unittest.cc:27: struct FakeClient : GpuMemoryManagerClient { On 2012/01/31 06:53:47, nduca wrote: > class Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager_unittest.cc:49: FakeCommandBufferStub ret; On 2012/01/31 06:53:47, nduca wrote: > ret -> stub Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager_unittest.cc:56: FakeCommandBufferStub CreateFakeCommandBufferStubWithoutSurface() { On 2012/01/31 06:53:47, nduca wrote: > you know, I'm thinking you might be better served by not having this helper > funciton. > > Instead, just create the surface and set all its fields explicitly every time. > > I find it easier to read test code this way... you'll do a little bit of typing > to get it written out but youll be able to read the test itself very very > clearly at that point. Done. https://chromiumcodereview.appspot.com/9289052/diff/11002/content/common/gpu/... content/common/gpu/gpu_memory_manager_unittest.cc:69: FakeCommandBufferStub fake_stub_with_surface1 = Working on it :) On 2012/01/31 06:53:47, nduca wrote: > i think you're killing yourself with these names. > > fake_client -> client > fake_stub_with_surface1 -> stub1, stub2, etc --- dont bother giving the names to > say whether they're with or without surface. If your setup is easy to read > [described earlier], you wont have to look hard to figure out what the different > stub numbers do. > > Just to help the newcomer in reading a test, I usually put a comment above the > TEST(xxx) macro saying the objective of a test. E.g. > // Verifies that the Send method is called on all stubs. > > You're gonna need more tests than this, I hope you know. > > At minimum: > > 1. At least 3 or 4 tests on the comparator. In each individual test, set up > pairs of stubs and verify that the comparator orders them correcty. E.g. > FakeCommandBufferStub stub1(); > stub1.visible = true > ... > FakeCommndBufferStub stub2(); > stub2.visible = false; > GpuMemoryManager::StubComparator cmp; > EXPECT_TRUE(cmp(stub1,stub2) < 0); > https://chromiumcodereview.appspot.com/9289052/diff/11002/content/content_tes... File content/content_tests.gypi (right): https://chromiumcodereview.appspot.com/9289052/diff/11002/content/content_tes... content/content_tests.gypi:262: 'common/gpu/gpu_memory_manager_unittest.cc', I've been doing gclient runhooks, which I assume just calls gyp_chromium. Is there a difference? On 2012/01/31 06:53:47, nduca wrote: > you might need to run ./build/gyp_chromium to get this to take
LGTM with nits. https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.cc:27: bool visible, Awkward indent. https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.cc:57: surface_state_(), not needed since its an scoped_ptr. https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.cc:58: affected_surface_ids_(), not needed. https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... File content/common/gpu/gpu_command_buffer_stub.h (left): https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.h:146: int32 surface_id_; Are you deleting this? Looks like you've gotten 99% of the way there... https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... File content/common/gpu/gpu_command_buffer_stub.h (right): https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.h:39: class CONTENT_EXPORT GpuCommandBufferStubBase { Put a comment above this class explaining that exposes methods on the stub for testability. https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.h:54: // Null if surface_id == 0. Null if this is an offscreen commandbuffer. surface_id isn't exposed on the base class. https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.h:94: // affected_surface_ids refers to others stubs' surfaces. This stubs' space between methods. Change comment so that it starts with a word? The affected_surfaces_ids_ field... https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.h:114: int32 surface_id() const { return (surface_state_.get()) ? newline after the { maybe move to cpp https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... File content/common/gpu/gpu_memory_allocation.h (right): https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_memory_allocation.h:46: } What's the oeprator overloading being used for? Seems superfluous to me. https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... File content/common/gpu/gpu_memory_manager.cc (right): https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_memory_manager.cc:19: static const int kResourceSizeForegroundTab = 100 * 1024 * 1024; you dont need static keywords inside a anon namespace. https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_memory_manager.cc:19: static const int kResourceSizeForegroundTab = 100 * 1024 * 1024; Put a comment block explaining what these magic values mean. https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_memory_manager.cc:22: static const size_t kDefaultMaxSurfacesWithFrontbufferSoftLimit = 8; It seem so to me we should set kResourceSizeNonHibernated and not have a fore vs back since we're not statically allocating. Moreover, we should set it to 1. 1 byte. E.g. nonzero. And clearly explain its intent is to just be nonzero and that in the future they will go away entirely. https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... File content/common/gpu/gpu_memory_manager.h (right): https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_memory_manager.h:44: void SetMaxSurfacesWithFrontbufferSoftLimit(size_t limit); Construction time parameter? Default value it, perhaps, if goog style guide doesn't forbid it. Non obvious how this gets set, otherwise. https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_memory_manager.h:46: private: redundant https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... File content/common/gpu/gpu_memory_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_memory_manager_unittest.cc:55: first_ = base::TimeTicks::FromInternalValue(1); first_, second_, third_ need better names. https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_memory_manager_unittest.cc:138: // surfaces, and stubs without surfaces but with affected surfaces These two first lines dont add a lot of value.
https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... File content/common/gpu/gpu_memory_allocation.h (right): https://chromiumcodereview.appspot.com/9289052/diff/24001/content/common/gpu/... content/common/gpu/gpu_memory_allocation.h:46: } For now, during testing. I could have defined the operators in the test file, but I think I may need them in a subsequent patch. (cache the current allocation in GpuCommandBufferStub and only send if it changed). On 2012/02/01 00:01:17, nduca wrote: > What's the oeprator overloading being used for? Seems superfluous to me.
> Operator overloading I dont understand why struct equality isn't sufficient?
This was resolved through IM: op== (etc) are not defined by default, and need to be provided even if they are trivial. On 2012/02/01 15:43:10, nduca wrote: > > Operator overloading > I dont understand why struct equality isn't sufficient?
http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_comm... File content/common/gpu/gpu_command_buffer_stub.h (right): http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.h:57: virtual SurfaceState* surface_state() = 0; Can the surface state be modified? maybe return a pointer to a const SurfaceState? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.h:63: const GpuMemoryAllocation& allocation) = 0; Not used in this CL. Nix? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.h:99: virtual const std::vector<int32>& affected_surface_ids() OVERRIDE; Chatted with mmocny offline. It appears that this isn't necessary right now. So he'll move it to a follow-on CL. http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.h:118: int32 surface_id() const; Why? Doesn't this prevent it from being inlined? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_allocation.h (right): http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_allocation.h:53: public: empty? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_manager.cc (right): http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.cc:37: size_t max_surfaces_with_frontbuffer_soft_limit) I'm not sure that I understand the advantage of this. If there's only one GpuMemoryManager per GpuChannelManager and one GpuChannelManager per GPU process, why not a const? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.cc:82: // 2. Background: Non visible surfaces, which have not surpassed the s/Non/No? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_manager.h (right): http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.h:29: static const size_t kDefaultMaxSurfacesWithFrontbufferSoftLimit = 8; Defined in the .cc? That is was we do for Chrome switches. http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.h:35: }; Why define this here? Could it be in an anon namspace in the .cc? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.h:47: void SetMaxSurfacesWithFrontbufferSoftLimit(size_t limit); Used anywhere? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_manager_unittest.cc (right): http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager_unittest.cc:91: EXPECT_FALSE(is_more_important(&stub_true1,&stub_true1)); need space after comma
http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_comm... File content/common/gpu/gpu_command_buffer_stub.h (right): http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.h:57: virtual SurfaceState* surface_state() = 0; It is modified: last_used_time and visibility are changed. I am not sure if it would be better to make getters and setters.. On 2012/02/01 19:43:17, jonathan.backer wrote: > Can the surface state be modified? maybe return a pointer to a const > SurfaceState? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.h:63: const GpuMemoryAllocation& allocation) = 0; It is used by the Manage function. Even though it is a no-op in the Stub at the moment, it is not a no-op in the tests, and it is the only way to test the Manage function. On 2012/02/01 19:43:17, jonathan.backer wrote: > Not used in this CL. Nix? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.h:118: int32 surface_id() const; It may prevent it from being inlined (linkers can do function inlining, and you can add the inline keyword to a declaration to hint to the compiler). Also, defining a function inside a class in a header strongly suggests inlining, but it isn't guaranteed. C++ details aside, I think the convention is to have only trivial getters/setters defined in the header, and the current getter is slightly less than trivial, imho. On 2012/02/01 19:43:17, jonathan.backer wrote: > Why? Doesn't this prevent it from being inlined? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_allocation.h (right): http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_allocation.h:53: public: This is not used, and can be removed until a subsequent patch. On 2012/02/01 19:43:17, jonathan.backer wrote: > empty? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_manager.cc (right): http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.cc:37: size_t max_surfaces_with_frontbuffer_soft_limit) Specifically: for testing, so that you can control the exact value. Additionally: when I was working on the previous tabmru list, people expressed wanting a runtime adjustable limit. Not that this is currently runtime adjustable, but its an argument against const. On 2012/02/01 19:43:17, jonathan.backer wrote: > I'm not sure that I understand the advantage of this. If there's only one > GpuMemoryManager per GpuChannelManager and one GpuChannelManager per GPU > process, why not a const? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.cc:82: // 2. Background: Non visible surfaces, which have not surpassed the I don't think so.. s/Non/Not or s/Non /In if anything, but I think this is fine? On 2012/02/01 19:43:17, jonathan.backer wrote: > s/Non/No? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_manager.h (right): http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.h:29: static const size_t kDefaultMaxSurfacesWithFrontbufferSoftLimit = 8; On 2012/02/01 19:43:17, jonathan.backer wrote: > Defined in the .cc? That is was we do for Chrome switches. Done. http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.h:35: }; For testability. I'll move this down to the privates section, since the test class is now a friend. On 2012/02/01 19:43:17, jonathan.backer wrote: > Why define this here? Could it be in an anon namspace in the .cc? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.h:47: void SetMaxSurfacesWithFrontbufferSoftLimit(size_t limit); Not any more, since we now have a constructor argument. On 2012/02/01 19:43:17, jonathan.backer wrote: > Used anywhere? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_manager_unittest.cc (right): http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager_unittest.cc:91: EXPECT_FALSE(is_more_important(&stub_true1,&stub_true1)); On 2012/02/01 19:43:17, jonathan.backer wrote: > need space after comma Done.
http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_comm... File content/common/gpu/gpu_command_buffer_stub.h (right): http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.h:57: virtual SurfaceState* surface_state() = 0; On 2012/02/01 20:44:19, mmocny wrote: > It is modified: last_used_time and visibility are changed. Inside the Stub, we can use surface_state_ to modify. Is it modified outside the Stub? http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.h:118: int32 surface_id() const; On 2012/02/01 20:44:19, mmocny wrote: > It may prevent it from being inlined (linkers can do function inlining, and you > can add the inline keyword to a declaration to hint to the compiler). > Also, defining a function inside a class in a header strongly suggests inlining, > but it isn't guaranteed. > C++ details aside, I think the convention is to have only trivial > getters/setters defined in the header, and the current getter is slightly less > than trivial, imho. It's very trivial, IMHO. http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_manager.cc (right): http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_manager.cc:82: // 2. Background: Non visible surfaces, which have not surpassed the I think is see. You mean non-visible. On 2012/02/01 20:44:19, mmocny wrote: > I don't think so.. > s/Non/Not or s/Non /In if anything, but I think this is fine? > On 2012/02/01 19:43:17, jonathan.backer wrote: > > s/Non/No? >
http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_comm... File content/common/gpu/gpu_command_buffer_stub.h (right): http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.h:57: virtual SurfaceState* surface_state() = 0; On 2012/02/01 21:39:14, jonathan.backer wrote: > On 2012/02/01 20:44:19, mmocny wrote: > > It is modified: last_used_time and visibility are changed. > > Inside the Stub, we can use surface_state_ to modify. Is it modified outside the > Stub? Done. http://codereview.chromium.org/9289052/diff/18038/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.h:118: int32 surface_id() const; On 2012/02/01 21:39:14, jonathan.backer wrote: > On 2012/02/01 20:44:19, mmocny wrote: > > It may prevent it from being inlined (linkers can do function inlining, and > you > > can add the inline keyword to a declaration to hint to the compiler). > > Also, defining a function inside a class in a header strongly suggests > inlining, > > but it isn't guaranteed. > > C++ details aside, I think the convention is to have only trivial > > getters/setters defined in the header, and the current getter is slightly less > > than trivial, imho. > > It's very trivial, IMHO. Done.
LGTM http://codereview.chromium.org/9289052/diff/23005/content/common/gpu/gpu_comm... File content/common/gpu/gpu_command_buffer_stub.cc (right): http://codereview.chromium.org/9289052/diff/23005/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.cc:535: return !!surface_state_.get(); Correct me if I'm wrong, I think that !! is redundant. AFAIK, ! is usually to coerce a smart pointer into a bool. But we don't have that here: get() is returning a raw pointer. Also, why not in the header? http://codereview.chromium.org/9289052/diff/23005/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.cc:541: return *surface_state_.get(); Header?
http://codereview.chromium.org/9289052/diff/23005/content/common/gpu/gpu_comm... File content/common/gpu/gpu_command_buffer_stub.cc (right): http://codereview.chromium.org/9289052/diff/23005/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.cc:535: return !!surface_state_.get(); Its not redundant, but its needlessly clever. I'll just change it to surface_state_.get() != NULL; which is far more clear. (Long answer: its not redundant because some compilers will warn on cast from int types to bool, since the only technically valid values for bool are true/false. Consider the case of non-0 int value but where the low order 8 bits are 0..) On 2012/02/02 14:03:53, jonathan.backer wrote: > Correct me if I'm wrong, I think that !! is redundant. AFAIK, ! is usually to > coerce a smart pointer into a bool. But we don't have that here: get() is > returning a raw pointer. > > Also, why not in the header? http://codereview.chromium.org/9289052/diff/23005/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.cc:535: return !!surface_state_.get(); On 2012/02/02 14:03:53, jonathan.backer wrote: > Correct me if I'm wrong, I think that !! is redundant. AFAIK, ! is usually to > coerce a smart pointer into a bool. But we don't have that here: get() is > returning a raw pointer. > > Also, why not in the header? Done. http://codereview.chromium.org/9289052/diff/23005/content/common/gpu/gpu_comm... content/common/gpu/gpu_command_buffer_stub.cc:541: return *surface_state_.get(); On 2012/02/02 14:03:53, jonathan.backer wrote: > Header? Done.
https://chromiumcodereview.appspot.com/9289052/diff/23005/content/common/gpu/... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://chromiumcodereview.appspot.com/9289052/diff/23005/content/common/gpu/... content/common/gpu/gpu_command_buffer_stub.cc:535: return !!surface_state_.get(); Update: These functions are virtual and should not be inlined. On 2012/02/02 14:58:10, mmocny wrote: > On 2012/02/02 14:03:53, jonathan.backer wrote: > > Correct me if I'm wrong, I think that !! is redundant. AFAIK, ! is usually to > > coerce a smart pointer into a bool. But we don't have that here: get() is > > returning a raw pointer. > > > > Also, why not in the header? > > Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mmocny@chromium.org/9289052/23031
Try job failure for 9289052-23031 (retry) on win_rel for step "test_shell_tests". It's a second try, previously, step "test_shell_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mmocny@chromium.org/9289052/26027
Change committed as 120339
content/content_common.gypi references 'common/gpu/gpu_memory_management.h' which doesn't seem to exist. This causes unnecessary rebuilds on Windows. Could you remove that (or add that file if it's supposed to be there?) |
