Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nanobenchmarks #4143

Merged
merged 22 commits into from
Dec 29, 2024
Merged

Nanobenchmarks #4143

merged 22 commits into from
Dec 29, 2024

Conversation

eggrobin
Copy link
Member

No description provided.

nanobenchmarks/main.cpp Outdated Show resolved Hide resolved
using namespace principia::nanobenchmarks::_performance_settings_controller;
using namespace principia::testing_utilities::_statistics;

struct LatencyDistributionTable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this struct to its own file with proper hpp/cpp separation. Also, maybe make it a class?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is all formatting, which is coupled to main, and the structure is tied to the Benchmark function. Let’s untangle that some other day.

nanobenchmarks/main.cpp Outdated Show resolved Hide resolved
nanobenchmarks/main.cpp Show resolved Hide resolved
Benchmark(BenchmarkedFunction f, Logger* logger) {
std::size_t const sample_count = absl::GetFlag(FLAGS_samples);
std::size_t const loop_iterations = absl::GetFlag(FLAGS_loop_iterations);
static std::vector<double>& samples = *new std::vector<double>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use a pointer, not a reference, to avoid depending on lifetime extension.

Copy link
Member Author

@eggrobin eggrobin Dec 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lifetime is obvious : *new, we leak.
Let’s not litter the code with (*) below.

nanobenchmarks/main.cpp Show resolved Hide resolved
nanobenchmarks/main.cpp Outdated Show resolved Hide resolved
nanobenchmarks/main.cpp Outdated Show resolved Hide resolved

FunctionRegistry& FunctionRegistry::singleton_ = *new FunctionRegistry();

std::map<std::string, BenchmarkedFunction, std::less<>> const&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use absl maps everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are ordered, so that the tests show up in alphabetical(ish) order; we don’t use absl ordered maps unless we have a good reason to.

nanobenchmarks/performance_settings_controller.hpp Outdated Show resolved Hide resolved
@@ -162,6 +219,21 @@ std::string_view WindowsPerformanceSettingsController::PerfBoostModeToString(
};
#undef PRINCIPIA_PROCESSOR_PERF_BOOST_MODE_CASE

#define PRINCIPIA_PROCESSOR_PROCESSOR_THROTTLE(value) \
case PROCESSOR_THROTTLE_##value: \
return #value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add blank line after.

return "Unknown";
}
};
#undef PRINCIPIA_PROCESSOR_PERF_BOOST_MODE_CASE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add blank line before.

@pleroy pleroy added the LGTM label Dec 29, 2024
@eggrobin eggrobin merged commit 4e38e35 into mockingbirdnest:master Dec 29, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants