Reduce draw validation test parameterization #3122
Conversation
This helps a lot in standalone (18s -> 12s) but it probably won't help quite as much in other modes.
kainino0x
requested review from
lokokung and
jiangzhaoming
and removed request for
jiangzhaoming
| // the factor for offset of attributes in vertex layout | ||
| .combine('offset', [0, 1, 2, 7]) // the offset of attribute will be factor * MIN(4, sizeof(vertexFormat)) | ||
| .beginSubcases() | ||
| .combine('setBufferOffset', [0, 200]) // must be a multiple of 4 | ||
| .combine('setBufferOffset', [200]) // must be a multiple of 4 |
There was a problem hiding this comment.
nit. If we are just using 1 value, maybe we can just make it a constant in the test?
There was a problem hiding this comment.
It's a bit silly but I left it here since the test is already parameterized around it. Having it here makes it clear we could add more values in the future if needed.
| @@ -459,7 +478,7 @@ success/error as expected. Such set of buffer parameters should include cases li | |||
| } | |||
|
|
|||
| const calcSetBufferSize = ( | |||
| boundBufferSizeState: 'zero' | 'exile' | 'enough', | |||
| boundBufferSizeState: 'zero' | 'oneTooSmall' | 'exact', | |||
There was a problem hiding this comment.
nit. maybe just 'tooSmall' is good enough? Not sure whether the one part is relevant.
There was a problem hiding this comment.
I think it's kinda helpful to a test reader to know that we're testing the boundary condition. Also both 'zero' and 'oneTooSmall' are too small.
| .combineWithParams([ | ||
| { vertexOffsetFactor: 0.0, instanceOffsetFactor: 0.0, indexOffsetFactor: 0.0 }, | ||
| { vertexOffsetFactor: 1.0, instanceOffsetFactor: 1.0, indexOffsetFactor: 1.0 }, | ||
| // |
There was a problem hiding this comment.
nit. Since you already included comment sections to separate the cases, maybe a brief one-liner explaining what the cases are testing?
There was a problem hiding this comment.
Honestly I don't have much of an explanation here. I just picked some reasonable looking subset of the original combinatorial explosion that would maybe have the right amounts of overlaps. But I don't fully understand how they're used so I can't really write intelligently here.
There was a problem hiding this comment.
Initially these factors are designed to emulate all overlay possibility of three buffer binding within a single GPUBuffer. Consider each binding size is 1, and the offset factor indicate the begin point of each binding form 0 to 2 in 0.5 steps.
So some of the cases are:
- No overlay: [0, 1, 2]
- The first two binding no overlay, third binding overlay: [0, 1, (1, 1.5)]
- The first two partially overlay, and the third one may or may not overlay with them: [0, 0.5, (0.5, 1, 1.5)]
- The first two complete overlay, and the third one may or may not overlay with them: [0, 0, (0, 0.5, 1)]
And three different kind of binding (vertex, instance, index) map to these three binding in several ways.
Yes there are too many combination and many of them is duplicated...
And oops I realize that the expected binding size of three kind is not necessary the same, so combining the factor doesn't ensure all overlay cases are tested.
Maybe I should review this tests later and replace these factor things with explicit overlay expectation...
There was a problem hiding this comment.
Thanks for the explanation, that's about what I thought but I wasn't sure.
Let me see if we can do something like:
- No overlay: [0, 2, 4]
- The first two binding no overlay, third binding overlay: [0, 2, (2, 2.5)]
- The first two partially overlay, and the third one may or may not overlay with them: [0, 0.5, (0.5, 1, 4)]
- The first two complete overlay, and the third one may or may not overlay with them: [0, 0, (0, 0.5, 4)]
There was a problem hiding this comment.
Actually - this test is only a tiny fraction of the slowness here, so I'm just going to revert this, add a TODO for what you suggested, and land this PR.
Draw validation tests were taking a huge chunk (maybe 25%) of the time of the whole api,validation suite. This should reduce their time by roughly 80-90%.
It may be useful to look at each commit separately to have a good sense of how much they're reducing the test coverage by. My estimate is that we still probably have all of the coverage we need.
cc @jzm-intel in case you have opinions about these tests.
Issue: None
Requirements for PR author:
.unimplemented()./** documented */and new helper files are found inhelper_index.txt.Requirements for reviewer sign-off:
When landing this PR, be sure to make any necessary issue status updates.