-
Notifications
You must be signed in to change notification settings - Fork 191
Refactor Synchronicity, Provide Async API #77
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.
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
Conversation
|
Last bit on this PR. I added an optional param with this refactor to set a offset for the toCPU buffer read. This is tested in the new test/test_gpu.cpp. I think the toCPU with copydata is now redundant. Will leave it alone if you prefer to keep it. |
| // kernel invocations | ||
| std::promise<void> *promise; | ||
| // kernel invocations | ||
| std::shared_ptr<std::promise<void>> promise; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent with the use of raw pointers is to signal non-owning-ness. Would prefer that as a default unless there's a specific rationale for shared ownership.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was running into an issue with premature deletion in the async callback chain and this ensured it remained valid through the multiple callbacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see ... with that behavior, I think there's an underlying lifetime issue that sharing ownership with CallbackData is masking. Specifically, the application should fully control the lifetime of the promise.
This is by design so that we don't get in a situation where our library is opinionated in a way that gets in the way of the application. We want control over lifetimes to be explicit and controlled by the user, rather than implicit through the lifetime of CallbackData.
If the application has its own mechanism for automatic lifetime management through shared pointers, that's okay (eg there could be some application-level struct that has a shared_ptrs of the promise and the CallbackData struct), but lifetime management of the promise (or any resource) shouldn't be baked into this library through shared ownership unless there's absolutely no other way to make it work.
In a similar spirit, the reason there's *Pool types in Context is because in the cases where the library is responsible for resource allocation and lifetimes, we try to make the management explicit through resource pools rather than implicit through object scope. For promises, it feels to me like the right place to manage those lifetimes is in the application rather than in the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pointer is passed into Dawn. I do not see how the user can be expected to manage the entire lifecycle given this? I tried to change it back to a raw pointer but the pointer becomes non-existent sometime during the processEvents cycle from Dawn. With a shared pointer all of the tests for test_gpu.cpp pass, with a raw pointer a memory exception is hit for wait on line 846, trying to get the future.
test_gpu.exe!std::future<void>::get() Line 911 C++
> test_gpu.exe!gpu::wait<void>(gpu::Context & ctx, std::future<void> & f) Line 846 C++
test_gpu.exe!stressTestToCPU() Line 240 C++| * std::vector<dawn::native::Adapter> adapters = getAdapters(ctx); | ||
| * @endcode | ||
| */ | ||
| inline std::vector<dawn::native::Adapter> getAdapters(Context &ctx) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would rather not have too many 1-line wrapper functions as it adds indirection to what is actually happening. Any reason not to unpack the underlying dawn API calls from the callsites for getAdapters and listAdapters? Do these need to be exposed externally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point these should be exposed to non-dawn builds as well when a way to enumerate adapters exists in the webgpu standard. Before this, there was not a way to see what adapters were actually available. If a user is going to change the adapter used they need to be able to get the full list and from a frontend perspective, output what is available.
Uh oh!
There was an error while loading. Please reload this page.