During virtual context switches only restore texture units that have changed from the previous cont…

gpu/command_buffer/service/context_state.cc

Index: gpu/command_buffer/service/context_state.cc
diff --git a/gpu/command_buffer/service/context_state.cc b/gpu/command_buffer/service/context_state.cc
index e8912cfe1625b769dc0047b935c2f098aed31da3..68f3fd31a26dbecb740bdf43953a1a2f6b7086fb 100644
--- a/gpu/command_buffer/service/context_state.cc
+++ b/gpu/command_buffer/service/context_state.cc
@@ -40,6 +40,7 @@ ContextState::ContextState(FeatureInfo* feature_info, Logger* logger)
       pack_reverse_row_order(false),
       fbo_binding_for_scissor_workaround_dirty_(false),
       feature_info_(feature_info),
+      max_active_texture_unit_(0),
       error_state_(ErrorState::Create(logger)) {
   Initialize();
 }
@@ -101,9 +102,17 @@ void ContextState::RestoreActiveTexture() const {
   glActiveTexture(GL_TEXTURE0 + active_texture_unit);
 }
 
-void ContextState::RestoreAllTextureUnitBindings() const {
+void ContextState::RestoreAllTextureUnitBindings(
+    const ContextState* prev_state) const {
+  size_t units_to_restore = texture_units.size();

[boliu, 2013/12/18 19:55:11]

Should this be max_active_texture_unit_?

+  if (prev_state) {
+    // Only restore units that have been modified by both contexts.
+    units_to_restore = std::max(prev_state->max_active_texture_unit_,
+                                max_active_texture_unit_) + 1;

[no sievers, 2013/12/18 19:36:20]

Do we really need |max_active_texture_unit_| which seems to track which units
*might* be set to something else than the default textures? But it doesn't know
when we set textures back to the default texture, and it also doesn't know about
which target was changed. But luckily we have all this information... :)

So.... can't you just compare the old and new service id in
RestoreTextureUnitBindings()? That would actually eliminate even more GL calls
(in fact, all redundant ones).

[kaanb, 2013/12/18 19:43:56]

Having max_active_texture_unit_ is useful as it reduces the number of iterations
of the loop. I was planning to land this patch first and then go into target
specific optimizations in RestoreTextureUnitBindings() after the holidays.

Even with this approach, we go down from 32 * 3 GL calls per switch to 1 * 3 GL
calls so it is still a huge win IMHO.

[boliu, 2013/12/18 19:55:11]

Just diffing against prev_state would be great for android webview. If we could
plug in the app's state as prev_state, then we could save a lot of GL calls when
switching between app and chromium.

[Ken Russell (switch to Gerrit), 2013/12/18 20:09:44]

Reducing the number of loop iterations is a premature optimization that is known
to not be fully correct. Please remove the newly added max_active_texture_unit_
state and just iterate over all texture units. If it's later determined that the
optimization would help, it can be added.

+  }
+
   // Restore Texture state.
-  for (size_t ii = 0; ii < texture_units.size(); ++ii) {
+  for (size_t ii = 0; ii < units_to_restore; ++ii) {
     RestoreTextureUnitBindings(ii);
   }
   RestoreActiveTexture();
@@ -139,8 +148,8 @@ void ContextState::RestoreGlobalState() const {
   InitState();
 }
 
-void ContextState::RestoreState() const {
-  RestoreAllTextureUnitBindings();
+void ContextState::RestoreState(const ContextState* prev_state) const {
+  RestoreAllTextureUnitBindings(prev_state);
 
   // Restore Attrib State
   // TODO: This if should not be needed. RestoreState is getting called