[webgpu][blink] Adds the capability to enable multithread GPU replies. [chromium/src : main]
Loko Kung (Gerrit)
Loko Kung voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Daniel Cheng
- Kai Ninomiya
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kai Ninomiya (Gerrit)
Kai Ninomiya voted and added 4 comments![]( "Open in Gerrit")
Votes added by Kai Ninomiya
| Code-Review | +1 |
4 comments
base::StrongAlias<class WebGPUHandleGPUReplyOnIOThreadTag, bool>;I had no idea base had this (and IdType), this is good to know about! chrome graphics code needs a LOT of these...
wgpu::WaitStatus WaitAny(It's a bit odd to put this publicly on DawnObjectBase because it kind of sounds like it matters which object you call WaitAny on. Could make it `protected` but how about just having (public) `DawnObjectBase::GetInstance()` and make callers call through that? (This would get rid of the default timeout but it doesn't seem to be used.)
// Function pointer type must be a raw pointer.nit: Could the struct use `raw_ptr` and just make the caller use `.get()`?
struct WGPUOnceCallbackInfo {nit: make this non-copyable
Related details
- Daniel Cheng
- Loko Kung
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
Sorry I've been a bit late here. +dom@ for a first pass on public
Related details
- Dominic Farolino
- Loko Kung
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
base::StrongAlias<class WebGPUHandleGPUReplyOnIOThreadTag, bool>;Why is this better than just a boolean? Is there any special risk of it being confused for, or assigned to, something else by mistake? I guess I'm trying to ask if this is better that just annotating in code comments `/*arg_name=*/false`. StrongAlias seems most useful when there are multiple differently-named types with the same backing type in close proximity.
Related details
- Loko Kung
Kai Ninomiya (Gerrit)
Kai Ninomiya added 1 comment![]( "Open in Gerrit")
base::StrongAlias<class WebGPUHandleGPUReplyOnIOThreadTag, bool>;Why is this better than just a boolean? Is there any special risk of it being confused for, or assigned to, something else by mistake? I guess I'm trying to ask if this is better that just annotating in code comments `/*arg_name=*/false`. StrongAlias seems most useful when there are multiple differently-named types with the same backing type in close proximity.
I personally think it's good because it's passed through many layers of the stack. I don't see neighboring bool arguments in any of the functions where it's used but there are still opportunities to mix it up.
Does Chromium have a lint that checks `/*arg_name=*/` comments are correct? I think it does; if so then bool is totally fine. (We usually work in Dawn and we don't often use that pattern.)
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Code-Review | +1 |
2 comments
lgtm; off to dcheng@
base::StrongAlias<class WebGPUHandleGPUReplyOnIOThreadTag, bool>;Kai NinomiyaWhy is this better than just a boolean? Is there any special risk of it being confused for, or assigned to, something else by mistake? I guess I'm trying to ask if this is better that just annotating in code comments `/*arg_name=*/false`. StrongAlias seems most useful when there are multiple differently-named types with the same backing type in close proximity.
I personally think it's good because it's passed through many layers of the stack. I don't see neighboring bool arguments in any of the functions where it's used but there are still opportunities to mix it up.
Does Chromium have a lint that checks `/*arg_name=*/` comments are correct? I think it does; if so then bool is totally fine. (We usually work in Dawn and we don't often use that pattern.)
I don't think Chromium has such a lint check, at least not that I've experienced recently. I don't feel strongly against this, though I feel like it's slightly overkill.
Kai Ninomiya (Gerrit)
Kai Ninomiya added 1 comment![]( "Open in Gerrit")
base::StrongAlias<class WebGPUHandleGPUReplyOnIOThreadTag, bool>;Kai NinomiyaWhy is this better than just a boolean? Is there any special risk of it being confused for, or assigned to, something else by mistake? I guess I'm trying to ask if this is better that just annotating in code comments `/*arg_name=*/false`. StrongAlias seems most useful when there are multiple differently-named types with the same backing type in close proximity.
Dominic FarolinoI personally think it's good because it's passed through many layers of the stack. I don't see neighboring bool arguments in any of the functions where it's used but there are still opportunities to mix it up.
Does Chromium have a lint that checks `/*arg_name=*/` comments are correct? I think it does; if so then bool is totally fine. (We usually work in Dawn and we don't often use that pattern.)
I don't think Chromium has such a lint check, at least not that I've experienced recently. I don't feel strongly against this, though I feel like it's slightly overkill.
Tested and unfortunately there isn't an _enforced_ lint, but there _is_ a clang-tidy warning that will at least alert about this on Gerrit! https://chromium-review.googlesource.com/c/chromium/src/+/7247461/2/services/viz/public/cpp/gpu/gpu.cc
Regardless I'm OK either way but I'll leave this open for Loko to look at when he's back.
Daniel Cheng (Gerrit)
Daniel Cheng added 6 comments![]( "Open in Gerrit")
base::StrongAlias<class WebGPUHandleGPUReplyOnIOThreadTag, bool>;Kai NinomiyaWhy is this better than just a boolean? Is there any special risk of it being confused for, or assigned to, something else by mistake? I guess I'm trying to ask if this is better that just annotating in code comments `/*arg_name=*/false`. StrongAlias seems most useful when there are multiple differently-named types with the same backing type in close proximity.
Dominic FarolinoI personally think it's good because it's passed through many layers of the stack. I don't see neighboring bool arguments in any of the functions where it's used but there are still opportunities to mix it up.
Does Chromium have a lint that checks `/*arg_name=*/` comments are correct? I think it does; if so then bool is totally fine. (We usually work in Dawn and we don't often use that pattern.)
Kai NinomiyaI don't think Chromium has such a lint check, at least not that I've experienced recently. I don't feel strongly against this, though I feel like it's slightly overkill.
Tested and unfortunately there isn't an _enforced_ lint, but there _is_ a clang-tidy warning that will at least alert about this on Gerrit! https://chromium-review.googlesource.com/c/chromium/src/+/7247461/2/services/viz/public/cpp/gpu/gpu.cc
Regardless I'm OK either way but I'll leave this open for Loko to look at when he's back.
I'd suggest just using an enum class here. It'll be almost as compact, and it's part of the official Blink guidelines.
Though... I didn't realize that the guidelines changed to allow StrongAlias here too: https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/blink-c++.md#prefer-enums-or-strongaliases-to-bare-bools-for-function-parameters
My personal feeling is enums are still better for readability but I'm not going to push strongly on this.
uint64_t timeout_ns = std::numeric_limits<uint64_t>::max());Does it add a lot of plumbing to use `base::TimeDelta` and only convert to nanoseconds where needed to interact with dawn?
&dawn_options, callback_info.mode, callback_info.callback,
callback_info.userdata);Seeing this makes me wonder if we should just be passing `callback_info` as a bundle here.
// Set the uncaptured error callback first because it's ownership will beNit: its
// whether we are being handled off the main thread, if so, we proxy to theNit: handed?
// Function pointer type must be a raw pointer.nit: Could the struct use `raw_ptr` and just make the caller use `.get()`?
Function pointers are supposed to be excluded from this check by default.
Are we using this to hold non-function-pointer things, or is the plugin getting confused?
Kai Ninomiya (Gerrit)
Kai Ninomiya added 3 comments![]( "Open in Gerrit")
base::StrongAlias<class WebGPUHandleGPUReplyOnIOThreadTag, bool>;Kai NinomiyaWhy is this better than just a boolean? Is there any special risk of it being confused for, or assigned to, something else by mistake? I guess I'm trying to ask if this is better that just annotating in code comments `/*arg_name=*/false`. StrongAlias seems most useful when there are multiple differently-named types with the same backing type in close proximity.
Dominic FarolinoI personally think it's good because it's passed through many layers of the stack. I don't see neighboring bool arguments in any of the functions where it's used but there are still opportunities to mix it up.
Does Chromium have a lint that checks `/*arg_name=*/` comments are correct? I think it does; if so then bool is totally fine. (We usually work in Dawn and we don't often use that pattern.)
Kai NinomiyaI don't think Chromium has such a lint check, at least not that I've experienced recently. I don't feel strongly against this, though I feel like it's slightly overkill.
Daniel ChengTested and unfortunately there isn't an _enforced_ lint, but there _is_ a clang-tidy warning that will at least alert about this on Gerrit! https://chromium-review.googlesource.com/c/chromium/src/+/7247461/2/services/viz/public/cpp/gpu/gpu.cc
Regardless I'm OK either way but I'll leave this open for Loko to look at when he's back.
I'd suggest just using an enum class here. It'll be almost as compact, and it's part of the official Blink guidelines.
Though... I didn't realize that the guidelines changed to allow StrongAlias here too: https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/blink-c++.md#prefer-enums-or-strongaliases-to-bare-bools-for-function-parameters
My personal feeling is enums are still better for readability but I'm not going to push strongly on this.
Good point, an enum with "IOThread" and "MainThread" would make sense.
&dawn_options, callback_info.mode, callback_info.callback,
callback_info.userdata);Seeing this makes me wonder if we should just be passing `callback_info` as a bundle here.
This would require a Dawn change, but agree it's probably something we should do later. Dawn's C API _does_ now take it as a bundle, we just haven't changed the C++ API to match.
// whether we are being handled off the main thread, if so, we proxy to theNit: handed?
"handled" is correct since it's talking about where gpu->renderer messages are handled. Maybe wording of this sentence could be a bit clearer
Loko Kung (Gerrit)
Loko Kung added 8 comments![]( "Open in Gerrit")
base::StrongAlias<class WebGPUHandleGPUReplyOnIOThreadTag, bool>;Kai NinomiyaWhy is this better than just a boolean? Is there any special risk of it being confused for, or assigned to, something else by mistake? I guess I'm trying to ask if this is better that just annotating in code comments `/*arg_name=*/false`. StrongAlias seems most useful when there are multiple differently-named types with the same backing type in close proximity.
Dominic FarolinoI personally think it's good because it's passed through many layers of the stack. I don't see neighboring bool arguments in any of the functions where it's used but there are still opportunities to mix it up.
Does Chromium have a lint that checks `/*arg_name=*/` comments are correct? I think it does; if so then bool is totally fine. (We usually work in Dawn and we don't often use that pattern.)
Kai NinomiyaI don't think Chromium has such a lint check, at least not that I've experienced recently. I don't feel strongly against this, though I feel like it's slightly overkill.
Daniel ChengTested and unfortunately there isn't an _enforced_ lint, but there _is_ a clang-tidy warning that will at least alert about this on Gerrit! https://chromium-review.googlesource.com/c/chromium/src/+/7247461/2/services/viz/public/cpp/gpu/gpu.cc
Regardless I'm OK either way but I'll leave this open for Loko to look at when he's back.
Kai NinomiyaI'd suggest just using an enum class here. It'll be almost as compact, and it's part of the official Blink guidelines.
Though... I didn't realize that the guidelines changed to allow StrongAlias here too: https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/blink-c++.md#prefer-enums-or-strongaliases-to-bare-bools-for-function-parameters
My personal feeling is enums are still better for readability but I'm not going to push strongly on this.
Good point, an enum with "IOThread" and "MainThread" would make sense.
Done using enums.
uint64_t timeout_ns = std::numeric_limits<uint64_t>::max());Does it add a lot of plumbing to use `base::TimeDelta` and only convert to nanoseconds where needed to interact with dawn?
No longer applicable.
It's a bit odd to put this publicly on DawnObjectBase because it kind of sounds like it matters which object you call WaitAny on. Could make it `protected` but how about just having (public) `DawnObjectBase::GetInstance()` and make callers call through that? (This would get rid of the default timeout but it doesn't seem to be used.)
Done
&dawn_options, callback_info.mode, callback_info.callback,
callback_info.userdata);Kai NinomiyaSeeing this makes me wonder if we should just be passing `callback_info` as a bundle here.
This would require a Dawn change, but agree it's probably something we should do later. Dawn's C API _does_ now take it as a bundle, we just haven't changed the C++ API to match.
Acknowledged
// Set the uncaptured error callback first because it's ownership will beLoko KungNit: its
Done
// whether we are being handled off the main thread, if so, we proxy to theKai NinomiyaNit: handed?
"handled" is correct since it's talking about where gpu->renderer messages are handled. Maybe wording of this sentence could be a bit clearer
Acknowledged
Daniel Chengnit: Could the struct use `raw_ptr` and just make the caller use `.get()`?
Function pointers are supposed to be excluded from this check by default.
Are we using this to hold non-function-pointer things, or is the plugin getting confused?
No longer applicable.
struct WGPUOnceCallbackInfo {Loko Kungnit: make this non-copyable
No longer applicable.
Related details
- Daniel Cheng
- Dominic Farolino
- Kai Ninomiya
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kai Ninomiya (Gerrit)
Kai Ninomiya voted Code-Review+1![]( "Open in Gerrit")
- Daniel Cheng
- Dominic Farolino
- Loko Kung
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 5 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
5 comments
LGTM w/nits addressed–apologies for the delay.
Addressing the nits may wipe the CR+1 bit, sorry :(
"+base/types/strong_alias.h",Probably not needed anymore
#include "base/types/strong_alias.h"Same here; I think this can be removed.
String message) {Nit: pass by const ref here since we're not passing ownership. While `String` is cheap to copy as a ref-counted wrapper around the actual `StringImpl`, not bumping refcount at all is free (...ish, of course there's a pointer indirection :)
(We could, but none of the constructors take `message` by value–and even if they did, we still need to reference `message` after calling the constructor for the error)
Related details
- Dominic Farolino
- Loko Kung
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |