Skip to content

Conversation

@HLilit
Copy link
Contributor

@HLilit HLilit commented Jul 11, 2019

Improve memory measurements for ROOT interpreter benchmark

Improve memory measurements for ROOT interpreter benchmark
@oshadura oshadura requested review from oshadura and vgvassilev July 12, 2019 12:12
@oshadura oshadura self-assigned this Jul 12, 2019
Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

This is a partial review.

#include <unistd.h>
#include <chrono>
#include <stdlib.h>
#include "rootbench/RBConfig.h"
Copy link
Member

Choose a reason for hiding this comment

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

Includes should be in reverse order, that is, less generic to more generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I will change the order.


std::string getMemoryStats(const std::string& dir, const std::string& filename, std::string& rootInvocation, const std::string& rootexe, const std::string& endInvoc) {
std::string perffile="";
if (access("/usr/bin/time", X_OK) == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

We should not hardcode this path /usr/bin/time, we should use a more standard way to resolve a system binary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright! I will try to change it.

// FIXME: no source in /usr/dash
// We are writing /usr/bin/time -v output in file to get maximum resident memory for the benchmark
rootInvocation = "/usr/bin/time -v -o \"" + perffile + rootexe + fullpath + endInvoc;
std::cout<<"helllloooooooooooooooooooo "<< rootInvocation <<" hellllllloooooooooooooo"<<std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like debug printouts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes sorry, forgot to remove them.

@oshadura
Copy link
Contributor

Closing in favor of #190

@oshadura oshadura closed this Sep 25, 2020
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.

3 participants