Make sure we call glBufferData on the same data we store internally.

gpu/command_buffer/service/buffer_manager.cc

Index: gpu/command_buffer/service/buffer_manager.cc
diff --git a/gpu/command_buffer/service/buffer_manager.cc b/gpu/command_buffer/service/buffer_manager.cc
index 261b2edd10b18f5935d0b9325b6493d487ae7f4a..091bdf9eb19a7d162ad320cf19a9400dda57c8c7 100644
--- a/gpu/command_buffer/service/buffer_manager.cc
+++ b/gpu/command_buffer/service/buffer_manager.cc
@@ -119,7 +119,6 @@ Buffer::Buffer(BufferManager* manager, GLuint service_id)
     : manager_(manager),
       size_(0),
       deleted_(false),
-      shadowed_(false),
       is_client_side_array_(false),
       service_id_(service_id),
       initial_target_(0),
@@ -138,28 +137,32 @@ Buffer::~Buffer() {
   }
 }
 
-void Buffer::SetInfo(
-    GLsizeiptr size, GLenum usage, bool shadow, const GLvoid* data,
-    bool is_client_side_array) {
-  usage_ = usage;
-  is_client_side_array_ = is_client_side_array;
-  ClearCache();
-  if (size != size_ || shadow != shadowed_) {
-    shadowed_ = shadow;
-    size_ = size;
-    if (shadowed_) {
-      shadow_.reset(new int8_t[size]);
-    } else {
-      shadow_.reset();
-    }
-  }
-  if (shadowed_) {
+const GLvoid* Buffer::StageShadow(bool use_shadow, GLsizeiptr size,
+                                  const GLvoid* data) {
+  if (use_shadow) {
+    shadow_.resize(size);

[piman, 2016/04/01 00:09:46]

Why the change to std::vector? We never need to append data to the shadow
buffer... Anyway, using a vector means that we make 2 copies here (vector.resize
copies the existing data, that we then overwrite by copying the new data or
memset).
Maybe clear() before resize to avoid the extra copy, unless the sizes match? (at
which point it's not much different than the previous logic).

[David Yen, 2016/04/01 17:00:53]

The reason I changed it to a vector was to reduce the number of memory
allocations. Essentially what I want is to have a capacity variable and size
variable. Then only reallocate memory if the size needed exceeds the capacity.
At that point it's looking more and more like std::vector. I also didn't like
how shadowed_ and shadow_ needed to be kept in lock step with one another,
although that could have been done by deleting shadow_ and checking if the
pointer is valid as well.

It's true that vector.resize() copies the existing data, but clearing and then
resizing will set each element to 0 which is effectively the same cost on new
allocations. For cases where no allocation happens this would have an extra
cost.

I agree neither case is ideal... perhaps the right thing to do is to clear and
then insert data or resize on NULL data?

     if (data) {
-      memcpy(shadow_.get(), data, size);
+      memcpy(shadow_.data(), data, size);
     } else {
-      memset(shadow_.get(), 0, size);
+      memset(shadow_.data(), 0, size);
     }
+    return shadow_.data();
+  } else {
+    shadow_.clear();
+    return data;
   }
+}
+
+void Buffer::SetInfo(
+    GLsizeiptr size, GLenum usage, bool shadow, bool is_client_side_array) {

[Ken Russell (switch to Gerrit), 2016/03/31 23:28:04]

It might be more clear now to rename "shadow" to "use_shadow".

[David Yen, 2016/04/01 17:00:53]

Done.

+  usage_ = usage;
+  is_client_side_array_ = is_client_side_array;
+  ClearCache();
+
+  // Shadow must have been setup already.
+  DCHECK_EQ(shadow_.size(), static_cast<size_t>(shadow ? size : 0u));
+  size_ = size;
+
   mapped_range_.reset(nullptr);
 }
 
@@ -177,8 +180,9 @@ bool Buffer::SetRange(
   if (!CheckRange(offset, size)) {
     return false;
   }
-  if (shadowed_) {
-    memcpy(shadow_.get() + offset, data, size);
+  if (!shadow_.empty()) {
+    DCHECK_LE(static_cast<size_t>(offset + size), shadow_.size());
+    memcpy(shadow_.data() + offset, data, size);
     ClearCache();
   }
   return true;
@@ -186,13 +190,14 @@ bool Buffer::SetRange(
 
 const void* Buffer::GetRange(
     GLintptr offset, GLsizeiptr size) const {
-  if (!shadowed_) {
+  if (shadow_.empty()) {
     return NULL;
   }
   if (!CheckRange(offset, size)) {
     return NULL;
   }
-  return shadow_.get() + offset;
+  DCHECK_LE(static_cast<size_t>(offset + size), shadow_.size());
+  return shadow_.data() + offset;
 }
 
 void Buffer::ClearCache() {
@@ -280,7 +285,7 @@ bool Buffer::GetMaxValueForRange(
     return false;
   }
 
-  if (!shadowed_) {
+  if (shadow_.empty()) {
     return false;
   }
 
@@ -288,7 +293,7 @@ bool Buffer::GetMaxValueForRange(
   GLuint max_v = 0;
   switch (type) {
     case GL_UNSIGNED_BYTE:
-      max_v = GetMaxValue<uint8_t>(shadow_.get(), offset, count,
+      max_v = GetMaxValue<uint8_t>(shadow_.data(), offset, count,
           primitive_restart_index);
       break;
     case GL_UNSIGNED_SHORT:
@@ -296,7 +301,7 @@ bool Buffer::GetMaxValueForRange(
       if ((offset & 1) != 0) {
         return false;
       }
-      max_v = GetMaxValue<uint16_t>(shadow_.get(), offset, count,
+      max_v = GetMaxValue<uint16_t>(shadow_.data(), offset, count,
           primitive_restart_index);
       break;
     case GL_UNSIGNED_INT:
@@ -304,7 +309,7 @@ bool Buffer::GetMaxValueForRange(
       if ((offset & 3) != 0) {
         return false;
       }
-      max_v = GetMaxValue<uint32_t>(shadow_.get(), offset, count,
+      max_v = GetMaxValue<uint32_t>(shadow_.data(), offset, count,
           primitive_restart_index);
       break;
     default:
@@ -338,19 +343,23 @@ bool BufferManager::UseNonZeroSizeForClientSideArrayBuffer() {
              .use_non_zero_size_for_client_side_stream_buffers;
 }
 
-void BufferManager::SetInfo(Buffer* buffer, GLenum target, GLsizeiptr size,
-                            GLenum usage, const GLvoid* data) {
-  DCHECK(buffer);
-  memory_type_tracker_->TrackMemFree(buffer->size());
+bool BufferManager::UseShadowBuffer(GLenum target, GLenum usage) {
   const bool is_client_side_array = IsUsageClientSideArray(usage);
   const bool support_fixed_attribs =
     gfx::GetGLImplementation() == gfx::kGLImplementationEGLGLES2;
+
   // TODO(zmo): Don't shadow buffer data on ES3. crbug.com/491002.
-  const bool shadow = target == GL_ELEMENT_ARRAY_BUFFER ||
-                      allow_buffers_on_multiple_targets_ ||
-                      (allow_fixed_attribs_ && !support_fixed_attribs) ||
-                      is_client_side_array;
-  buffer->SetInfo(size, usage, shadow, data, is_client_side_array);
+  return (target == GL_ELEMENT_ARRAY_BUFFER ||
+          allow_buffers_on_multiple_targets_ ||
+          (allow_fixed_attribs_ && !support_fixed_attribs) ||
+          is_client_side_array);
+}
+
+void BufferManager::SetInfo(Buffer* buffer, GLenum target, GLsizeiptr size,
+                            GLenum usage, bool use_shadow) {
+  DCHECK(buffer);
+  memory_type_tracker_->TrackMemFree(buffer->size());
+  buffer->SetInfo(size, usage, use_shadow, IsUsageClientSideArray(usage));
   memory_type_tracker_->TrackMemAlloc(buffer->size());
 }
 
@@ -398,13 +407,11 @@ void BufferManager::DoBufferData(
     GLsizeiptr size,
     GLenum usage,
     const GLvoid* data) {
-  // Clear the buffer to 0 if no initial data was passed in.
-  scoped_ptr<int8_t[]> zero;
-  if (!data) {
-    zero.reset(new int8_t[size]);
-    memset(zero.get(), 0, size);
-    data = zero.get();
-  }
+
+    // Stage the shadow buffer first if we are using a shadow buffer so that we

[Ken Russell (switch to Gerrit), 2016/03/31 23:28:04]

Is the indentation off here?

[David Yen, 2016/04/01 17:00:53]

Done. I just ran cl format.

+    // validate what we store internally.
+    const bool use_shadow = UseShadowBuffer(target, usage);
+    data = buffer->StageShadow(use_shadow, size, data);
 
   ERRORSTATE_COPY_REAL_GL_ERRORS_TO_WRAPPER(error_state, "glBufferData");
   if (IsUsageClientSideArray(usage)) {
@@ -414,11 +421,12 @@ void BufferManager::DoBufferData(
     glBufferData(target, size, data, usage);
   }
   GLenum error = ERRORSTATE_PEEK_GL_ERROR(error_state, "glBufferData");
-  if (error == GL_NO_ERROR) {
-    SetInfo(buffer, target, size, usage, data);
-  } else {
-    SetInfo(buffer, target, 0, usage, NULL);
+  if (error != GL_NO_ERROR) {
+    size = 0;
+    buffer->StageShadow(false, 0, nullptr); // Also clear the shadow.
   }
+
+  SetInfo(buffer, target, size, usage, use_shadow);
 }
 
 void BufferManager::ValidateAndDoBufferSubData(