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

#722 Adding Host Name in Reporting #733

Merged
merged 9 commits into from
Dec 11, 2018
Merged

#722 Adding Host Name in Reporting #733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3f44492
Adding Host Name and test
jatinx Nov 29, 2018
f38f8ef
Addressing Review Comments
jatinx Nov 29, 2018
2c6d90c
Adding Test for JSON Reporter
jatinx Nov 29, 2018
0ffce1d
Adding HOST_NAME_MAX for MacOS systems
jatinx Nov 29, 2018
8128e07
Adding Explaination for MacOS HOST_NAME_MAX Addition
jatinx Nov 30, 2018
7147aee
Addressing Peer Review Comments
jatinx Dec 10, 2018
b628cf4
Adding codecvt in windows header guard
jatinx Dec 10, 2018
6223a48
Changing name SystemInfo and adding empty message incase host name fe…
jatinx Dec 10, 2018
a651c6f
Adding Comment on Struct SystemInfo
jatinx Dec 10, 2018
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
10 changes: 10 additions & 0 deletions include/benchmark/benchmark.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1293,6 +1293,15 @@ struct CPUInfo {
BENCHMARK_DISALLOW_COPY_AND_ASSIGN(CPUInfo);
};

//Adding Struct for System Information
struct SystemInfo {
std::string name;
static const SystemInfo& Get();
private:
SystemInfo();
BENCHMARK_DISALLOW_COPY_AND_ASSIGN(SystemInfo);
};

// Interface for custom benchmark result printers.
// By default, benchmark reports are printed to stdout. However an application
// can control the destination of the reports by calling
Expand All @@ -1302,6 +1311,7 @@ class BenchmarkReporter {
public:
struct Context {
CPUInfo const& cpu_info;
SystemInfo const& sys_info;
// The number of chars in the longest benchmark name.
size_t name_field_width;
static const char* executable_name;
Expand Down
2 changes: 2 additions & 0 deletions src/json_reporter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ bool JSONReporter::ReportContext(const Context& context) {
std::string walltime_value = LocalDateTimeString();
out << indent << FormatKV("date", walltime_value) << ",\n";

out << indent << FormatKV("host_name", context.sys_info.name) << ",\n";
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. We are sure that this won't contain \\? (see executable_name)
  2. Do we want to print "Unable to Get Host Name"?2
  3. I wonder if this should be after the executable_name.

Copy link
Member

Choose a reason for hiding this comment

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

  1. No. We should check and only output if it's available.

Copy link
Contributor Author

@jatinx jatinx Dec 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Yes, I checked the output of the function, it just outputs the hostname for windows without the \\
  2. Addressed
  3. I am not sure about this one, it can be done, but since the output being in json only, is it desirable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aren't we still always printing it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks about right other than this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you talking about the 2nd point?
I have placed an empty string incase it fails to fetch the host name


if (Context::executable_name) {
// windows uses backslash for its path separator,
// which must be escaped in JSON otherwise it blows up conforming JSON
Expand Down
3 changes: 2 additions & 1 deletion src/reporter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ void BenchmarkReporter::PrintBasicContext(std::ostream *out,
// No initializer because it's already initialized to NULL.
const char *BenchmarkReporter::Context::executable_name;

BenchmarkReporter::Context::Context() : cpu_info(CPUInfo::Get()) {}
BenchmarkReporter::Context::Context()
: cpu_info(CPUInfo::Get()), sys_info(SystemInfo::Get()) {}

std::string BenchmarkReporter::Run::benchmark_name() const {
std::string name = run_name;
Expand Down
38 changes: 38 additions & 0 deletions src/sysinfo.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#undef StrCat // Don't let StrCat in string_util.h be renamed to lstrcatA
#include <versionhelpers.h>
#include <windows.h>
#include <codecvt>
#else
#include <fcntl.h>
#ifndef BENCHMARK_OS_FUCHSIA
Expand Down Expand Up @@ -52,6 +53,7 @@
#include <limits>
#include <memory>
#include <sstream>
#include <locale>

#include "check.h"
#include "cycleclock.h"
Expand Down Expand Up @@ -366,6 +368,35 @@ std::vector<CPUInfo::CacheInfo> GetCacheSizes() {
#endif
}

std::string GetSystemName() {
#if defined(BENCHMARK_OS_WINDOWS)
std::string str;
const unsigned COUNT = MAX_COMPUTERNAME_LENGTH+1;
TCHAR hostname[COUNT] = {'\0'};
DWORD DWCOUNT = COUNT;
if (!GetComputerName(hostname, &DWCOUNT))
return std::string("");
#ifndef UNICODE
str = std::string(hostname, DWCOUNT);
#else
//Using wstring_convert, Is deprecated in C++17
using convert_type = std::codecvt_utf8<wchar_t>;
std::wstring_convert<convert_type, wchar_t> converter;
std::wstring wStr(hostname, DWCOUNT);
str = converter.to_bytes(wStr);
#endif
return str;
#else // defined(BENCHMARK_OS_WINDOWS)
#ifdef BENCHMARK_OS_MACOSX //Mac Doesnt have HOST_NAME_MAX defined
#define HOST_NAME_MAX 64
#endif
char hostname[HOST_NAME_MAX];
int retVal = gethostname(hostname, HOST_NAME_MAX);
if (retVal != 0) return std::string("");
return std::string(hostname);
#endif // Catch-all POSIX block.
}

int GetNumCPUs() {
#ifdef BENCHMARK_HAS_SYSCTL
int NumCPU = -1;
Expand Down Expand Up @@ -609,4 +640,11 @@ CPUInfo::CPUInfo()
scaling_enabled(CpuScalingEnabled(num_cpus)),
load_avg(GetLoadAvg()) {}


const SystemInfo& SystemInfo::Get() {
static const SystemInfo* info = new SystemInfo();
return *info;
}

SystemInfo::SystemInfo() : name(GetSystemName()) {}
} // end namespace benchmark
1 change: 1 addition & 0 deletions test/reporter_output_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ static int AddContextCases() {
{{"^\\{", MR_Default},
{"\"context\":", MR_Next},
{"\"date\": \"", MR_Next},
{"\"host_name\":", MR_Next},
{"\"executable\": \".*(/|\\\\)reporter_output_test(\\.exe)?\",",
MR_Next},
{"\"num_cpus\": %int,$", MR_Next},
Expand Down