CPUDetect: improve win/arm64 support #10876
Conversation
|
|
||
| return sum; | ||
| return JoinStrings(sum, ","); |
There was a problem hiding this comment.
Might be better to join by ", " to keep the formatting consistent with older builds.
There was a problem hiding this comment.
Yea I have purposefully removed that because leading space makes it a tiny bit harder to parse the output (not that we do, but it could be nice to accumulate stats on analytics for example, to just do [feature_map[x] += 1 for x in str.split(',')] or similar).
There was a problem hiding this comment.
Wouldn't this split the CPU type stats on analytics between old formatting and new formatting? Not sure if that's a concern.
There was a problem hiding this comment.
Yea, and I've changed formatting of first 2 elements as well: where it used to report something like
AMD Ryzen Threadripper 3990X 64-Core Processor (AuthenticAMD)
it will now report
AMD Ryzen Threadripper 3990X 64-Core Processor,17:31:0
on windows/arm64 it will report something like
Microsoft SQ1 @ 3.0 GHz,51:D:1111:805:E
This fixes:
- removes trailing space in the model name string (we were not following the x64 specification and stripping padding spaces)
- reports the family/model/stepping information instead of the brand name, because FMS actually gives us useful information instead of the brand which is already in the model name (and the brand can be determined from the FMS ). FMS is also easily parsed and may be interesting to query server-side.
- on windows/arm64 nothing was reported before for model/brand. now we'll get model info.
Breaking compat with the old format doesn't seem like a huge deal to me. In the future we might change it again (e.g. on arm64 - maybe just the raw MIDR would be better, similar to FMS, because that should at least be obtainable on windows and linux).
@delroth any comment?
edit: actually to make the parsing even better, i'm going to ensure the strings reported from the processor don't include any , and always include those strings (even if empty). So it will be real csv
edit2: done
|
Changes for arm64 Macs LGTM. |
|
Also, I'm noticing that linux appears to put all the MIDR fields in /proc/cpuinfo as the CPU implementer/architecture/variant/part/revision fields. Parsing that might be a more sane fallback than triggering MIDR_EL1 traping/emulation on the current core |
|
personally i think parsing text is insane :D (case in point: "Hardware" line) anyway, it will only invoke the instruction if it's advertised as supported from hwcaps. |
|
Yeah, I was thinking more from a stats perspective (because that's what I'm slightly more interested in). The more sane part is because you can collect MIDR for the all cores, not just the current one. |
|
considering we're uploading a text string that (currently) we just view in grafana, it seems troublesome to start reporting a lot more text. we could dedup client side first, but it still results in the <model,cpu_id> prefix of the string becoming variable sized -> harder to parse. |
read brand_string on macos/arm64 remove unused flags report family/model info instead of vendor name
|
FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:
automated-fifoci-reporter |
Notably FMA4 is also not actually used; the emitter got support but the related ops are never used. I've only removed flags which are never referenced, so same may apply to other flags as well.