Bug 32748 - Index validation code must always copy client data
Summary: Index validation code must always copy client data
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-18 15:41 PST by Kenneth Russell
Modified: 2010-01-03 18:40 PST (History)
12 users (show)

See Also:


Attachments
Patch (7.26 KB, patch)
2009-12-18 15:46 PST, Kenneth Russell
mjs: review-
Details | Formatted Diff | Diff
Revised patch (8.11 KB, patch)
2009-12-29 08:43 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2009-12-18 15:41:55 PST
The index validation code added in https://bugs.webkit.org/show_bug.cgi?id=31239 tries to minimize copies of indices submitted to OpenGL by referring to client data during calls to bufferData. Unfortunately this optimization is illegal. The client can defeat the validation by uploading out-of-range indices and then changing them only in the client-side copy. Upon submitting data to GL via bufferData, the implementation must immediately copy the incoming data.
Comment 1 Kenneth Russell 2009-12-18 15:46:46 PST
Created attachment 45195 [details]
Patch
Comment 2 Oliver Hunt 2009-12-18 15:49:21 PST
This is exactly the kind of issue i was referring to in the other bug -- honestly i think we should consider caching maximum values in the buffer itself.

Seriously what's the perf cost of disabling this optimisation simply to allow indexing arrays?
Comment 3 WebKit Review Bot 2009-12-18 15:50:46 PST
style-queue ran check-webkit-style on attachment 45195 [details] without any errors.
Comment 4 Kenneth Russell 2009-12-18 16:05:56 PST
(In reply to comment #2)
> This is exactly the kind of issue i was referring to in the other bug --
> honestly i think we should consider caching maximum values in the buffer
> itself.

I remember our earlier discussion.

To be clear, this is a preexisting bug unrelated to the performance optimization I added in https://bugs.webkit.org/show_bug.cgi?id=32466 . Thanks for mentioning it there, because it encouraged me to dig deeper into the validation code.

The semantics of the index validation are that at draw time, precisely the indices referenced in the element array buffer must be validated against the bound array buffers. Always caching the maximum index stored in a WebGLUnsignedByteArray or WebGLUnsignedShortArray would not help. There are situations where a given draw call may not use a particular index in an element array buffer, and it is incorrect to require that all bound array buffers have enough storage to cover the unused index. This is the distinction between the "conservative" and "precise" validation.

> Seriously what's the perf cost of disabling this optimisation simply to allow
> indexing arrays?

I'm not sure to which optimization you are referring. If you are asking why we don't always cache the maximum value in WebGLUnsignedByteArrays and WebGLUnsignedShortArrays, doing so would not help, per above, and it would also be very expensive. If you are asking about the performance optimization I added in https://bugs.webkit.org/show_bug.cgi?id=32466 , I've measured speedups from 50% to 500% on various demos so I would consider it critical to preserve (and unrelated to this bug).
Comment 5 Darin Fisher (:fishd, Google) 2009-12-18 16:39:58 PST
Comment on attachment 45195 [details]
Patch

> Index: WebCore/html/canvas/WebGLBuffer.cpp
...
> +        m_elementArrayBuffer = WebGLArrayBuffer::create(array->buffer().get());

Would it be valuable to change WebGLArrayBuffer to be internally copy-on-write?
Was there no good reason for delaying the copy previously?
Comment 6 Kenneth Russell 2009-12-18 16:45:40 PST
(In reply to comment #5)
> (From update of attachment 45195 [details])
> > Index: WebCore/html/canvas/WebGLBuffer.cpp
> ...
> > +        m_elementArrayBuffer = WebGLArrayBuffer::create(array->buffer().get());
> 
> Would it be valuable to change WebGLArrayBuffer to be internally copy-on-write?
> Was there no good reason for delaying the copy previously?

This wouldn't make sense. The WebGLArrayBuffer is the backing store for one or more views of the data in the form of WebGL[T]Arrays. The client assembles data to hand down to the GL using WebGL[T]Arrays.

The index validation is the only case where the WebGL code itself has to retain a copy of client-side data.
Comment 7 Maciej Stachowiak 2009-12-29 04:34:05 PST
Comment on attachment 45195 [details]
Patch

Code change loks fine, but can you please give the test case a more meaningful name than "bug-32748"? And could you add a description to the test that describes what it is actually testing? Maybe this should go in the ChangeLog too. Basically I'm looking for something that completes the sentence, "Index validation code must always copy client data, because if it doesn't then __________".

I will happily r+ this if you can provide that clarification.
Comment 8 Kenneth Russell 2009-12-29 08:43:51 PST
Created attachment 45603 [details]
Revised patch

Addressed review feedback. Renamed test, changed description and added same text to ChangeLogs.
Comment 9 WebKit Review Bot 2009-12-29 08:45:32 PST
style-queue ran check-webkit-style on attachment 45603 [details] without any errors.
Comment 10 Maciej Stachowiak 2009-12-30 15:04:27 PST
Comment on attachment 45603 [details]
Revised patch

Thanks for updating the patch!

r=me
Comment 11 Kenneth Russell 2009-12-30 15:13:06 PST
Thanks for the review; could you please cq+ the patch? I don't have commit privileges yet.
Comment 12 WebKit Commit Bot 2009-12-31 10:40:52 PST
Comment on attachment 45603 [details]
Revised patch

Rejecting patch 45603 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueueSVN/LayoutTests
Testing 11859 test cases.
animations/3d/state-at-end-event-transform.html -> new (results generated in /Users/eseidel/Projects/CommitQueueSVN/LayoutTests/platform/mac/animations/3d)

Exiting early after 1 failures. 113 tests run.
13.49s total testing time

112 test cases (99%) succeeded
1 test case (<1%) was new
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/155189
Comment 13 Kenneth Russell 2009-12-31 15:54:07 PST
I'm not sure whether this is a flaky or broken layout test but its failure is unrelated to my changes. Requesting commit-queue again.
Comment 14 Eric Seidel (no email) 2010-01-01 10:30:24 PST
I suspect this is another instance of bug 28603.  I really should move the commit-queue back to using git until bug 28603 can be fixed.
Comment 15 WebKit Commit Bot 2010-01-03 18:40:11 PST
Comment on attachment 45603 [details]
Revised patch

Clearing flags on attachment: 45603

Committed r52700: <http://trac.webkit.org/changeset/52700>
Comment 16 WebKit Commit Bot 2010-01-03 18:40:20 PST
All reviewed patches have been landed.  Closing bug.