Memory sync test for buffer #302

Richard-Yunchao · 2020-10-02T23:39:01Z

This change add memory synchronization tests for buffer in compute.
Each test read the data from a buffer, add 1, and write back to the
buffer and iterate twice in different validation scopes: different
dispatch calls, different passes, or different command buffers. Then
verify the data in buffer.

Richard-Yunchao · 2020-10-02T23:44:41Z

The first pr for memory sync tests for buffer. I verified the tests in Chromium because currently we have little wgsl code in CTS. It works, which indicate that we can support storage buffer in wgsl(tint) in Chromium. Please take a look at the latest test plan too (We may need to add more binding types, and/or read-after-write, write-after-write, read-before-write combinations). Comments are welcome.

Richard-Yunchao · 2020-10-06T00:44:48Z

Please don't review this pr. I have revised it at my local side. I will upload the new one soon.

kainino0x · 2020-10-06T02:45:40Z

Thanks for the heads up. FYI I just reviewed your test plan and I think it looks good, if we can manage to write the code for it. I changed one small thing and added a suggestion for the R-W and W-R tests.

Richard-Yunchao · 2020-10-06T16:24:55Z

Thanks for reviewing the test plan. @kainino0x . This is the first test for write-after-write test. PTAL. Note that I may separate write-after-write into a few tests (for example, copy_and_pass_to pass, and copy_and_pass_to_copy, considering the complexity and the latter will change the destination buffer. Or 4 categories: {copy, pass} x {copy, pass}). The current test is pass to pass. Comments for how to organize the upcoming PRs are also welcome.

kainino0x · 2020-10-08T01:39:43Z

Sorry I still haven't been able to get to this. There is too much to do. :)

Richard-Yunchao · 2020-10-08T02:33:22Z

Sorry I still haven't been able to get to this. There is too much to do. :)

No worries. I suppose the test plan is OK. So I am writing tests for the others at my local side.

src/webgpu/api/operation/memory_sync/buffer.spec.ts

kainino0x

Sorry it's taken me so long to look at this. Added a few comments. @Kangz could you continue to work with Yunchao on the open comments?

Overall I would like to make this cover the complete matrix of pairs of possible reads/writes. Helpers that can do functionally-equivalent reads and writes in every possible way would be complicated but would hopefully make the tests easy to write (and review). It would probably be a helper class, because in a way these tests are stateful (what type of encoder is current) and it should encapsulate that.

src/webgpu/api/operation/memory_sync/buffer.spec.ts

Richard-Yunchao · 2020-10-19T07:13:41Z

Thanks for your review, @Kangz and @kainino0x . It is a little bit late to address all comments because the changes are relatively significant. PTAL.

kainino0x

Latest changes LGTM. I'm sorry I haven't had time to review the whole thing in a while, I am not currently certain about whether this is implementing the direction we want to take or not.

Maybe tomorrow we can chat on slack (maybe in the morning with @Kangz too?) about what the current state of these tests (this PR + the later ones) is? And also talk about fully general read-op/write-op abstractions that should be generally useful in the test suite.

Richard-Yunchao · 2020-10-23T02:50:17Z

Latest changes LGTM. I'm sorry I haven't had time to review the whole thing in a while, I am not currently certain about whether this is implementing the direction we want to take or not.

Maybe tomorrow we can chat on slack (maybe in the morning with @Kangz too?) about what the current state of these tests (this PR + the later ones) is? And also talk about fully general read-op/write-op abstractions that should be generally useful in the test suite.

No problem for me. We can discuss via slack tomorrow morning if needed. What about the time around 9am or 10am?

My rough idea is:

Put all helper functions, for example, createBuffer, createBindGroup, writeByRenderPass, etc., in a separate file. And write tests in write-after-write-in-buffer.spec.ts, read-after-write-in-buffer.spec.ts, and read-before-write-in-buffer.spec.ts.
In each test, it might has a couple of comprehensive tests of combined parameters to cover most cases, while a few separated tests to cover some special cases. Take write-after-write as an example, the comprehensive test is the current one we are reviewing (write-after-write), but we need to test two writes in the same render or compute pass. The latter tests are not easy to be combined into the current test, and the expected result of render pass (two writes in the same render pass) should have loose guarantees.

kainino0x · 2020-10-23T02:52:57Z

Thanks for the summary, that will definitely be helpful to discuss.

github-actions · 2020-11-10T22:58:27Z

Previews, as seen at the time of posting this comment:
Run this PR's tests live
_0baf2e5

Richard-Yunchao · 2020-11-10T23:05:16Z

@kainino0x , I added fence for buffer initialization, Please take another look.

kainino0x · 2020-11-10T23:48:48Z

