WEBGL_get_buffer_sub_data_async extension #2079

kainino0x · 2016-10-06T00:44:18Z

I took @kenrussell's spec text from #1937 but significantly rewrote it text to exactly match the new text for getBufferSubData where appropriate. I also changed the IDL to match getBufferSubData.

This entry point solves the problem of synchronous, blocking GPU-to-CPU memory readback on the main thread.

Some performance results in real-world usage (screen picking):
https://github.com/kainino0x/getBufferSubDataAsync-Demo

Copy rewording from clarify and test corner cases of getBufferSubData #2078 once it's settled
Don't test ordering: WEBGL_get_buffer_sub_data_async extension #2079 (comment)
Add test for the transform feedback error case to the test added in add test for getBufferSubData when transform feedback is active #2097 (it should be added to verifyGetBufferSubData)
Move to an extension
Remove unnecessary manual tests in sdk/tests/extra/.

This PR fixes #1938.

kenrussell

This is excellent work @kainino0x . The tests are superb and you clearly thought through all of the edge cases when writing them.

Working group members, please review this; in particular, @jdashg @Kangz @grorg @RafaelCintron @zhenyao @junov . All of you reviewed the earlier pull request #1937 which has been closed in favor of this one.

Kai has implemented this in Chrome and it's showing excellent results. It eliminates the synchronous cost of getting data back from the GPU, and in Chrome, the time between when getBufferSubDataAsync is called and when the promise is resolved is less than when making the synchronous getBufferSubData call! This benefit will be realized for any multi-process or multi-threaded WebGL implementation. Kai will send more details and a test case to the mailing list tomorrow.

A couple of minor comments about the tests. They look basically perfect overall. The chaining of promises together in the main test is an excellent design and is comprehensible even to newcomers.

kenrussell · 2016-10-06T03:15:02Z

sdk/tests/conformance2/buffers/getBufferSubDataAsync-stress.html

+for (var i = 0; i < TEST_COUNT; i++) {
+  (function() {
+    var counter = i;
+    var color = [counter & 0xff, (counter << 8) & 0xff, (counter << 16) & 0xff, 255];


Did you mean to right shift instead of left shift these? They'll always shift in zero bits as written.

Still looking for an update in this area.

Whoops, missed this. Fixed.

kenrussell · 2016-10-06T03:21:24Z

sdk/tests/conformance2/buffers/getBufferSubDataAsync.html

+  debug("");
+  finishTest();
+}).catch(function(e) {
+  testFailed(e.toString());


Should this also call finishTest so the test harness (webgl-conformance-test.html) doesn't report this as a timeout if an exception is thrown?

kenrussell

Looking great. One unresolved comment from earlier.

kenrussell · 2016-10-07T03:22:35Z

sdk/tests/conformance2/buffers/getBufferSubDataAsync-stress.html

+for (var i = 0; i < TEST_COUNT; i++) {
+  (function() {
+    var counter = i;
+    var color = [counter & 0xff, (counter << 8) & 0xff, (counter << 16) & 0xff, 255];


Still looking for an update in this area.

Kangz

LGTM overall, a couple comments.

Kangz · 2016-10-07T14:37:09Z

sdk/tests/conformance2/buffers/getBufferSubDataAsync-stress.html

+            buf + " but expected " + color);
+      }
+      if (counter <= last_counter) {
+        testFailed("getBufferSubDataAsync calls #" + last_counter + " and #" + counter + " out of order");


The getBufferSubDataAsync promises being called in order doesn't appear in the spec (or I didn't find it) and seems to be an implementation detail. How about having a vector of bools and in Promise.all(promises).then() check that it is all true?

That is a good point. The command "executes serially with respect to others in the command stream" but the promises may not need to return that way. However, I'm not totally certain whether it should be changed in the spec or the test. @kenrussell, what do you think?

I'm not sure how the guarantees about what data is seen in the ArrayBufferView would be maintained if the promises resolved out of order. The same ArrayBufferView might be used as the target to getBufferSubData multiple times, and the promises' "then" callbacks would assume the data would be observed sequentially.

Should we say something in the spec about sequential calls A and B to getBufferSubDataAsync, and that the promise for call A must be resolved before the promise to call B, assuming both are not rejected early?

It'd be fine by me to change the test. Is there a simple way to express success? I would assume that Promise.all's resolve callback would only be called upon success of all of the individual promises and their resolve callbacks -- so throwing an exception out of one of them would cause Promise.all to fail. Could you investigate?

Since the ArrayBufferView isn't written until promise resolution, I don't see a problem there. We don't make any guarantees about the contents of the ArrayBufferView after the promise callback is executed; only during its execution.

Yes, failing one would cause Promise.all to fail. But there can be testFailed messages inside the individual callbacks as well so I think that would be okay.

Like Kai, I don't think we need to add ordering to the spec, the fact that all the promises are resolved on the borwsing context event loop gives the guarantee that inside the promise.then, the ArrayBufferView content is correct.

OK. Given your analyses I think it's fine to leave the spec as is.

Kangz · 2016-10-07T14:45:26Z

sdk/tests/conformance2/buffers/getBufferSubDataAsync.html

+});
+
+allPromises = allPromises.then(function() {
+  var code = "promise = gl.getBufferSubDataAsync(gl.ARRAY_BUFFER, 0, new ArrayBuffer(4))";


Why should these be rejected? Is it because no buffer is bound to gl.ARRAY_BUFFER (I think some might be)?

Those are type checks, which I didn't make clear at all. I added debug prints to explain.

PASS correctly rejected with TypeError: Failed to execute 'getBufferSubDataAsync' on 'WebGL2RenderingContext': parameter 3 is not of type 'ArrayBufferView'.: promise = gl.getBufferSubDataAsync(gl.ARRAY_BUFFER, 0, new ArrayBuffer(4))

PASS correctly rejected with TypeError: Failed to execute 'getBufferSubDataAsync' on 'WebGL2RenderingContext': parameter 3 is not of type 'ArrayBufferView'.: promise = gl.getBufferSubDataAsync(gl.ARRAY_BUFFER, 0, null)

Kangz · 2016-10-07T14:47:30Z

sdk/tests/conformance2/buffers/getBufferSubDataAsync.html

+  wtu.shouldGenerateGLError(gl, gl.INVALID_VALUE, code);
+  return expectRejected(promise, code);
+});
+


A couple more tests that might be useful:

Check that getBufferSubDataAsync on an uninitialized (gl.bufferData with null) buffer returns zeros.

Check that getBufferSubDataAsync immediately followed by a buffer resize to be too small doesn't crash.

Thanks, I will add those.

kainino0x · 2016-10-07T18:43:26Z

specs/latest/2.0/index.html

+        <code>dstData</code>. If any of the above errors occur, the Promise is already rejected upon
+        return. If the context is lost before the Promise is fulfilled, or if <code>dstData</code>
+        is neutered at the time the Promise is fulfilled, then the Promise will be rejected later.
+        In this case, no WebGL error is generated. If the promise rejects, no data is written to


@kenrussell Just want to make sure that this is the semantic we want. Right now no WebGL error will ever be generated upon resolution of the promise.

Yes, that sounds like the best semantic to me.

kainino0x · 2016-10-07T21:57:05Z

I merged master into this branch, updated the spec language to match getBufferSubData, etc. I think I've addressed all of the outstanding comments and issues.

Kangz · 2016-10-11T14:46:47Z

Still LGTM

kainino0x · 2016-10-12T21:07:36Z

Added a test for the transform feedback error case (finally).

Also added in getBufferSubData tests in #2097

… active" This reverts commit a695837.

kainino0x · 2016-10-12T22:08:46Z

Added a test for the transform feedback error case (finally).

Removed, will add it back to this PR once that one has landed and this PR's fate has stabilized.

EDIT: it's added back now

kenrussell · 2016-10-24T21:16:25Z

Excellent work. For a little more background for other reviewers: the new form of the spec text came from a suggestion from Elliott Sprehn from the Blink team, pointing us at other web specs that return promises.

LGTM.

kainino0x · 2016-12-14T01:25:45Z

I've finally moved this entire spec into an extension. The extension is implemented and tested on my local Chromium build. Aside from the new specification text, everything is as it was before. Ready for review.

zhenyao

LGTM with a question
Feel free to merge yourself

zhenyao · 2016-12-15T18:14:59Z

sdk/tests/conformance2/extensions/webgl-get-buffer-sub-data-async-stress.html

+  var promises = [];
+  debug("kicking off " + TEST_COUNT + " getBufferSubDataAsync calls");
+  for (var i = 0; i < TEST_COUNT; i++) {
+    (function() {


Curious: why make the loop body a function? why regular code will not do?

This is a javascript programming pattern that guards against a weirdness of closures, which is that they always capture variables, not values. So if you capture i, then all of the closures (promise callbacks) will see i == TEST_COUNT instead of the loop value.

After writing this, though, I learned about let, which actually will solve this. I'm going to go ahead and clean this up using let.

zhenyao · 2016-12-15T18:17:13Z

sdk/tests/conformance2/extensions/webgl-get-buffer-sub-data-async.html

+  });
+
+  for (var i = 0; i < types.length; i++) {
+    (function() {


Same question here.

zhenyao · 2016-12-15T18:32:40Z

sdk/tests/extra/getBufferSubDataAsync-dom-suspension-manual.html

+  debug("kicking off " + TEST_COUNT + " getBufferSubDataAsync calls");
+  for (var i = 0; i < TEST_COUNT; i++) {
+    insertTimeoutAssertion();
+    (function() {


Same question here?

I actually forgot about these tests and haven't updated them. I need to do this before I merge.

What about leaving these manual tests out of this pull request?

Sure, good call. Done.

zhenyao · 2016-12-15T21:59:12Z

lgtm

kenrussell · 2016-12-19T01:10:50Z

Very nice @kainino0x and thanks @zhenyao for carrying forward the code review. LGTM after the fact.

Add an extension for asynchronous buffer readback. Name subject to bikeshedding. Fixes KhronosGroup#1938.

kainino0x added 5 commits October 5, 2016 16:59

WebGL 2: test getBufferSubDataAsync

5dadf4e

stress test getBufferSubDataAsync

cd7d8bd

getBufferSubDataAsync-lose-context.html

a21d8ca

add getBufferSubDataAsync to WebGL 2 spec

05f5648

more tests for getBufferSubDataAsync

183846a

kainino0x mentioned this pull request Oct 6, 2016

Add getBufferSubDataAsync. #1937

Closed

add corner case

4be0799

kainino0x force-pushed the async branch from 6fed00f to 4be0799 Compare October 6, 2016 01:30

kenrussell reviewed Oct 6, 2016

View reviewed changes

kenrussell mentioned this pull request Oct 6, 2016

Add conformance tests for getBufferSubDataAsync #1938

Closed

update spec language to match new getBufferSubData

4e129ff

kenrussell reviewed Oct 7, 2016

View reviewed changes

Kangz reviewed Oct 7, 2016

View reviewed changes

Correct stress test and add more tests

b00c412

kainino0x mentioned this pull request Oct 7, 2016

clarify and test corner cases of getBufferSubData #2078

Merged

Merge branch 'master' into async

df573f4

kainino0x commented Oct 7, 2016

View reviewed changes

kainino0x added 3 commits October 7, 2016 14:47

remove ordering requirement, and fix/clean up some catches

27b3624

Merge branch 'master' into async

f357342

update spec language to match getBufferSubData

8b7df39

hopefully improve robustness of getBufferSubDataAsync-lose-context.html

81121c7

add test for getBufferSubDataAsync when transform feedback is active

a695837

kainino0x added 2 commits October 12, 2016 14:09

Merge branch 'master' into async

19f92b5

Revert "add test for getBufferSubDataAsync when transform feedback is…

b060aac

… active" This reverts commit a695837.

kainino0x added 2 commits October 12, 2016 16:12

small fix

4af531e

Merge branch 'master' into async

1e89eb2

kainino0x added 4 commits October 12, 2016 16:15

test transform feedback error case

a8a2d58

Merge branch 'master' into async

4c12d19

formalize getBufferSubDataAsync spec text + modify stress test

80df4a0

Merge branch 'master' into async

2ba44a8

kainino0x added 5 commits November 4, 2016 14:13

Merge branch 'master' into async

90b809d

Merge branch 'master' into async

cceab14

move to extension

fa9821c

update tests

100232b

write extension draft

3c9a7c6

zhenyao approved these changes Dec 15, 2016

View reviewed changes

kainino0x added 2 commits December 15, 2016 10:54

use 'let' in for-loops

4748934

remove manual tests which were not updated anyway

5cabc1a

kainino0x changed the title ~~add+test getBufferSubDataAsync for WebGL 2~~ WEBGL_get_buffer_sub_data_async extension Dec 15, 2016

kainino0x merged commit 8415ca3 into KhronosGroup:master Dec 15, 2016

kainino0x deleted the async branch December 15, 2016 19:41

xingri pushed a commit to xingri/WebGL that referenced this pull request Jan 6, 2017

WEBGL_get_buffer_sub_data_async extension (KhronosGroup#2079)

be9770f

Add an extension for asynchronous buffer readback. Name subject to bikeshedding. Fixes KhronosGroup#1938.

grovesNL mentioned this pull request Feb 11, 2017

Compiling/validating shaders asynchronously gpuweb/gpuweb#6

Closed

kainino0x restored the async branch February 24, 2017 18:42

kainino0x deleted the async branch February 24, 2017 18:45

WEBGL_get_buffer_sub_data_async extension #2079

WEBGL_get_buffer_sub_data_async extension #2079

Conversation

kainino0x commented Oct 6, 2016 • edited

kenrussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenrussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kangz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kainino0x commented Oct 7, 2016

Kangz commented Oct 11, 2016

kainino0x commented Oct 12, 2016

kainino0x commented Oct 12, 2016 • edited

kenrussell commented Oct 24, 2016

kainino0x commented Dec 14, 2016

zhenyao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenyao commented Dec 15, 2016

kenrussell commented Dec 19, 2016

kainino0x commented Oct 6, 2016 •

edited

kainino0x commented Oct 12, 2016 •

edited