[webkit-reviews] review denied: [Bug 32466] Performance problems with index validation code for drawElements : [Attachment 44734] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 15 11:26:54 PST 2009


Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Kenneth Russell
<kbr at google.com>'s request for review:
Bug 32466: Performance problems with index validation code for drawElements
https://bugs.webkit.org/show_bug.cgi?id=32466

Attachment 44734: Patch
https://bugs.webkit.org/attachment.cgi?id=44734&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebCore/html/canvas/WebGLBuffer.cpp
...
> +void WebGLBuffer::setCachedMaxIndex(unsigned long type, long value)
> +{
> +    int numEntries = sizeof(m_maxIndexCache) / sizeof(MaxIndexCacheEntry);

nit: You should probably change "i" and "numEntries" to be size_t.


> Index: WebCore/html/canvas/WebGLBuffer.h
...
> +	   // Optimization for index validation. For each type of index
> +	   // (i.e., UNSIGNED_SHORT), cache the maximum index in the
> +	   // entire buffer.
> +	   // 
> +	   // This is sufficient to eliminate a lot of work upon each
> +	   // draw call as long as all bound array buffers are at least
> +	   // that size.
> +	   struct MaxIndexCacheEntry {
> +	       unsigned long type;
> +	       long maxIndex;
> +	   };
> +	   // OpenGL ES 2.0 only has two valid index types (UNSIGNED_BYTE
> +	   // and UNSIGNED_SHORT), but might as well leave open the
> +	   // possibility of adding others.
> +	   MaxIndexCacheEntry m_maxIndexCache[4];
> +	   unsigned m_nextAvailableCacheEntry;

nit: If I were writing this code, I would just have two member variables:

  long m_unsignedByteMaxIndex;
  long m_unsignedShortMaxIndex;

Then I'd have a case-switch in the set function that hits an ASSERT_NOT_REACHED

if given an unknown type.  That way it is really obvious that we should add
another variable in the future.  I think this is simpler, but I agree that your

solution avoids the need to make any future code changes, which might be nice.


> Index: WebCore/html/canvas/WebGLRenderingContext.cpp
...
> -    if (offset < 0 || !validateIndexArray(count, type, offset, numElements)
|| !validateRenderingState(numElements)) {
> +	if (offset < 0
> +	    || !validateElementArraySize(count, type, offset)
> +	    || ((!validateIndexArrayConservative(type, numElements) ||
!validateRenderingState(numElements))
> +		  && (!validateIndexArrayPrecise(count, type, offset,
numElements) || !validateRenderingState(numElements)))) {

nit: I think it would help readability to break this out into a separate
helper function or multiple if checks.


> Index: WebCore/manual-tests/webgl/ManyPlanetsDeepHiRes.html

Since this is mostly a copy of ManyPlanetsDeep.html, can you please
re-factor to avoid duplication?  Maybe the original HTML file can be
loaded as an IFRAME w/ a query parameter to adjust the values of the
call to makeSphere.


More information about the webkit-reviews mailing list