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

Implement Topdown metric support #318

Closed
wants to merge 1 commit into from
Closed

Conversation

cvonelm
Copy link
Member

@cvonelm cvonelm commented Feb 6, 2024

This commit adds support for the topdown metrics (fe-bound, be-bound etc.) as seen in perf

This commit adds support for the topdown metrics (fe-bound, be-bound
etc.) as seen in perf
Copy link
Member

@bmario bmario left a comment

Choose a reason for hiding this comment

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

This whole thing really feels like feature creep. Why do we need special support for topdown? It seems to simply add a bunch of metrics to the trace. Why can't we use (or extend) the existing perf counter group reader?

Comment on lines +99 to +100
//topdown
bool topdown;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//topdown
bool topdown;
bool topdown;

@@ -74,6 +75,8 @@ class ScopeMonitor : public PollMonitor
std::unique_ptr<perf::sample::Writer> sample_writer_;
std::unique_ptr<perf::counter::group::Writer> group_counter_writer_;
std::unique_ptr<perf::counter::userspace::Writer> userspace_counter_writer_;

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

* This file is part of the lo2s software.
* Linux OTF2 sampling
*
* Copyright (c) 2021,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2021,
* Copyright (c) 2024,

* This file is part of the lo2s software.
* Linux OTF2 sampling
*
* Copyright (c) 2018,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2018,
* Copyright (c) 2024,

uint64_t slots;
uint64_t retiring;
uint64_t bad_spec;
uint64_t fe_bound;
Copy link
Member

Choose a reason for hiding this comment

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

fe?

Comment on lines +77 to +78
topdown_writer_ = std::make_unique<perf::counter::topdown::Writer>(scope, parent.trace());
add_fd(topdown_writer_->fd());
Copy link
Member

Choose a reason for hiding this comment

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

clang-format

@@ -344,6 +345,8 @@ void parse_program_options(int argc, const char** argv)
.metavar("MSEC")
.default_value("100");

topdown_options.toggle("topdown", "Enable recording of topdown metrics");
Copy link
Member

Choose a reason for hiding this comment

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

It's a special perf metric. No need for groups of one.

ByExecutionScope(groups_.get_parent(writer_scope))),
otf2::definition::location::location_type::cpu_thread);

comm_locations_group_.add_member(intern_location);
Copy link
Member

Choose a reason for hiding this comment

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

Why adding the location to comm_locations_group_? Without that this function is identical to
metric_writer(MeasurementScope::topdown(scope)).

@@ -170,6 +170,9 @@ set(SOURCE_FILES
src/perf/counter/userspace/reader.cpp

src/perf/counter/userspace/writer.cpp
src/perf/counter/topdown/reader.cpp

Copy link
Member

Choose a reason for hiding this comment

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

random new line?

otf2::common::metric_occurence::async, otf2::common::recorder_kind::abstract);
topdown_metric_class_->add_member(metric_member(
"slots", "Number of UOPS slots", otf2::common::metric_mode::absolute_point,
otf2::common::type::int64, "slots"));
Copy link
Member

Choose a reason for hiding this comment

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

the second "slots" is the unit of the metric. uops seems to be the obvious choice.

@cvonelm cvonelm closed this Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants