cpu_performance: Improve the default implementation [chromium/src : main]
Nikolaos Papaspyrou (Gerrit)
Nikolaos Papaspyrou voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nikolaos Papaspyrou
| Commit-Queue | +1 |
1 comment
PTAL, thank you!
It would be great if we could land this before the M146 cut.
Related details
- Dave Tapuska
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
I do not love this regular expression at all.
Is there not something we can do with x86 process_type and family/stepping. Those are widely available to be read across platforms.
And on ARM there are certainly similar things to read directly from cpu info.
Adding Mark because this seems like it should be using base directly and we should add functions there that expose the information we want about CPU info if they already do not exist.
Related details
- Mark Mentovai
- Michael Lippautz
- Nikolaos Papaspyrou
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. |
Mark Mentovai (Gerrit)
Mark Mentovai added 2 comments![]( "Open in Gerrit")
I agree with Dave. The list of regexes will be very fragile.
Even if you need to introspect on a per-model basis in order to classify things into tiers, I think it’d be better do do with structured data such as what you might obtain from an OS or hardware interface, or ideally, a wrapper like base/cpu.h.
Here are a few key points that I see missing from the design (https://github.com/WICG/cpu-performance) that might help guide this in productive direction.
1. Are all browsers that implement this expected to map the same CPUs into the same tiers? (I think that “implementation-specific” means “no”, but I’m not positive—it might mean something else, or more, or less. Certainly the tier number is of less utility if the web is expected to implement things like the doc’s example switch block, but each browser assigns its own tiers.)
2. How will newly introduced CPUs be assigned a tier and make their way into whatever logic is implemented (regexes, structured cpuinfo, or otherwise).
3. There will inevitably be bugs where a CPU will be mis-classified into an inappropriate live tier (not tier 0). This might happen as a result of a newly introduced CPU that doesn’t map to the existing logic (especially if regex-oriented as here), or it might happen as a result of bugs introduced as this code is maintained. A CPU isn’t supposed to move between tiers. What happens now?
4. The design is very heavily motivated by a single use case, video calls. CPU performance is only one aspect of a system’s suitability for this application—some tasks crucial to its performance might be offloaded off-CPU, while other aspects of suitability might be hard-gated on other aspects of system design or configuration. I’ll assume that there are other interfaces to get that information, so, fine, we can focus on just the CPU here. There’s still trouble: an assigned CPU tier that focuses too heavily on an application such as video calling might prioritize a CPU that has certain hardware tuning beneficial for audio/video encode/decode despite otherwise being a mediocre performer. That makes the interface’s applicability far less general than the doc purports to provide. In that case, it might not be “navigator.cpuPerformance” at all, but “navigator.cpuPerformanceForVp9EncodeAndDecodeAndNothingMore”.
// Post the task that will update the tier asynchronously, using the more
// accurate (but potentially blocking) implementation.
base::ThreadPool::PostTask(
FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
base::BindOnce([]() {
Tier tier = GetTierFromCpuInfo(base::SysInfo::CPUModelName(),
base::SysInfo::NumberOfProcessors());
g_tier_cached.store(tier);
}));
}Can you call out what’s blocking? As written below, I don‘t see it. It looks like a long chain of regexes.
Slow? Maybe. But blocking?
Related details
- Michael Lippautz
- Nikolaos Papaspyrou
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. |
Nikolaos Papaspyrou (Gerrit)
Nikolaos Papaspyrou added 1 comment![]( "Open in Gerrit")
// Post the task that will update the tier asynchronously, using the more
// accurate (but potentially blocking) implementation.
base::ThreadPool::PostTask(
FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
base::BindOnce([]() {
Tier tier = GetTierFromCpuInfo(base::SysInfo::CPUModelName(),
base::SysInfo::NumberOfProcessors());
g_tier_cached.store(tier);
}));
}Can you call out what’s blocking? As written below, I don‘t see it. It looks like a long chain of regexes.
Slow? Maybe. But blocking?
This was made an async task because `base::SysInfo::CPUModelName()` may be blocking the UI thread, at least on Linux where it reads `/proc/cpuinfo`. PS #1 failed for this reason.
Related details
- Mark Mentovai
- Michael Lippautz
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. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
// Post the task that will update the tier asynchronously, using the more
// accurate (but potentially blocking) implementation.
base::ThreadPool::PostTask(
FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
base::BindOnce([]() {
Tier tier = GetTierFromCpuInfo(base::SysInfo::CPUModelName(),
base::SysInfo::NumberOfProcessors());
g_tier_cached.store(tier);
}));
}Nikolaos PapaspyrouCan you call out what’s blocking? As written below, I don‘t see it. It looks like a long chain of regexes.
Slow? Maybe. But blocking?
This was made an async task because `base::SysInfo::CPUModelName()` may be blocking the UI thread, at least on Linux where it reads `/proc/cpuinfo`. PS #1 failed for this reason.
This was made an async task because `base::SysInfo::CPUModelName()` may be blocking the UI thread, at least on Linux where it reads `/proc/cpuinfo`. PS #1 failed for this reason.
Oh.
/proc reads don’t really block, but going through the filesystem interface, we don’t really have any way to see that.
I see. OK. I’d ask that the comment clarify that /proc/cpuinfo is the whole reason for this.
Related details
- Michael Lippautz
- Nikolaos Papaspyrou
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. |
Nikolaos Papaspyrou (Gerrit)
Nikolaos Papaspyrou added 1 comment![]( "Open in Gerrit")
// Post the task that will update the tier asynchronously, using the more
// accurate (but potentially blocking) implementation.
base::ThreadPool::PostTask(
FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
base::BindOnce([]() {
Tier tier = GetTierFromCpuInfo(base::SysInfo::CPUModelName(),
base::SysInfo::NumberOfProcessors());
g_tier_cached.store(tier);
}));
}Nikolaos PapaspyrouCan you call out what’s blocking? As written below, I don‘t see it. It looks like a long chain of regexes.
Slow? Maybe. But blocking?
Mark MentovaiThis was made an async task because `base::SysInfo::CPUModelName()` may be blocking the UI thread, at least on Linux where it reads `/proc/cpuinfo`. PS #1 failed for this reason.
This was made an async task because `base::SysInfo::CPUModelName()` may be blocking the UI thread, at least on Linux where it reads `/proc/cpuinfo`. PS #1 failed for this reason.
Oh.
/proc reads don’t really block, but going through the filesystem interface, we don’t really have any way to see that.
I see. OK. I’d ask that the comment clarify that /proc/cpuinfo is the whole reason for this.
OK, I will update the comment to make this clear. Thank you!
Related details
- Mark Mentovai
- Michael Lippautz
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. |
Chen Xing (Gerrit)
Chen Xing added 3 comments![]( "Open in Gerrit")
if (Search(cpu_model, "(?i)\\b(AMD|Athlon)\\b")) {As far as I can tell, there's no valid AMD CPU model string that doesn't contain the word "AMD". There are a handful of instances of "Athlon" strings without "AMD" but all of them seem to come from corruptions (or manually modified strings) rather than something that the OS will legitimately return by default.
auto [manufacturer, model] = SplitCpuModel(cpu_model);`SplitCpuModel()` has only been verified to produce an accurate "model" string for Apple/AMD/Intel CPUs. In particular: It produces the wrong result for Qualcomm CPUs and will possibly also return "model strings with garbage text" for other manufacturers.
tests = {{"Intel(R) Core(TM) i7-10700K CPU @ 3.80GHz",Ensure that the unit tests results in 100% code coverage?
Related details
- Mark Mentovai
- Michael Lippautz
- Nikolaos Papaspyrou
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. |
Nikolaos Papaspyrou (Gerrit)
Nikolaos Papaspyrou added 4 comments![]( "Open in Gerrit")
As far as I can tell, there's no valid AMD CPU model string that doesn't contain the word "AMD". There are a handful of instances of "Athlon" strings without "AMD" but all of them seem to come from corruptions (or manually modified strings) rather than something that the OS will legitimately return by default.
I am removing "Athlon" from the regex, thanks for noticing!
// Post the task that will update the tier asynchronously, using the more
// accurate (but potentially blocking) implementation.
base::ThreadPool::PostTask(
FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
base::BindOnce([]() {
Tier tier = GetTierFromCpuInfo(base::SysInfo::CPUModelName(),
base::SysInfo::NumberOfProcessors());
g_tier_cached.store(tier);
}));
}Nikolaos PapaspyrouCan you call out what’s blocking? As written below, I don‘t see it. It looks like a long chain of regexes.
Slow? Maybe. But blocking?
Mark MentovaiThis was made an async task because `base::SysInfo::CPUModelName()` may be blocking the UI thread, at least on Linux where it reads `/proc/cpuinfo`. PS #1 failed for this reason.
Nikolaos PapaspyrouThis was made an async task because `base::SysInfo::CPUModelName()` may be blocking the UI thread, at least on Linux where it reads `/proc/cpuinfo`. PS #1 failed for this reason.
Oh.
/proc reads don’t really block, but going through the filesystem interface, we don’t really have any way to see that.
I see. OK. I’d ask that the comment clarify that /proc/cpuinfo is the whole reason for this.
OK, I will update the comment to make this clear. Thank you!
Done
`SplitCpuModel()` has only been verified to produce an accurate "model" string for Apple/AMD/Intel CPUs. In particular: It produces the wrong result for Qualcomm CPUs and will possibly also return "model strings with garbage text" for other manufacturers.
For manufacturers other than AMD, Apple and Intel, `SplitCpuModel` just performs some basic cleanup:
- sanitizes whitespace
- removes parentheses
- removes special characters
- removes GHz information
- removes filler words (CPU, Mobile, Processor, Silicon, SOC, Technology)
It then returns the manufacturer and the resulting CPU model text. For manufacturers other than AMD, Apple and Intel, this text is not further used. I do not see how it can produce the wrong manufacturer or garbage text.
Let us continue this discussion offline.
Ensure that the unit tests results in 100% code coverage?
I added more tests to increase the 90%.
Thank you for suggesting.
Related details
- Dave Tapuska
- Mark Mentovai
- Michael Lippautz
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. |
Chen Xing (Gerrit)
Chen Xing added 1 comment![]( "Open in Gerrit")
auto [manufacturer, model] = SplitCpuModel(cpu_model);Nikolaos Papaspyrou`SplitCpuModel()` has only been verified to produce an accurate "model" string for Apple/AMD/Intel CPUs. In particular: It produces the wrong result for Qualcomm CPUs and will possibly also return "model strings with garbage text" for other manufacturers.
For manufacturers other than AMD, Apple and Intel, `SplitCpuModel` just performs some basic cleanup:
- sanitizes whitespace
- removes parentheses
- removes special characters
- removes GHz information
- removes filler words (CPU, Mobile, Processor, Silicon, SOC, Technology)
It then returns the manufacturer and the resulting CPU model text. For manufacturers other than AMD, Apple and Intel, this text is not further used. I do not see how it can produce the wrong manufacturer or garbage text.
Let us continue this discussion offline.
The text is currently not used any further but it leaves a tripwire for future developers since `auto [manufacturer, model] = SplitCpuModel(cpu_model);` makes it look as though the function has done the extra care to do a proper split for other manufacturers.
This is for example noticeable in one of the unit tests where `Snapdragon(R) X Elite - X1E78100 - Qualcomm(R) Oryon(TM) CPU` returns Manufacturer::kQualcomm + "Snapdragon X Elite - X1E78100 - Oryon". The latter is almost certainly not the preferred format of the returned model string.
I think the fix is quite simple here. When in doubt, simply return the empty string. E.g. `Snapdragon(R) X Elite - X1E78100 - Qualcomm(R) Oryon(TM) CPU` might as well return Manufacturer::kQualcomm + "" and it will remove that risk completely. :-)
Related details
- Dave Tapuska
- Mark Mentovai
- Michael Lippautz
- Nikolaos Papaspyrou
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. |
Nikolaos Papaspyrou (Gerrit)
Nikolaos Papaspyrou voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nikolaos Papaspyrou
| Commit-Queue | +1 |
1 comment
auto [manufacturer, model] = SplitCpuModel(cpu_model);Nikolaos Papaspyrou`SplitCpuModel()` has only been verified to produce an accurate "model" string for Apple/AMD/Intel CPUs. In particular: It produces the wrong result for Qualcomm CPUs and will possibly also return "model strings with garbage text" for other manufacturers.
Chen XingFor manufacturers other than AMD, Apple and Intel, `SplitCpuModel` just performs some basic cleanup:
- sanitizes whitespace
- removes parentheses
- removes special characters
- removes GHz information
- removes filler words (CPU, Mobile, Processor, Silicon, SOC, Technology)
It then returns the manufacturer and the resulting CPU model text. For manufacturers other than AMD, Apple and Intel, this text is not further used. I do not see how it can produce the wrong manufacturer or garbage text.
Let us continue this discussion offline.
The text is currently not used any further but it leaves a tripwire for future developers since `auto [manufacturer, model] = SplitCpuModel(cpu_model);` makes it look as though the function has done the extra care to do a proper split for other manufacturers.
This is for example noticeable in one of the unit tests where `Snapdragon(R) X Elite - X1E78100 - Qualcomm(R) Oryon(TM) CPU` returns Manufacturer::kQualcomm + "Snapdragon X Elite - X1E78100 - Oryon". The latter is almost certainly not the preferred format of the returned model string.
I think the fix is quite simple here. When in doubt, simply return the empty string. E.g. `Snapdragon(R) X Elite - X1E78100 - Qualcomm(R) Oryon(TM) CPU` might as well return Manufacturer::kQualcomm + "" and it will remove that risk completely. :-)
As it's something used internally and there's a comment warning that the implementation "is experimental and subject to change", I don't see a problem with this. We'll fix the model for these manufacturers when there's reason to do so.
I'm happy to return "" instead, if the reviewers agree.
Related details
- Dave Tapuska
- Mark Mentovai
- Michael Lippautz
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. |