[tracing] Fold CPUFreqMonitor into SystemMetricsSampler [chromium/src : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 1 comment![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Commit-Queue | +1 |
1 comment
Related details
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason voted and added 41 comments![]( "Open in Gerrit")
Votes added by Joe Mason
| Code-Review | +1 |
41 comments
Nit: please fix the clang-tidy warnings.
Lots of nits, but nothing blocking.
std::vector<std::pair<unsigned int, base::ScopedFD>> file_descriptors_;Nit: please #include <utility> for IWYU.
virtual std::string GetScalingCurFreqPathString(unsigned int cpu_id) const;Nit: please #include <string> for IWYU.
virtual unsigned int GetKernelMaxCPUs() const;Nit: call this `GetKernelMaxCPUId()` so that it's clear that it's returning an ID, not a count. (Otherwise I'd expect it to return 8 on an 8-core CPU.)
virtual std::vector<unsigned int> GetCPUIds() const;Optional: should wrap this in `base::IdType` so that it can't be confused with a different id. Can skip this if it would be a lot of work to convert back and forth. (Or switch to IdType in a followup.)
#include "base/memory/scoped_refptr.h"Nit: unused.
#include "base/base_export.h"Nit: not used. (And should be COMPONENT_EXPORT in components/ if it's needed. But I don't think it is.)
#include "base/memory/scoped_refptr.h"Nit: unused
std::string str;Nit: please #include <string> for IWYU.
base::FilePath("/sys/devices/system/cpu/kernel_max"), &str)) {Nit: please #include "base/files/file_path.h" for IWYU.
return base::StringPrintf(Nit: prefer `absl::StrFormat`.
std::vector<unsigned int> CPUFreqMonitor::Delegate::GetCPUIds() const {Nit: please sort these function definitions in the same order as the header. (So this one should be first.)
base::FixedArray<bool> cpus_to_monitor(kernel_max_cpu + 1, true);TIL about `FixedArray`.
: delegate_(std::move(delegate)) {Nit: please #include <utility> for IWYU.
for (auto& id_fd : file_descriptors_) {Optional nit: this could be `const auto& [id, fd] : file_descriptors_` (and then use `lseek(fd.get(), ...)`, etc)
lseek(fd, 0L, SEEK_SET);Nit: please #include <unistd.h> for IWYU.
std::string_view content(data, bytes_read);Nit: please #include <string_view> for IWYU.
std::string_view content(data, bytes_read);Thanks for fixing the UNSAFE_TODO from the original!
#include "base/task/single_thread_task_runner.h"Nit: unused.
TestDelegate(const std::string& temp_dir_path)Nit: please #include <string> for IWYU.
std::vector<unsigned int> GetCPUIds() const override {Nit: please #include <vector> for IWYU.
return base::StringPrintf("%s/scaling_cur_freq%d", temp_dir_path_.c_str(),Nit: prefer absl::StrFormat.
unsigned int GetKernelMaxCPUs() const override { return kernel_max_cpu_; }Nit: please put these in the same order as the header (so this should come right after GetCPUIds.)
temp_dir_ = std::make_unique<base::ScopedTempDir>();Nit: please #include <memory> for IWYU.
const std::vector<std::pair<unsigned int, unsigned int>>& frequencies) {Nit: please #include <utility> for IWYU.
for (auto& pair : frequencies) {Nit: prefer `const auto& [var1, var2]`. I guess that's `id, freq`? But hard to tell without var names.
std::string str_freq = base::StringPrintf("%d\n", pair.second);Nit: prefer absl::StrFormat.
base::WriteFile(base::FilePath(file_path), str_freq);Nit: please #include "base/files/file_path.h" for IWYU.
void InitBasicCPUInfo() {Nit: this whole method is unused.
std::string file_path = base::StringPrintf(Nit: prefer absl::StrFormat.
write(fd, str_freq.c_str(), str_freq.length());Nit: please #include <unistd.h> for IWYU.
You might want to ASSERT that the result of write is str_freq.length(), so that if the test fails due to a write error the cause is obvious.
// Sample() will end early.Nit: this comment is obsolete since Start() and Sample() don't exist anymore.
EXPECT_EQ(cpu_ids.size(), 2U);Nit: Should be ASSERT_EQ so that the test will fail cleanly and not crash if the result size is wrong.
std::vector<unsigned int> cpu_ids = delegate()->GetCPUIds();Nit: it took me a minute to realize why this test is valid. It's easy to assume it's falling back to the production impls of all the delegate methods, not just GetCPUIds().
It could use a comment that `GetCPUIds` reads from the overridden `GetRelatedCPUsPathString()` from the test delegate, but the test hasn't written anything at that string.
EXPECT_EQ(cpu_ids.size(), 1U);Nit: should be ASSERT_EQ.
#if BUILDFLAG(IS_ANDROID)Nit: please #include "build/build_config.h" for IWYU.
Related details
- Etienne Pierre-Doray
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 39 comments![]( "Open in Gerrit")
+dtapuska@ for content/browser/
std::vector<std::pair<unsigned int, base::ScopedFD>> file_descriptors_;Nit: please #include <utility> for IWYU.
Done
virtual std::string GetScalingCurFreqPathString(unsigned int cpu_id) const;Nit: please #include <string> for IWYU.
Done
Nit: call this `GetKernelMaxCPUId()` so that it's clear that it's returning an ID, not a count. (Otherwise I'd expect it to return 8 on an 8-core CPU.)
Done
Optional: should wrap this in `base::IdType` so that it can't be confused with a different id. Can skip this if it would be a lot of work to convert back and forth. (Or switch to IdType in a followup.)
Done
#include "base/memory/scoped_refptr.h"Etienne Pierre-DorayNit: unused.
Done
Nit: not used. (And should be COMPONENT_EXPORT in components/ if it's needed. But I don't think it is.)
Done
#include "base/compiler_specific.h"Etienne Pierre-DorayNit: unused?
Done
#include "base/functional/bind.h"Etienne Pierre-DorayNit: unused
Done
#include "base/memory/scoped_refptr.h"Etienne Pierre-DorayNit: unused
Done
#include "base/no_destructor.h"Etienne Pierre-DorayNit: unused
Done
Nit: please #include <string> for IWYU.
Done
base::FilePath("/sys/devices/system/cpu/kernel_max"), &str)) {Nit: please #include "base/files/file_path.h" for IWYU.
Done
Nit: prefer `absl::StrFormat`.
Done
std::vector<unsigned int> CPUFreqMonitor::Delegate::GetCPUIds() const {Nit: please sort these function definitions in the same order as the header. (So this one should be first.)
Done
Nit: please #include <utility> for IWYU.
Done
Optional nit: this could be `const auto& [id, fd] : file_descriptors_` (and then use `lseek(fd.get(), ...)`, etc)
Done
Nit: please #include <unistd.h> for IWYU.
Done
std::string_view content(data, bytes_read);Etienne Pierre-DorayNit: please #include <string_view> for IWYU.
Done
#include "base/task/single_thread_task_runner.h"Etienne Pierre-DorayNit: unused.
Done
#include "base/time/time.h"Etienne Pierre-DorayNit: unused.
Done
Nit: please #include <string> for IWYU.
Done
Nit: please #include <vector> for IWYU.
Done
return base::StringPrintf("%s/scaling_cur_freq%d", temp_dir_path_.c_str(),Etienne Pierre-DorayNit: prefer absl::StrFormat.
Done
unsigned int GetKernelMaxCPUs() const override { return kernel_max_cpu_; }Nit: please put these in the same order as the header (so this should come right after GetCPUIds.)
Done
Nit: please #include <memory> for IWYU.
Done
const std::vector<std::pair<unsigned int, unsigned int>>& frequencies) {Nit: please #include <utility> for IWYU.
Done
Nit: prefer `const auto& [var1, var2]`. I guess that's `id, freq`? But hard to tell without var names.
Done
std::string str_freq = base::StringPrintf("%d\n", pair.second);Etienne Pierre-DorayNit: prefer absl::StrFormat.
Done
Nit: please #include "base/files/file_path.h" for IWYU.
Done
Nit: this whole method is unused.
Done
std::string file_path = base::StringPrintf(Etienne Pierre-DorayNit: prefer absl::StrFormat.
Done
Nit: please #include <unistd.h> for IWYU.
You might want to ASSERT that the result of write is str_freq.length(), so that if the test fails due to a write error the cause is obvious.
Done
Nit: this comment is obsolete since Start() and Sample() don't exist anymore.
Done
Nit: Should be ASSERT_EQ so that the test will fail cleanly and not crash if the result size is wrong.
Done
std::vector<unsigned int> cpu_ids = delegate()->GetCPUIds();Nit: it took me a minute to realize why this test is valid. It's easy to assume it's falling back to the production impls of all the delegate methods, not just GetCPUIds().
It could use a comment that `GetCPUIds` reads from the overridden `GetRelatedCPUsPathString()` from the test delegate, but the test hasn't written anything at that string.
Done
EXPECT_EQ(cpu_ids.size(), 1U);Etienne Pierre-DorayNit: should be ASSERT_EQ.
Done
Nit: please #include "build/build_config.h" for IWYU.
- Dave Tapuska
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. |
Dave Tapuska (Gerrit)
Dave Tapuska voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dave Tapuska
| Code-Review | +1 |
1 comment
- Etienne Pierre-Doray
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
Lots of nits, but nothing blocking.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
16 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: components/system_cpu/cpu_freq_android.cc
Insertions: 2, Deletions: 2.
@@ -54,13 +54,13 @@
}
}
}
- result.push_back(CpuId(i));
+ result.emplace_back(i);
}
// If none of the files were readable, we assume CPU0 exists and fall back to
// using that.
if (result.size() == 0) {
- result.push_back(CpuId(0));
+ result.emplace_back(0);
}
return result;
}
```
```
The name of the file: components/system_cpu/cpu_freq_android_unittest.cc
Insertions: 1, Deletions: 1.
@@ -70,7 +70,7 @@
class CPUFreqMonitorTest : public testing::Test {
public:
- CPUFreqMonitorTest() {}
+ CPUFreqMonitorTest() = default;
void SetUp() override {
temp_dir_ = std::make_unique<base::ScopedTempDir>();
```
Change information
[tracing] Fold CPUFreqMonitor into SystemMetricsSampler
A lot of complexity in CPUFreqMonitor come from integration with
tracing. This CL extracts the code to sample the frequency, move
it to components/system_cpu (where similar classes live), and
reuses it in existing SystemMetricsSampler which already handles
the integration for such things.
- M base/BUILD.gn
- D base/trace_event/cpufreq_monitor_android.cc
- D base/trace_event/cpufreq_monitor_android.h
- D base/trace_event/cpufreq_monitor_android_unittest.cc
- M components/system_cpu/BUILD.gn
- A components/system_cpu/cpu_freq_android.cc
- A components/system_cpu/cpu_freq_android.h
- A components/system_cpu/cpu_freq_android_unittest.cc
- M content/browser/browser_main_loop.cc
- M services/tracing/public/cpp/system_metrics_sampler.cc
- M services/tracing/public/cpp/system_metrics_sampler.h
Code-Review: +1 by Dave Tapuska, +1 by Joe Mason
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |