From 5ffd5baeef9d5537e8b7e2e1d256ac669d46559b Mon Sep 17 00:00:00 2001 From: "bsalomon@google.com" Date: Thu, 1 Mar 2012 15:29:07 +0000 Subject: [PATCH] Workaround for chrome on mac buffer object performance issue. Review URL: http://codereview.appspot.com/5713051/ git-svn-id: http://skia.googlecode.com/svn/trunk@3289 2bbb7eff-a529-9590-31e7-b0007b416f81 --- include/gpu/gl/GrGLConfig.h | 33 +++++++++++++++++++++++++++ src/gpu/gl/GrGLIndexBuffer.cpp | 50 ++++++++++++++++++++++++----------------- src/gpu/gl/GrGLVertexBuffer.cpp | 44 +++++++++++++++++++++--------------- 3 files changed, 89 insertions(+), 38 deletions(-) diff --git a/include/gpu/gl/GrGLConfig.h b/include/gpu/gl/GrGLConfig.h index 5ae931c..2cc4f31 100644 --- a/include/gpu/gl/GrGLConfig.h +++ b/include/gpu/gl/GrGLConfig.h @@ -149,6 +149,39 @@ #define GR_GL_CHECK_FBO_STATUS_ONCE_PER_FORMAT 0 #endif +/** + * There is a strange bug that occurs on Macs with NVIDIA GPUs. We don't + * fully understand it. When (element) array buffers are continually + * respecified using glBufferData at the same (or perhaps just a small) size + * performance can fall off of a cliff. The driver winds up performing many + * DMA mapping / unmappings and chews up ~50% of the core. However, it has been + * observed that respecifiying the buffer with a larger size and then writing + * the small amount of actual data using glBufferSubData prevents the bad + * behavior. + * + * There is a lot of uncertainty around this issue. In Chrome backgrounding + * the tab somehow initiates this behavior and we don't know what the connection + * is. Another observation is that Chrome's cmd buffer server will actually + * create a buffer full of zeros when it sees a NULL data param (for security + * reasons). If this is disabled and NULL is actually passed all the way to the + * driver then the workaround doesn't help. + * + * The issue is tracked at: + * http://code.google.com/p/chromium/issues/detail?id=114865 + * + * When GR_GL_USE_BUFFER_DATA_NULL_HINT is not set we encounter the bug + * if we keep uploading small vetex / index sizes. So, every 1K uploads we + * act as though GR_GL_USE_BUFFER_DATA_NULL_HINT was set: we specify the buffer + * at full size with a NULL data ptr and then do a partial update with + * glBufferSubData. + * + * Hopefully we will understand this better and have a cleaner fix or get a + * OS/driver level fix. + */ +#define GR_GL_MAC_BUFFER_OBJECT_PERFOMANCE_WORKAROUND \ + (GR_MAC_BUILD && \ + !GR_GL_USE_BUFFER_DATA_NULL_HINT) + #if(GR_GL_NO_CONSTANT_ATTRIBUTES) && (GR_GL_ATTRIBUTE_MATRICES) #error "Cannot combine GR_GL_NO_CONSTANT_ATTRIBUTES and GR_GL_ATTRIBUTE_MATRICES" #endif diff --git a/src/gpu/gl/GrGLIndexBuffer.cpp b/src/gpu/gl/GrGLIndexBuffer.cpp index b64668e..f424e78 100644 --- a/src/gpu/gl/GrGLIndexBuffer.cpp +++ b/src/gpu/gl/GrGLIndexBuffer.cpp @@ -104,28 +104,38 @@ bool GrGLIndexBuffer::updateData(const void* src, size_t srcSizeInBytes) { } this->bind(); GrGLenum usage = dynamic() ? GR_GL_DYNAMIC_DRAW : GR_GL_STATIC_DRAW; -#if !GR_GL_USE_BUFFER_DATA_NULL_HINT - // Note that we're cheating on the size here. Currently no methods - // allow a partial update that preserves contents of non-updated - // portions of the buffer (and lock() does a glBufferData(..size, NULL..)) - GL_CALL(BufferData(GR_GL_ELEMENT_ARRAY_BUFFER, srcSizeInBytes, src, usage)); -#else - if (this->sizeInBytes() == srcSizeInBytes) { - GL_CALL(BufferData(GR_GL_ELEMENT_ARRAY_BUFFER, - srcSizeInBytes, src, usage)); + + bool doNullHint = GR_GL_USE_BUFFER_DATA_NULL_HINT; +#if GR_GL_MAC_BUFFER_OBJECT_PERFOMANCE_WORKAROUND + GrAssert(!doNullHint); + static int N = 0; + doNullHint = (0 == N % 1024); + ++N; +#endif + + if (doNullHint) { + if (this->sizeInBytes() == srcSizeInBytes) { + GL_CALL(BufferData(GR_GL_ELEMENT_ARRAY_BUFFER, + srcSizeInBytes, src, usage)); + } else { + // Before we call glBufferSubData we give the driver a hint using + // glBufferData with NULL. This makes the old buffer contents + // inaccessible to future draws. The GPU may still be processing + // draws that reference the old contents. With this hint it can + // assign a different allocation for the new contents to avoid + // flushing the gpu past draws consuming the old contents. + GL_CALL(BufferData(GR_GL_ELEMENT_ARRAY_BUFFER, + this->sizeInBytes(), NULL, usage)); + GL_CALL(BufferSubData(GR_GL_ELEMENT_ARRAY_BUFFER, + 0, srcSizeInBytes, src)); + } } else { - // Before we call glBufferSubData we give the driver a hint using - // glBufferData with NULL. This makes the old buffer contents - // inaccessible to future draws. The GPU may still be processing draws - // that reference the old contents. With this hint it can assign a - // different allocation for the new contents to avoid flushing the gpu - // past draws consuming the old contents. - GL_CALL(BufferData(GR_GL_ELEMENT_ARRAY_BUFFER, - this->sizeInBytes(), NULL, usage)); - GL_CALL(BufferSubData(GR_GL_ELEMENT_ARRAY_BUFFER, - 0, srcSizeInBytes, src)); + // Note that we're cheating on the size here. Currently no methods + // allow a partial update that preserves contents of non-updated + // portions of the buffer (lock() does a glBufferData(..size, NULL..)) + GL_CALL(BufferData(GR_GL_ELEMENT_ARRAY_BUFFER, + srcSizeInBytes, src, usage)); } -#endif return true; } diff --git a/src/gpu/gl/GrGLVertexBuffer.cpp b/src/gpu/gl/GrGLVertexBuffer.cpp index 33c1e7e..4928868 100644 --- a/src/gpu/gl/GrGLVertexBuffer.cpp +++ b/src/gpu/gl/GrGLVertexBuffer.cpp @@ -101,26 +101,34 @@ bool GrGLVertexBuffer::updateData(const void* src, size_t srcSizeInBytes) { } this->bind(); GrGLenum usage = dynamic() ? GR_GL_DYNAMIC_DRAW : GR_GL_STATIC_DRAW; -#if !GR_GL_USE_BUFFER_DATA_NULL_HINT - // Note that we're cheating on the size here. Currently no methods - // allow a partial update that preserves contents of non-updated - // portions of the buffer (and lock() does a glBufferData(..size, NULL..)) - GL_CALL(BufferData(GR_GL_ARRAY_BUFFER, srcSizeInBytes, src, usage)); -#else - if (this->sizeInBytes() == srcSizeInBytes) { - GL_CALL(BufferData(GR_GL_ARRAY_BUFFER, srcSizeInBytes, src, usage)); + + bool doNullHint = GR_GL_USE_BUFFER_DATA_NULL_HINT; +#if GR_GL_MAC_BUFFER_OBJECT_PERFOMANCE_WORKAROUND + GrAssert(!doNullHint); + static int N = 0; + doNullHint = (0 == N % 1024); + ++N; +#endif + if (doNullHint) { + if (this->sizeInBytes() == srcSizeInBytes) { + GL_CALL(BufferData(GR_GL_ARRAY_BUFFER, srcSizeInBytes, src, usage)); + } else { + // Before we call glBufferSubData we give the driver a hint using + // glBufferData with NULL. This makes the old buffer contents + // inaccessible to future draws. The GPU may still be processing + // draws that reference the old contents. With this hint it can + // assign a different allocation for the new contents to avoid + // flushing the gpu past draws consuming the old contents. + GL_CALL(BufferData(GR_GL_ARRAY_BUFFER, + this->sizeInBytes(), NULL, usage)); + GL_CALL(BufferSubData(GR_GL_ARRAY_BUFFER, 0, srcSizeInBytes, src)); + } } else { - // Before we call glBufferSubData we give the driver a hint using - // glBufferData with NULL. This makes the old buffer contents - // inaccessible to future draws. The GPU may still be processing draws - // that reference the old contents. With this hint it can assign a - // different allocation for the new contents to avoid flushing the gpu - // past draws consuming the old contents. - GL_CALL(BufferData(GR_GL_ARRAY_BUFFER, - this->sizeInBytes(), NULL, usage)); - GL_CALL(BufferSubData(GR_GL_ARRAY_BUFFER, 0, srcSizeInBytes, src)); + // Note that we're cheating on the size here. Currently no methods + // allow a partial update that preserves contents of non-updated + // portions of the buffer (lock() does a glBufferData(..size, NULL..)) + GL_CALL(BufferData(GR_GL_ARRAY_BUFFER, srcSizeInBytes, src, usage)); } -#endif return true; } -- 2.7.4