[d3d12] add support for optional adapter telemetry #8576
Conversation
jimblandy
left a comment
jimblandy
left a comment
There was a problem hiding this comment.
This looks fine to me, but if @cwfitzgerald has time to look at it, it'd be great to get his opinion too.
There was a problem hiding this comment.
No particularly deep qualms with the code - I do agree we should probably put the logic of this in its own module. Also could help to add a bit of documentation about what "telemetry" means to that module as I could see this PR becoming an news article if we're not careful.
More concretely, I'm worried about the telemetry being completely schemaless. It would be nice to at least have a definition somewhere about how these telemetry should be laid out. Especially when we don't have any tests to make sure things work as expected, keeping the formatting in a single place as opposed to spread out all over the place would help these stay consistent.
The telemetry here is quite opinionated since it's meant to be used with Firefox's infrastructure, I tried coming up with something more generic but since I didn't know how other telemetry infrastructure would integrate I didn't make any progress on that. The plan in my mind was that if others are interested in this area, we could try to generalize it. Including the formatting wasn't great, I ended up removing it. It can now be part of the Firefox bindings. |
| #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] | ||
| pub enum FeatureLevel { | ||
| _11_0, | ||
| _11_1, | ||
| _12_0, | ||
| _12_1, | ||
| _12_2, | ||
| } | ||
|
|
||
| #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] | ||
| pub enum ShaderModel { | ||
| _5_1, | ||
| _6_0, | ||
| _6_1, | ||
| _6_2, | ||
| _6_3, | ||
| _6_4, | ||
| _6_5, | ||
| _6_6, | ||
| _6_7, | ||
| _6_8, | ||
| _6_9, | ||
| } |
There was a problem hiding this comment.
Maybe FL11_0 and SM5_1 for identifiers
There was a problem hiding this comment.
I'm not really a fan of the duplication FeatureLevel::FL11_0, ShaderModel::SM5_1. We seem to have used V as a prefix for naga's ShaderModel (i.e. ShaderModel::V5_1) but I don't think it works well for FeatureLevel. Do you have other suggestions?
There was a problem hiding this comment.
Not a lot of good ones, we're destined to have some redundancy as we can't start an enum with a number which is what we really want. I mainly just really dislike the look of these identifiers :) They look like they're going to stab me
There was a problem hiding this comment.
I won't be able to attend tomorrow's maintainers meeting but I'll add this to the agenda if you guys want to bikeshed solutions. I for one am ok with the _ as a prefix; I've also seen it used in some other rust projects.
There was a problem hiding this comment.
Well lets land this then debate it later.
There was a problem hiding this comment.
My only concern with a leading _ on the identifiers is that that's conventionally a “please ignore this when linting for dead code” in Rust. I don't think there's any actual difference in behavior with tooling like rustc diagnostics or clippy for enum variants, though.
There was a problem hiding this comment.
Leading _ behaves almost exactly like a #[allow(dead_code)] attribute, including suppressing transitive dead code, and that does apply to individual enum variants — but I can’t think of any way that affects a pub enum which will never get dead code warnings.
Personally, I would go with a prefix V here, since this is a sort of version number and people will recognize V<number> as not meaning very much beyond the number.
There was a problem hiding this comment.
Ah, right, checking if the variant is used at all is still subject to an underscore prefix. I agree, V sounds like a reasonable prefix, then.
5 participants
Connections
Bug 1980057
Description
Adds support for optional adapter telemetry to be integrated in the Firefox bindings.
Testing
I've tested this manually with Firefox's telemetry infrastructure.
Squash or Rebase?
n/a