Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement api,validation,state,device_mismatched: Part II #1094

Merged
merged 4 commits into from
Mar 23, 2022
Merged

Implement api,validation,state,device_mismatched: Part II #1094

merged 4 commits into from
Mar 23, 2022

Conversation

haoxli
Copy link
Contributor

@haoxli haoxli commented Mar 21, 2022 •
edited
Loading

  • Remove helpers for getting device mismatched buffer/texture/queryset.
  • Implement remaining device mismatch tests under
    validation/encoding/cmds and validation/encoding/queue.

Issue: #912

Requirements for PR author:

  • All missing test coverage is tracked with "TODO" or .unimplemented().
  • New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)

Requirements for reviewer sign-off:

  • Tests are properly located in the test tree.
  • Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
  • Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
  • Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

@haoxli
Implement api,validation,state,device_mismatched: Part II
2581575 
- Update DeviceHolder to provide multiple devices with same descriptor
  at once, and remove the new DevicePool of mismatched device.
- Remove selectMismatchedDeviceOrSkipTestCase function.
- Implement remaining device mismatch tests under
  validation/encoding/cmds and validation/encoding/queue.

Fixed gpuweb#912
@haoxli haoxli requested a review from kainino0x March 21, 2022 01:00
@github-actions
Copy link

Previews, as seen when this build job started (2581575):
Run tests | View tsdoc

kainino0x
kainino0x reviewed Mar 21, 2022
View reviewed changes
Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice coverage! Can we split the new tests off from the framework changes though?

I'll be out the rest of this week, you'll need to find another reviewer when you're ready.

src/webgpu/util/device_pool.ts Outdated
@@ -234,42 +237,55 @@ function supportsFeature(
type DeviceHolderState = 'free' | 'reserved' | 'acquired';

/**
* Holds a GPUDevice and tracks its state (free/reserved/acquired) and handles device loss.
* Holds GPUDevice with same descriptor and tracks device state (free/reserved/acquired) and handles device loss.
* All the devices must be free/reserved/acquired together.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of getting multiple devices from the device pool was to increase the pool's size so it could have more than 1 default device holder, rather than having multiple devices in each device holder. The device holder is designed to hold just one device (e.g. it has only one lostInfo). Extending it to hold 2 works for these tests, but seems overspecialized for this exact use case, I don't think it can be used for much else.

This new code is overall nicer even though it's weird in a different way, but it also makes every test use 2 devices. That's probably not super costly, but it is surprising and I'd prefer not to.

Possible to split new test coverage and these framework changes? I'd like to land the new tests but I'm wary of the 2-device thing which could have effects on all the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will split the new tests to a separate PR, we can land it first. About getting multiple devices, I have different thought, let's dicuss it in another PR when you get back.

src/webgpu/api/validation/encoding/cmds/clearBuffer.spec.ts Outdated Show resolved Hide resolved
src/webgpu/api/validation/encoding/cmds/compute_pass.spec.ts Outdated Show resolved Hide resolved
src/webgpu/api/validation/encoding/cmds/render/setPipeline.spec.ts Outdated Show resolved Hide resolved
src/webgpu/api/validation/encoding/cmds/setBindGroup.spec.ts Outdated Show resolved Hide resolved
src/webgpu/api/validation/validation_test.ts Outdated Show resolved Hide resolved
src/webgpu/api/validation/queue/submit.spec.ts Outdated Show resolved Hide resolved
@haoxli haoxli force-pushed the 912-device-mismatch-validation branch from dc3aa95 to 9e6db51 Compare March 22, 2022 08:38
@haoxli haoxli force-pushed the 912-device-mismatch-validation branch from 9e6db51 to 108e5f5 Compare March 22, 2022 08:41
@haoxli haoxli requested a review from austinEng March 22, 2022 08:47
@haoxli
Copy link
Contributor Author

haoxli commented Mar 22, 2022

@austinEng Could you help to review the changes? Thanks!

@github-actions
Copy link

Previews, as seen when this build job started (bd38ced):
Run tests | View tsdoc

austinEng
austinEng approved these changes Mar 23, 2022
View reviewed changes
Copy link
Collaborator

@austinEng austinEng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@haoxli haoxli merged commit 64f6b15 into gpuweb:main Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kainino0x kainino0x kainino0x left review comments

@austinEng austinEng austinEng approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@haoxli @kainino0x @austinEng