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.

Sign up for GitHub

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

Jump to bottom

Code refactor #408

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Code refactor #408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
651a80d
Using '= default', remove unused vars, code simplify
GermanAizek Jun 3, 2022
f7de464
Vars renamed like others, using std::move() and str to char optimize
GermanAizek Jun 3, 2022
b0662ef
Added const reference params
GermanAizek Jun 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions src/cpucounters.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4046,7 +4046,7 @@ void PCM::cleanupRDT(const bool silent)
if (!silent) std::cerr << " Freeing up all RMIDs\n";
}

void PCM::setOutput(const std::string filename, const bool cerrToo)
void PCM::setOutput(const std::string & filename, const bool cerrToo)
{
outfile = new std::ofstream(filename.c_str());
backup_ofile = std::cout.rdbuf();
Expand Down Expand Up @@ -4700,10 +4700,11 @@ PCM::ErrorCode PCM::program(const RawPMUConfigs& curPMUConfigs_, const bool sile
}
if (globalRegPos < corePMUConfig.programmable.size())
{
conf.OffcoreResponseMsrValue[0] = corePMUConfig.programmable[globalRegPos].first[OCR0Pos];
conf.OffcoreResponseMsrValue[1] = corePMUConfig.programmable[globalRegPos].first[OCR1Pos];
conf.LoadLatencyMsrValue = corePMUConfig.programmable[globalRegPos].first[LoadLatencyPos];
conf.FrontendMsrValue = corePMUConfig.programmable[globalRegPos].first[FrontendPos];
const auto& cfgProg = corePMUConfig.programmable[globalRegPos];
conf.OffcoreResponseMsrValue[0] = cfgProg.first[OCR0Pos];
conf.OffcoreResponseMsrValue[1] = cfgProg.first[OCR1Pos];
conf.LoadLatencyMsrValue = cfgProg.first[LoadLatencyPos];
conf.FrontendMsrValue = cfgProg.first[FrontendPos];
}
conf.nGPCounters = (uint32)c;
conf.gpCounterCfg = regs;
Expand Down Expand Up @@ -5696,7 +5697,7 @@ void initSocket2Bus(std::vector<std::pair<uint32, uint32> > & socket2bus, uint32
if(DEV_IDS[i] == device_id)
{
// std::cout << "DEBUG: found bus " << std::hex << bus << " with device ID " << device_id << std::dec << "\n";
socket2bus.push_back(std::make_pair(mcfg[s].PCISegmentGroupNumber,bus));
socket2bus.emplace_back(std::make_pair(mcfg[s].PCISegmentGroupNumber,bus));
break;
}
}
Expand Down Expand Up @@ -6693,10 +6694,7 @@ size_t ServerPCICFGUncore::getNumMCChannels(const uint32 controller) const
return 0;
}

ServerPCICFGUncore::~ServerPCICFGUncore()
{
}

ServerPCICFGUncore::~ServerPCICFGUncore() = default;

void ServerPCICFGUncore::programServerUncoreMemoryMetrics(const ServerUncoreMemoryMetrics & metrics, const int rankA, const int rankB)
{
Expand Down
3 changes: 1 addition & 2 deletions src/cpucounters.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,7 +675,7 @@ class PCM_API PCM
}

//! \brief Redirects output destination to provided file, instead of std::cout and std::cerr (optional)
static void setOutput(const std::string filename, const bool cerrToo = false);
static void setOutput(const std::string & filename, const bool cerrToo = false);

