Memory sync test for buffer: write after write in the same pass #384

Richard-Yunchao · 2020-11-23T07:53:59Z

PTAL. Thanks. The naming like verifyAlternativeData and expectAlternativeContents might not be very good, though. But I can't think of a better one.

src/webgpu/gpu_test.ts

kainino0x · 2020-11-24T00:05:22Z

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

@@ -83,12 +83,54 @@ g.test('two_draws_in_the_same_render_pass')
    a storage buffer. The second write will write 2 into the same buffer in the same pass. Expected
    data in buffer is either 1 or 2. It may use bundle in each draw.`
  )
-  .unimplemented();
+  .params(pbool('useBundle'))


Now that I'm looking at the implementation, I think we should test at least these three options:

same pass, no bundle

same pass, different bundles

same bundle

Agreed. I was meant to test 1) same pass, no bundle; 2) same pass, one bundle; 3) same pass, two separate bundles. But I wrote it wrongly. And I missed same bundle (definitely one pass).
Now I added all of them. Note that I separated the tests two-draws-in-the-same-bundle from the original one. I tried to merge into the same one, but it requires unless and multiple ifs, making the code logic in a mess. So I think it is better to write a standalone test for same-bundle test.

Richard-Yunchao · 2020-11-24T19:14:44Z

src/webgpu/gpu_test.ts

-        niceStack.message = check1 + check2;
+        niceStack.message = <span class="x x-first x-last">`Expected one of the following two checks to succeed:</span>
+- ${check1}
+- ${check2}`;


@kainino0x , do you know why this code snippet failed to build? I removed <span> stuff, it works.

I have no idea where the span stuff came from?? It must have been a GitHub bug.

Kangz

LGTM

Kangz · 2020-11-25T14:49:05Z

src/webgpu/gpu_test.ts

@@ -167,6 +167,35 @@ export class GPUTest extends Fixture {
    });
  }

+  expectContentsMultipleValidValues(
+    src: GPUBuffer,
+    expected1: TypedArrayBufferView,


If anything, I think this should take a sequence<TypedArrayBufferView>. Maybe we could also refactor the expectContents to take a validator function so it's easy to build additional variations in the future.

Thanks for your suggestions, Corentin. I considered about this too but didn't do this because: 1) it is somehow over-engineering. 2) actually we have two directions to expand this function:

expand two values to an array (or a sequence), and the expected result should fall into any value of the array. it is just like any() function. This direction is what you said.

expand two values to two vectors, and the expected result should be a vector whose value at any index should be either of the two values at the same index of the two vectors. Take (R, G, B, A) as an example, if two vectors are (0, 0, 0, 0) and (255, 255, 255, 255), then there are 16 correct values like (0, 0, 0, 0), (0, 0, 0, 255), ..., (0, 255, 255, 0), ..., (255, 255, 255, 255). This is another possible expansion. If we have multiple values in the buffer, a race condition of write after write is more likely to behave in this direction. But I don't want to make the test too complicated.

Considering that we have two directions and it will bring unnecessary complex, I didn't expand this function.

How about we keep it and expand this function to whatever we want if we do need it in future?

While I think it is ok in this test fixture, I have concerns keeping the method in the harness as-is: it says 'multiple values" but only take two. In general the harness needs to be higher quality and more future proof than test code.

What we could do is do the refactor of expectContents to to have a version that takes a validator and make the validator check agains the two values in the fixture.

Yeah. I have concerns about the naming stuff too (I said about the naming problem at the very beginning). I hesitated to pick up a name like alternative, either, two, multiple, etc. Let's be clear and just say "two" valid values here. And we can expand it to multiple valid values or two mixed vectors if needed in future. I understand that the code should be future-proof but I don't think it is very clear that which direction is the future (maybe neither of them is needed). So I added a comment about it. WDYT?

The comment is inconsistent with the idea of passing a validator function, but it links to this discussion, so it's fine. I also considered suggesting passing an array here (instead of 2 values); it wouldn't really be over-engineering as it wouldn't be any more complex. However I think a validator function makes more sense in the future. A validator function pattern also works much better for texture content expectation. (I was thinking of passing a function(x,y,z)->expected value into expectTextureContents but a validator is more flexible.)

Kangz · 2020-11-25T14:49:28Z

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

@@ -255,4 +255,12 @@ export class BufferSyncTest extends GPUTest {
    bufferData[0] = expectedValue;
    this.expectContents(buffer, bufferData);
  }
+
+  verifyDataMultipleValidValues(buffer: GPUBuffer, expectedValue1: number, expectedValue2: number) {


nit: this could take a sequence<number>

same reason

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

kainino0x · 2020-12-01T22:01:52Z

src/webgpu/gpu_test.ts

@@ -167,6 +167,35 @@ export class GPUTest extends Fixture {
    });
  }

+  expectContentsMultipleValidValues(
+    src: GPUBuffer,
+    expected1: TypedArrayBufferView,


The comment is inconsistent with the idea of passing a validator function, but it links to this discussion, so it's fine. I also considered suggesting passing an array here (instead of 2 values); it wouldn't really be over-engineering as it wouldn't be any more complex. However I think a validator function makes more sense in the future. A validator function pattern also works much better for texture content expectation. (I was thinking of passing a function(x,y,z)->expected value into expectTextureContents but a validator is more flexible.)

Memory sync test for buffer: write after write in the same pass

f410e03

Richard-Yunchao requested a review from kainino0x November 23, 2020 07:54

Richard-Yunchao assigned Kangz and austinEng and unassigned Kangz and austinEng Nov 23, 2020

Richard-Yunchao requested review from austinEng and Kangz November 23, 2020 07:54

kainino0x approved these changes Nov 24, 2020

View reviewed changes

Richard-Yunchao commented Nov 24, 2020

View reviewed changes

Address feedback from Kai

cab6c9b

Richard-Yunchao force-pushed the buffer-sync-ww-in-same-pass branch from 1908733 to cab6c9b Compare November 24, 2020 19:28

Kangz approved these changes Nov 25, 2020

View reviewed changes

Address feedback from Corentin

2c76c23

kainino0x approved these changes Dec 1, 2020

View reviewed changes

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

c68e800

kainino0x mentioned this pull request Dec 1, 2020

Make expectContents take a "validator" function #388

Closed

kainino0x merged commit 3c2fe38 into gpuweb:main Dec 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory sync test for buffer: write after write in the same pass #384

Memory sync test for buffer: write after write in the same pass #384

Richard-Yunchao commented Nov 23, 2020

kainino0x Nov 24, 2020

Richard-Yunchao Nov 24, 2020 •

edited

Loading

Richard-Yunchao Nov 24, 2020 •

edited

Loading

kainino0x Dec 1, 2020

Kangz left a comment

Kangz Nov 25, 2020

Richard-Yunchao Dec 1, 2020 •

edited

Loading

Kangz Dec 1, 2020 •

edited

Loading

Richard-Yunchao Dec 1, 2020

kainino0x Dec 1, 2020

Kangz Nov 25, 2020

Richard-Yunchao Dec 1, 2020

kainino0x Dec 1, 2020

Memory sync test for buffer: write after write in the same pass #384

Memory sync test for buffer: write after write in the same pass #384

Conversation

Richard-Yunchao commented Nov 23, 2020

Choose a reason for hiding this comment

Richard-Yunchao Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

Richard-Yunchao Nov 24, 2020 • edited Loading

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

Richard-Yunchao Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Kangz Dec 1, 2020 • edited Loading

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

Richard-Yunchao Nov 24, 2020 •

edited

Loading

Richard-Yunchao Nov 24, 2020 •

edited

Loading

Richard-Yunchao Dec 1, 2020 •

edited

Loading

Kangz Dec 1, 2020 •

edited

Loading