Adding GpuMemoryManager to track GpuCommandBufferStub visibility and last_used_time and dictate mem…

content/common/gpu/gpu_memory_manager.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/common/gpu/gpu_memory_manager.h"
+
+#if defined(ENABLE_GPU)
+
+#include "content/common/gpu/gpu_channel_manager.h"
+#include "content/common/gpu/gpu_channel.h"
+#include "content/common/gpu/gpu_command_buffer_stub.h"
+
+#include <vector>
+
+////////////////////////////////////////////////////////////////////////////////
+// Local helpers
+
+namespace {
+
+/*
+ * A RenderWidgetDescriptor is used to combine the information various stubs
+ * receive, in order to best infer the RenderWidget's current state.
+ */
+class RenderWidgetDescriptor {
+public:
+  RenderWidgetDescriptor(int render_widget_id,

[nduca, 2012/01/27 10:10:13]

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.

+                         bool visible,
+                         int64 last_used_time,
+                         GpuCommandBufferStub* stub)
+      : render_widget_id_(render_widget_id)
+      , visible_(visible)
+      , last_used_time_(last_used_time)
+      , stubs_(1,stub) {
+  }
+
+public:
+  int render_widget_id() const { return render_widget_id_; }
+  bool visible() const { return visible_; }
+  int64 last_used_time() const { return last_used_time_; }
+  std::vector<GpuCommandBufferStub*> const& stubs() const { return stubs_; }
+
+  // This is a helper function to learn from another descriptor for the same

[nduca, 2012/01/27 10:10:13]

??? when do you merge? This feels like some serious voodoo that makes me think
something is wrong further up.

[mmocny, 2012/01/27 19:51:33]

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:

+  // RenderWidget, thus creating a single more accurate one.
+  void merge(RenderWidgetDescriptor const& other) {
+    DCHECK(render_widget_id_ == other.render_widget_id_);
+    // Check if this RWD has staler data than other:
+    if ((last_used_time_ == GpuCommandBufferStub::kUnknownLastUsedTime) ||
+        (other.last_used_time_ != GpuCommandBufferStub::kUnknownLastUsedTime &&
+            last_used_time_ < other.last_used_time_)) {
+      visible_ = other.visible_;
+      last_used_time_ = other.last_used_time_;
+    }
+    // Either way, merge stubs
+    stubs_.insert(stubs_.end(), other.stubs_.begin(), other.stubs_.end());
+    // TODO(mmocny): DCHECK(confirm-no-duplicates)
+    // This is currently certain to be true, but should state assumptions
+    // since this assumption isn't enforced
+  }
+
+private:
+  int render_widget_id_;
+
+  bool visible_;
+  int64 last_used_time_;
+
+  std::vector<GpuCommandBufferStub*> stubs_;
+};
+
+/*
+ * Used to sort RenderWidgetDescriptors into most-to-least "important" order
+ */
+struct RenderWidgetDescriptorSorter {
+  bool operator()(RenderWidgetDescriptor* lhs, RenderWidgetDescriptor* rhs) {
+    if (lhs->visible() != rhs->visible()) // Visible RWD first
+      return lhs->visible();
+    else if (lhs->visible()) // Use id as tiebreaker when both are visible
+      return lhs->render_widget_id() < rhs->render_widget_id();
+
+    DCHECK(lhs->last_used_time() != GpuCommandBufferStub::kUnknownLastUsedTime);
+    DCHECK(rhs->last_used_time() != GpuCommandBufferStub::kUnknownLastUsedTime);
+    // Last-used-time order for non visible ones
+    return lhs->last_used_time() > rhs->last_used_time();
+  }
+};

[nduca, 2012/01/27 10:10:13]

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.

[mmocny, 2012/01/27 19:51:33]

This should be easy to test.

On 2012/01/27 10:10:13, nduca wrote:

+
+/*
+ * GetGpuCommandBufferStubs
+ */
+std::vector<GpuCommandBufferStub*> GetGpuCommandBufferStubs(
+    GpuChannelManager* channel_manager) {
+  std::vector<GpuCommandBufferStub*> ret;
+
+  std::vector<GpuChannel*> channels = channel_manager->GetChannels();
+
+  for (std::vector<GpuChannel*>::const_iterator channel_it = channels.begin();
+      channel_it != channels.end(); ++channel_it ) {
+    GpuChannel* channel = *channel_it;
+    std::vector<GpuCommandBufferStub*> stubs = channel->GetCommandBuffers();
+    ret.insert(ret.end(), stubs.begin(), stubs.end());
+  }
+  return ret;
+}
+
+/*
+ * ComputeRenderWidgetDescriptorsFromStubs
+ */
+std::vector<RenderWidgetDescriptor*> ComputeRenderWidgetDescriptorsFromStubs(

[nduca, 2012/01/27 10:10:13]

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...

[mmocny, 2012/01/27 19:51:33]

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:

+    std::vector<GpuCommandBufferStub*> stubs) {
+  std::vector<RenderWidgetDescriptor*> ret;
+
+  for (std::vector<GpuCommandBufferStub*>::iterator gcbs_it = stubs.begin();
+      gcbs_it != stubs.end(); ++gcbs_it) {
+    GpuCommandBufferStub* stub = *gcbs_it;
+    std::vector<int> render_widget_ids = stub->render_widget_ids();
+
+    for (std::vector<int>::iterator rwids_it = render_widget_ids.begin();
+        rwids_it != render_widget_ids.end(); ++rwids_it) {
+      RenderWidgetDescriptor* rwd = new RenderWidgetDescriptor(*rwids_it,
+          stub->visible(), stub->last_used_time(), stub);
+
+      // Try to find existing Render Widget
+      for (std::vector<RenderWidgetDescriptor*>::iterator rwds_it = ret.begin();
+          rwds_it != ret.end(); ++rwds_it) {
+        if ((*rwds_it)->render_widget_id() == *rwids_it) {
+          (*rwds_it)->merge(*rwd);
+          rwd = NULL;
+          break;
+        }
+      }
+      if (rwd)
+        ret.push_back(rwd);
+    }
+  }
+
+  return ret;
+}
+
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// Constructors/Destructors
+
+GpuMemoryManager::GpuMemoryManager(GpuChannelManager* channel_manager)
+    : channel_manager_(channel_manager) {
+
+}
+
+GpuMemoryManager::~GpuMemoryManager() {
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
+void GpuMemoryManager::Manage() const {
+  // Compute RenderWidgetDescriptors from GpuCommandBufferStubs
+  std::vector<GpuCommandBufferStub*> stubs = GetGpuCommandBufferStubs(
+      channel_manager_);
+  std::vector<RenderWidgetDescriptor*> render_widget_descriptors =
+      ComputeRenderWidgetDescriptorsFromStubs(stubs);
+
+  // Sort them in {visibility,last_used_time} order using custom sorter
+  std::sort(render_widget_descriptors.begin(), render_widget_descriptors.end(),
+      RenderWidgetDescriptorSorter());
+
+  // TODO(mmocny): What follows is vastly simplified logic based on counts,
+  // should consider actual memory usage and availability.
+
+  // Separate into three sets, identified by render_widget_id
+  //  1. all_buffers:   Every visible RenderWidget must have all buffers.
+  //  2. front_buffers: Invisible RenderWidgets can have a frontbuffer if the
+  //                    the total count is under some soft limit.
+  //  3. no_buffers:    The rest should drop all buffers.
+  // TODO(mmocny): all_buffers takes up ~3 times more memory than front_buffers.
+  //               Couldn't we have 3 front_buffer per each all_buffer?
+  std::set<int> all_buffers, front_buffers, no_buffers;
+  static const size_t kMaxFrontBufferSoftLimit = 8;
+
+  for (std::vector<RenderWidgetDescriptor*>::iterator rwds_it =
+      render_widget_descriptors.begin();
+      rwds_it != render_widget_descriptors.end(); ++rwds_it) {
+    RenderWidgetDescriptor* rwd = *rwds_it;
+    if (rwd->visible())
+      all_buffers.insert(rwd->render_widget_id());
+    else if ((all_buffers.size() + front_buffers.size()) <
+        kMaxFrontBufferSoftLimit)
+      front_buffers.insert(rwd->render_widget_id());
+    else
+      no_buffers.insert(rwd->render_widget_id());
+  }
+
+  // Now, go through the command buffer stubs, and match their render widgets
+  // up to the buckets we divided.  Since they may be associated with
+  // RenderWidgets in various buckets, the most visible one takes priority
+  for (std::vector<GpuCommandBufferStub*>::const_iterator it = stubs.begin();
+      it != stubs.end(); ++it) {
+    GpuCommandBufferStub* stub = *it;
+    GpuMemoryAllocation allocation;
+    std::vector<int> render_widget_ids = stub->render_widget_ids();
+    if (std::find_first_of(all_buffers.begin(), all_buffers.end(),
+        render_widget_ids.begin(), render_widget_ids.end()) !=
+            all_buffers.end()) {
+      allocation.gpuResourceSizeInBytes =
+          GpuMemoryAllocation::kResourceSizeForegroundTab;
+      allocation.hasFrontbuffer = true;
+      allocation.hasBackbuffer = true;
+    } else if (std::find_first_of(front_buffers.begin(), front_buffers.end(),
+        render_widget_ids.begin(), render_widget_ids.end()) !=
+            front_buffers.end()) {
+      allocation.gpuResourceSizeInBytes =
+          GpuMemoryAllocation::kResourceSizeBackgroundTab;
+      allocation.hasFrontbuffer = true;
+      allocation.hasBackbuffer = false;
+    } else {
+      allocation.gpuResourceSizeInBytes =
+          GpuMemoryAllocation::kResourceSizeHibernatedTab;
+      allocation.hasFrontbuffer = false;
+      allocation.hasBackbuffer = false;
+    }
+    stub->SetMemoryAllocation(allocation);
+  }
+}
+

[nduca, 2012/01/27 10:10:13]

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.

[Ken Russell (switch to Gerrit), 2012/01/27 19:21:24]

+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.

[mmocny, 2012/01/27 19:51:33]

ok.
On 2012/01/27 10:10:13, nduca wrote:

+////////////////////////////////////////////////////////////////////////////////
+
+#endif