//! \brief Restores output, closes output file if opened
void restoreOutput();
Expand Down Expand Up @@ -977,7 +977,6 @@ class PCM_API PCM
auto ctrl = pmu.counterControl[c];
if (ctrl.get() != nullptr)
{
*ctrl = MC_CH_PCI_PMON_CTL_EN;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer if you keep " *ctrl = MC_CH_PCI_PMON_CTL_EN;" because IIRC some PMU docs suggested that the PMU control register needs to be written without the event/umask and then in the next write the event/umask needs to be written.

*ctrl = MC_CH_PCI_PMON_CTL_EN | *curEvent;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/lspci.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ void print_pci(struct pci p, const PCIDB & pciDB)
void load_PCIDB(PCIDB & pciDB)
{
std::ifstream in(PCI_IDS_PATH);
std::string line, item;
std::string line;

if (!in.is_open())
{
Expand Down
2 changes: 1 addition & 1 deletion src/pcm-core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ extern "C" {
}
}

void print_usage(const string progname)
void print_usage(const string & progname)
{
cerr << "\n Usage: \n " << progname
<< " --help | [delay] [options] [-- external_program [external_program_options]]\n";
Expand Down
29 changes: 14 additions & 15 deletions src/pcm-iio.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,14 +180,14 @@ void print_nameMap() {
}


string a_title (const string &init, const string &name) {
string a_title(const string &init, const string &name) {
char begin = init[0];
string row = init;
row += name;
return row + begin;
}

string a_data (string init, struct data d) {
string a_data(const string &init, struct data d) {
char begin = init[0];
string row = init;
string str_d = unit_format(d.value);
Expand All @@ -198,7 +198,7 @@ string a_data (string init, struct data d) {
return row + begin;
}

string build_line(string init, string name, bool last_char = true, char this_char = '_')
string build_line(const string &init, const string &name, bool last_char = true, char this_char = '_')
{
char begin = init[0];
string row = init;
Expand All @@ -209,12 +209,12 @@ string build_line(string init, string name, bool last_char = true, char this_cha
}


string a_header_footer (string init, string name)
string a_header_footer(const string &init, const string &name)
{
return build_line(init, name);
}

vector<string> combine_stack_name_and_counter_names(string stack_name)
vector<string> combine_stack_name_and_counter_names(const string &stack_name)
{

vector<string> v;
Expand All @@ -223,7 +223,7 @@ vector<string> combine_stack_name_and_counter_names(string stack_name)
for (std::map<string,std::pair<h_id,std::map<string,v_id>>>::const_iterator iunit = nameMap.begin(); iunit != nameMap.end(); ++iunit) {
string h_name = iunit->first;
int h_id = (iunit->second).first;
tmp[h_id] = h_name;
tmp[h_id] = std::move(h_name);
//cout << "h_id:" << h_id << " name:" << h_name << "\n";
}
//XXX: How to simplify and just combine tmp & v?
Expand Down Expand Up @@ -350,7 +350,7 @@ vector<string> build_display(vector<struct iio_stacks_on_socket>& iios, vector<s

std::string build_csv_row(const std::vector<std::string>& chunks, const std::string& delimiter)
{
return std::accumulate(chunks.begin(), chunks.end(), std::string(""),
return std::accumulate(chunks.begin(), chunks.end(), std::string(),
[delimiter](const string &left, const string &right){
return left.empty() ? right : left + delimiter + right;
});
Expand Down Expand Up @@ -386,7 +386,6 @@ vector<string> build_csv(vector<struct iio_stacks_on_socket>& iios, vector<struc
for (std::map<uint32_t,map<uint32_t,struct counter*>>::const_iterator vunit = v_sort.cbegin(); vunit != v_sort.cend(); ++vunit) {
map<uint32_t, struct counter*> h_array = vunit->second;
uint32_t vv_id = vunit->first;
vector<uint64_t> h_data;
string v_name = h_array[0]->v_event_name;
if (human_readable) {
v_name += string(max_name_width - (v_name.size()), ' ');
Expand Down Expand Up @@ -545,7 +544,7 @@ bool IPlatformMapping10Nm::getSadIdRootBusMap(uint32_t socket_id, std::map<uint8

if ((sad_ctrl_cfg & 0xf) == socket_id) {
uint8_t sid = (sad_ctrl_cfg >> 4) & 0x7;
sad_id_bus_map.insert(std::pair<uint8_t, uint8_t>(sid, (uint8_t)bus));
sad_id_bus_map.emplace(sid, (uint8_t)bus);
}
}
}
Expand Down Expand Up @@ -921,19 +920,19 @@ vector<struct counter> load_events(PCM * m, const char* fn)
/* Ignore anyline with # */
//TODO: substring until #, if len == 0, skip, else parse normally
pccr->set_ccr_value(0);
if (line.find("#") != std::string::npos)
if (line.find('#') != std::string::npos)
continue;
/* If line does not have any deliminator, we ignore it as well */
if (line.find("=") == std::string::npos)
if (line.find('=') == std::string::npos)
continue;
std::istringstream iss(line);
string h_name, v_name;
while (std::getline(iss, item, ',')) {
std::string key, value;
uint64 numValue;
/* assume the token has the format <key>=<value> */
key = item.substr(0,item.find("="));
value = item.substr(item.find("=")+1);
key = item.substr(0,item.find('='));
value = item.substr(item.find('=')+1);
istringstream iss2(value);
iss2 >> setbase(0) >> numValue;

Expand Down Expand Up @@ -1120,11 +1119,11 @@ bool extract_argument_value(const char* arg, std::initializer_list<const char*>
const auto arg_name_len = strlen(arg_name);
if (arg_len > arg_name_len && strncmp(arg, arg_name, arg_name_len) == 0 && arg[arg_name_len] == '=') {
value = arg + arg_name_len + 1;
const auto last_pos = value.find_last_not_of("\"");
const auto last_pos = value.find_last_not_of('\"');
if (last_pos != string::npos) {
value.erase(last_pos + 1);
}
const auto first_pos = value.find_first_not_of("\"");
const auto first_pos = value.find_first_not_of('\"');
if (first_pos != string::npos) {
value.erase(0, first_pos);
}
Expand Down
10 changes: 5 additions & 5 deletions src/pcm-memory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ bool anyPmem(const ServerUncoreMemoryMetrics & metrics)

bool skipInactiveChannels = true;

void print_help(const string prog_name)
void print_help(const string & progname)
{
cerr << "\n Usage: \n " << prog_name
cerr << "\n Usage: \n " << progname
Copy link
Contributor

@ogbrugge-work ogbrugge-work Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can you merge this change into the previous change? I would like each change to compile on its own and it is clear that this cannot compile on its own and needs both changes

Copy link
Contributor

Choose a reason for hiding this comment

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

the change of the parameter name is not necessary and requires changing the name in other code lines. Could you please keep the old name?

<< " --help | [delay] [options] [-- external_program [external_program_options]]\n";
cerr << " <delay> => time interval to sample performance counters.\n";
cerr << " If not specified, or 0, with external program given\n";
Expand All @@ -107,9 +107,9 @@ void print_help(const string prog_name)
cerr << " --uninstallDriver | --installDriver=> (un)install driver\n";
#endif
cerr << " Examples:\n";
cerr << " " << prog_name << " 1 => print counters every second without core and socket output\n";
cerr << " " << prog_name << " 0.5 -csv=test.log => twice a second save counter values to test.log in CSV format\n";
cerr << " " << prog_name << " /csv 5 2>/dev/null => one sampe every 5 seconds, and discard all diagnostic output\n";
cerr << " " << progname << " 1 => print counters every second without core and socket output\n";
cerr << " " << progname << " 0.5 -csv=test.log => twice a second save counter values to test.log in CSV format\n";
cerr << " " << progname << " /csv 5 2>/dev/null => one sampe every 5 seconds, and discard all diagnostic output\n";
cerr << "\n";
}

Expand Down
2 changes: 1 addition & 1 deletion src/pcm-power.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ int main(int argc, char * argv[])
std::vector<std::string> skxLicenseStr = { "Core cycles where the core was running with power-delivery for baseline license level 0. This includes non-AVX codes, SSE, AVX 128-bit, and low-current AVX 256-bit codes.",
"Core cycles where the core was running with power-delivery for license level 1. This includes high current AVX 256-bit instructions as well as low current AVX 512-bit instructions.",
"Core cycles where the core was running with power-delivery for license level 2 (introduced in Skylake Server michroarchtecture). This includes high current AVX 512-bit instructions." };
licenseStr = skxLicenseStr;
licenseStr = std::move(skxLicenseStr);
regs[0].fields.event_select = 0x28; // CORE_POWER.LVL0_TURBO_LICENSE
regs[0].fields.umask = 0x07; // CORE_POWER.LVL0_TURBO_LICENSE
regs[1].fields.event_select = 0x28; // CORE_POWER.LVL1_TURBO_LICENSE
Expand Down
45 changes: 26 additions & 19 deletions src/pcm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,9 @@ double getAverageUncoreFrequencyGhz(const UncoreStateType& before, const UncoreS
return getAverageUncoreFrequency(before, after) / 1e9;
}

void print_help(const string prog_name)
void print_help(const string & progname)
{
cerr << "\n Usage: \n " << prog_name
cerr << "\n Usage: \n " << progname
Copy link
Contributor

@ogbrugge-work ogbrugge-work Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can you merge this change into the previous change? I would like each change to compile on its own and it is clear that this cannot compile on its own and needs both changes

Copy link
Contributor

Choose a reason for hiding this comment

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

same here. Please keep the old name.

<< " --help | [delay] [options] [-- external_program [external_program_options]]\n";
cerr << " <delay> => time interval to sample performance counters.\n";
cerr << " If not specified, or 0, with external program given\n";
Expand All @@ -103,10 +103,10 @@ void print_help(const string prog_name)
cerr << " -i[=number] | /i[=number] => allow to determine number of iterations\n";
print_help_force_rtm_abort_mode(37);
cerr << " Examples:\n";
cerr << " " << prog_name << " 1 -nc -ns => print counters every second without core and socket output\n";
cerr << " " << prog_name << " 1 -i=10 => print counters every second 10 times and exit\n";
cerr << " " << prog_name << " 0.5 -csv=test.log => twice a second save counter values to test.log in CSV format\n";
cerr << " " << prog_name << " /csv 5 2>/dev/null => one sampe every 5 seconds, and discard all diagnostic output\n";
cerr << " " << progname << " 1 -nc -ns => print counters every second without core and socket output\n";
cerr << " " << progname << " 1 -i=10 => print counters every second 10 times and exit\n";
cerr << " " << progname << " 0.5 -csv=test.log => twice a second save counter values to test.log in CSV format\n";
cerr << " " << progname << " /csv 5 2>/dev/null => one sampe every 5 seconds, and discard all diagnostic output\n";
cerr << "\n";
}

Expand Down Expand Up @@ -185,14 +185,22 @@ void print_output(PCM * m,
cout << " L3MPI : number of L3 (read) cache misses per instruction\n";
if (m->isL2CacheMissesAvailable())
cout << " L2MPI : number of L2 (read) cache misses per instruction\n";
if (m->memoryTrafficMetricsAvailable()) cout << " READ : bytes read from main memory controller (in GBytes)\n";
if (m->memoryTrafficMetricsAvailable()) cout << " WRITE : bytes written to main memory controller (in GBytes)\n";
if (m->localMemoryRequestRatioMetricAvailable()) cout << " LOCAL : ratio of local memory requests to memory controller in %\n";
if (m->LLCReadMissLatencyMetricsAvailable()) cout << "LLCRDMISSLAT: average latency of last level cache miss for reads and prefetches (in ns)\n";
if (m->PMMTrafficMetricsAvailable()) cout << " PMM RD : bytes read from PMM memory (in GBytes)\n";
if (m->PMMTrafficMetricsAvailable()) cout << " PMM WR : bytes written to PMM memory (in GBytes)\n";
if (m->MCDRAMmemoryTrafficMetricsAvailable()) cout << " MCDRAM READ : bytes read from MCDRAM controller (in GBytes)\n";
if (m->MCDRAMmemoryTrafficMetricsAvailable()) cout << " MCDRAM WRITE : bytes written to MCDRAM controller (in GBytes)\n";
if (m->memoryTrafficMetricsAvailable()) {
cout << " READ : bytes read from main memory controller (in GBytes)\n";
cout << " WRITE : bytes written to main memory controller (in GBytes)\n";
}
if (m->localMemoryRequestRatioMetricAvailable()) {
cout << " LOCAL : ratio of local memory requests to memory controller in %\n";
cout << "LLCRDMISSLAT: average latency of last level cache miss for reads and prefetches (in ns)\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

the availability of LLCRDMISSLAT metric should be checked with m->LLCReadMissLatencyMetricsAvailable() and not with m->localMemoryRequestRatioMetricAvailable()

}
if (m->PMMTrafficMetricsAvailable()) {
cout << " PMM RD : bytes read from PMM memory (in GBytes)\n";
cout << " PMM WR : bytes written to PMM memory (in GBytes)\n";
}
if (m->MCDRAMmemoryTrafficMetricsAvailable()) {
cout << " MCDRAM READ : bytes read from MCDRAM controller (in GBytes)\n";
cout << " MCDRAM WRITE : bytes written to MCDRAM controller (in GBytes)\n";
}
if (m->memoryIOTrafficMetricAvailable()) {
cout << " IO : bytes read/written due to IO requests to memory controller (in GBytes); this may be an over estimate due to same-cache-line partial requests\n";
cout << " IA : bytes read/written due to IA requests to memory controller (in GBytes); this may be an over estimate due to same-cache-line partial requests\n";
Expand Down Expand Up @@ -437,12 +445,11 @@ void print_output(PCM * m,
cout << " PMM RD | PMM WR |";
if (m->MCDRAMmemoryTrafficMetricsAvailable())
cout << " MCDRAM READ | MCDRAM WRITE |";
if (m->memoryIOTrafficMetricAvailable())
if (m->memoryIOTrafficMetricAvailable()) {
cout << " IO |";
if (m->memoryIOTrafficMetricAvailable())
cout << " IA |";
if (m->memoryIOTrafficMetricAvailable())
cout << " GT |";
}
if (m->packageEnergyMetricsAvailable())
cout << " CPU energy |";
if (m->dramEnergyMetricsAvailable())
Expand Down Expand Up @@ -547,13 +554,13 @@ void print_basic_metrics_csv_header(const PCM * m)
cout << "Frontend_bound(%),Bad_Speculation(%),Backend_Bound(%),Retiring(%),";
}

void print_csv_header_helper(string header, int count=1){
void print_csv_header_helper(const string & header, int count=1){
for(int i = 0; i < count; i++){
cout << header << ",";
}
}

void print_basic_metrics_csv_semicolons(const PCM * m, string header)
void print_basic_metrics_csv_semicolons(const PCM * m, const string & header)
{
print_csv_header_helper(header, 3); // EXEC;IPC;FREQ;
if (m->isActiveRelativeFrequencyAvailable())
Expand Down
2 changes: 1 addition & 1 deletion src/topology.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ class HyperThread : public SystemObject
}

void addMSRHandle( std::shared_ptr<SafeMsrHandle> handle ) {
msrHandle_ = handle;
msrHandle_ = std::move(handle);
}

int32 threadID() const {
Expand Down
2 changes: 1 addition & 1 deletion src/utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ std::vector<std::string> split(const std::string & str, const char delim)
uint64 read_number(const char* str)
{
std::istringstream stream(str);
if (strstr(str, "x")) stream >> std::hex;
if (strchr(str, 'x')) stream >> std::hex;
uint64 result = 0;
stream >> result;
return result;
Expand Down
4 changes: 2 additions & 2 deletions src/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ class MainLoop
{
unsigned numberOfIterations = 0;
public:
MainLoop() {}
MainLoop() = default;
bool parseArg(const char * arg)
{
if (strncmp(arg, "-i", 2) == 0 ||
Expand Down Expand Up @@ -372,7 +372,7 @@ struct StackedBarItem {
double fraction{0.0};
std::string label{""}; // not used currently
char fill{'0'};
StackedBarItem() {}
StackedBarItem() = default;
StackedBarItem(double fraction_,
const std::string & label_,
char fill_) : fraction(fraction_), label(label_), fill(fill_) {}
Expand Down