Bug 32748 – Index validation code must always copy client data

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

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.

[Kenneth Russell]

Created attachment 45195 [details]
Patch

[Oliver Hunt]

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?

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45195 [details] without any errors.

[Kenneth Russell]

(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).

[Darin Fisher (:fishd, Google)]

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?

[Kenneth Russell]

(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.

[Maciej Stachowiak]

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.

[Kenneth Russell]

Created attachment 45603 [details]
Revised patch

Addressed review feedback. Renamed test, changed description and added same text to ChangeLogs.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45603 [details] without any errors.

[Maciej Stachowiak]

Comment on attachment 45603 [details]
Revised patch

Thanks for updating the patch!

r=me

[Kenneth Russell]

Thanks for the review; could you please cq+ the patch? I don't have commit privileges yet.

[WebKit Commit Bot]

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

[Kenneth Russell]

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.

[Eric Seidel (no email)]

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.

[WebKit Commit Bot]

Comment on attachment 45603 [details]
Revised patch

Clearing flags on attachment: 45603

Committed r52700: <http://trac.webkit.org/changeset/52700>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.