Latest change looks good. Did you have a chance to work on the plan document we talked about for these tests? (If so, I haven't seen it, sorry!)

Richard-Yunchao · 2020-11-11T00:33:42Z

Latest change looks good. Did you have a chance to work on the plan document we talked about for these tests? (If so, I haven't seen it, sorry!)

Yeah. I am working on that. Will send out it soon.

Richard-Yunchao · 2020-11-17T01:46:34Z

@kainino0x , Is this change ready for being merged? What do I need to do if it is not ready?

kainino0x · 2020-11-17T02:35:15Z

@kainino0x , Is this change ready for being merged? What do I need to do if it is not ready?

I can't rereview this in detail tonight but I don't think it matches the structure in the plan doc. I don't think it will be easier for me to review the changes than to just rereview the tests from scratch once those changes have been made, so I think I'd rather update this PR to match at least the high level structure (file/test/parameterization structure, test descriptions, helpers), rather than landing it and then moving a bunch of code around.

Richard-Yunchao · 2020-11-17T04:42:14Z

@kainino0x , Is this change ready for being merged? What do I need to do if it is not ready?

I can't rereview this in detail tonight but I don't think it matches the structure in the plan doc. I don't think it will be easier for me to review the changes than to just rereview the tests from scratch once those changes have been made, so I think I'd rather update this PR to match at least the high level structure (file/test/parameterization structure, test descriptions, helpers), rather than landing it and then moving a bunch of code around.

No problem. I separated the helper functions into a standalone file. I think the vast majority (if not all) of the helper functions can be shared among tests.

I noticed your latest comment, are you saying that the helper functions should not be put in a dedicated class for buffer sync test (I named the class as BufferSyncTest though)?

The bot failed, but it looks like it is not caused by the test code according to logs?

Kangz

Looks much better. I think the idea about a context with various encoders could still prove useful in the future but I don't think we need to block on it.

src/webgpu/api/operation/memory_sync/buffer/write_after_write_in_buffer.spec.ts

src/webgpu/api/operation/memory_sync/buffer/buffer_sync_test.ts

src/webgpu/api/operation/memory_sync/buffer/write_after_write_in_buffer.spec.ts

This pull request adds write-after-write tests for buffer sync. The tests write twice (different data) into render pass or compute pass. The second write should overwrite the first one. The tests also consider factors like using bundle or not for render, using the same command buffer or separate command buffers, etc.

It uses new syntax for wgsl, adds writeOps like copy, writeBuffer, map write, etc., and adds tests for separate submits.

kainino0x · 2020-11-17T22:09:37Z

I noticed your latest comment, are you saying that the helper functions should not be put in a dedicated class for buffer sync test (I named the class as BufferSyncTest though)?

Yes, I don't think they need to be part of a fixture; they can just be standalone helper functions that take the GPUDevice as an argument.

kainino0x

Many small nits but looking good overall!

src/webgpu/api/operation/memory_sync/buffer/buffer_sync_test.ts

src/webgpu/api/operation/memory_sync/buffer/write_after_write_in_buffer.spec.ts

src/webgpu/api/operation/memory_sync/buffer/buffer_sync_test.ts

src/webgpu/api/operation/memory_sync/buffer/write_after_write_in_buffer.spec.ts

src/webgpu/api/operation/memory_sync/buffer/buffer_sync_test.ts

Richard-Yunchao · 2020-11-19T01:16:46Z

Thanks for your detailed review, Corentin and Kai. I forgot to use git merge but rebased the code in the middle in order to fix a build error after I updated my local main branch. Sorry for that. It might be inconvenient for you to review.

src/webgpu/api/operation/memory_sync/buffer/ww.spec.ts

src/webgpu/api/operation/memory_sync/buffer/buffer_sync_test.ts

src/webgpu/api/operation/memory_sync/buffer/write_after_write_in_buffer.spec.ts

kainino0x

np, in this case I just reviewed the two new commits assuming there's nothing new before that.

kainino0x · 2020-11-19T19:18:32Z

I cleaned up the "resolved" comment state so every comment that isn't marked as resolved still needs attention.

Richard-Yunchao · 2020-11-21T02:37:40Z

All comments were resolved. PTAL @kainino0x

src/webgpu/api/operation/memory_sync/buffer/buffer_sync_test.ts

src/webgpu/api/operation/memory_sync/buffer/rw_and_wr.spec.ts

kainino0x · 2020-11-21T03:00:09Z

LGTM

Richard-Yunchao requested review from austinEng, kainino0x and Kangz October 2, 2020 23:45

Richard-Yunchao force-pushed the memory-sync-for-buffer branch from 4b382bd to 344640e Compare October 6, 2020 16:04

Kangz reviewed Oct 8, 2020

View reviewed changes

Richard-Yunchao commented Oct 9, 2020

View reviewed changes

src/webgpu/api/operation/memory_sync/buffer.spec.ts Outdated Show resolved Hide resolved

kainino0x reviewed Oct 14, 2020

View reviewed changes

Richard-Yunchao commented Oct 19, 2020

View reviewed changes

src/webgpu/api/operation/memory_sync/buffer.spec.ts Outdated Show resolved Hide resolved

kainino0x reviewed Oct 23, 2020

View reviewed changes

Kangz reviewed Nov 17, 2020

View reviewed changes

Richard-Yunchao added 4 commits November 17, 2020 12:25

Address feedback from Corentin

3085b3a

Address feedback from Corentin and Kai

e637121

It uses new syntax for wgsl, adds writeOps like copy, writeBuffer, map write, etc., and adds tests for separate submits.

Address feedback from Kai: make mapAsync related calls async

5b4b9d1

Richard-Yunchao added 3 commits November 17, 2020 12:25

Remove mapWrite related tests for write-after-write tests

f1dc183

Address Kai's feedback: add fence for buffer initialization

e29c5a5

Restructure the test file

8233566

kainino0x reviewed Nov 17, 2020

View reviewed changes

Richard-Yunchao added 2 commits November 17, 2020 22:44

Address feedback from Corentin

077f403

Address feedback from Kai

1ed1108

Richard-Yunchao force-pushed the memory-sync-for-buffer branch from d807fe5 to 1ed1108 Compare November 19, 2020 00:53

Richard-Yunchao commented Nov 19, 2020

View reviewed changes

src/webgpu/api/operation/memory_sync/buffer/buffer_sync_test.ts Show resolved Hide resolved

kainino0x reviewed Nov 19, 2020

View reviewed changes

kainino0x approved these changes Nov 19, 2020

View reviewed changes

Address feedback from Kai

bb99c3a

kainino0x approved these changes Nov 21, 2020

View reviewed changes

src/webgpu/api/operation/memory_sync/buffer/buffer_sync_test.ts Outdated Show resolved Hide resolved

src/webgpu/api/operation/memory_sync/buffer/rw_and_wr.spec.ts Outdated Show resolved Hide resolved

fix typos

c256233

kainino0x merged commit 6107b35 into gpuweb:main Nov 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory sync test for buffer #302

Memory sync test for buffer #302

Richard-Yunchao commented Oct 2, 2020

Richard-Yunchao commented Oct 2, 2020

Richard-Yunchao commented Oct 6, 2020

kainino0x commented Oct 6, 2020 •

edited

Richard-Yunchao commented Oct 6, 2020

kainino0x commented Oct 8, 2020

Richard-Yunchao commented Oct 8, 2020

kainino0x left a comment

Richard-Yunchao commented Oct 19, 2020

kainino0x left a comment

Richard-Yunchao commented Oct 23, 2020 •

edited

kainino0x commented Oct 23, 2020

github-actions bot commented Nov 10, 2020

Richard-Yunchao commented Nov 10, 2020

kainino0x commented Nov 10, 2020

Richard-Yunchao commented Nov 11, 2020

Richard-Yunchao commented Nov 17, 2020

kainino0x commented Nov 17, 2020

Richard-Yunchao commented Nov 17, 2020

Kangz left a comment

kainino0x commented Nov 17, 2020

kainino0x left a comment

Richard-Yunchao commented Nov 19, 2020

kainino0x left a comment

kainino0x commented Nov 19, 2020

Richard-Yunchao commented Nov 21, 2020

kainino0x commented Nov 21, 2020

Memory sync test for buffer #302

Memory sync test for buffer #302

Conversation

Richard-Yunchao commented Oct 2, 2020

Richard-Yunchao commented Oct 2, 2020

Richard-Yunchao commented Oct 6, 2020

kainino0x commented Oct 6, 2020 • edited

Richard-Yunchao commented Oct 6, 2020

kainino0x commented Oct 8, 2020

Richard-Yunchao commented Oct 8, 2020

kainino0x left a comment

Choose a reason for hiding this comment

Richard-Yunchao commented Oct 19, 2020

kainino0x left a comment

Choose a reason for hiding this comment

Richard-Yunchao commented Oct 23, 2020 • edited

kainino0x commented Oct 23, 2020

github-actions bot commented Nov 10, 2020

Richard-Yunchao commented Nov 10, 2020

kainino0x commented Nov 10, 2020

Richard-Yunchao commented Nov 11, 2020

Richard-Yunchao commented Nov 17, 2020

kainino0x commented Nov 17, 2020

Richard-Yunchao commented Nov 17, 2020

Kangz left a comment

Choose a reason for hiding this comment

kainino0x commented Nov 17, 2020

kainino0x left a comment

Choose a reason for hiding this comment

Richard-Yunchao commented Nov 19, 2020

kainino0x left a comment

Choose a reason for hiding this comment

kainino0x commented Nov 19, 2020

Richard-Yunchao commented Nov 21, 2020

kainino0x commented Nov 21, 2020

kainino0x commented Oct 6, 2020 •

edited

Richard-Yunchao commented Oct 23, 2020 •

edited