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

#722 Adding Host Name in Reporting #733

Merged
merged 9 commits into from
Dec 11, 2018
9 changes: 9 additions & 0 deletions include/benchmark/benchmark.h
Original file line number Diff line number Diff line change
Expand Up @@ -1293,6 +1293,14 @@ struct CPUInfo {
BENCHMARK_DISALLOW_COPY_AND_ASSIGN(CPUInfo);
};

struct SystemInformation {
Copy link
Member

Choose a reason for hiding this comment

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

nit: for consistency with CPUInfo, maybe SystemInfo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can be done, let me know if you want me proceed with this

Copy link
Member

Choose a reason for hiding this comment

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

Yes please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

std::string name;
static const SystemInformation& Get();
private:
SystemInformation();
BENCHMARK_DISALLOW_COPY_AND_ASSIGN(SystemInformation);
};

// 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 +1310,7 @@ class BenchmarkReporter {
public:
struct Context {
CPUInfo const& cpu_info;
SystemInformation const& sys_info;
// The number of chars in the longest benchmark name.
size_t name_field_width;
static const char* executable_name;
Expand Down
5 changes: 4 additions & 1 deletion src/reporter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ void BenchmarkReporter::PrintBasicContext(std::ostream *out,

Out << LocalDateTimeString() << "\n";

const SystemInformation &sys_info = context.sys_info;
Out << "System Name "<< 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.

Is this printed only for the json dumper, or for all of them?
Put differently, i don't think it should be in the console output, it is getting rather crowded already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed from console and added only to JSON


if (context.executable_name)
Out << "Running " << context.executable_name << "\n";

Expand Down Expand Up @@ -79,7 +82,7 @@ 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(SystemInformation::Get()) {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Linewrap


std::string BenchmarkReporter::Run::benchmark_name() const {
std::string name = run_name;
Expand Down
31 changes: 31 additions & 0 deletions src/sysinfo.cc
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,30 @@ std::vector<CPUInfo::CacheInfo> GetCacheSizes() {
#endif
}

std::string GetSystemName() {
#if defined(BENCHMARK_OS_WINDOWS)
std::string str;
const unsigned COUNT = 32767;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, why invent a magic number.
MAX_COMPUTERNAME_LENGTH+1 ?
And that being said, isn't that a rather too big to have as an array on stack?
Maybe use resized std::string?

TCHAR hostname[COUNT];
DWORD DWCOUNT = COUNT;
if (!GetComputerName(hostname, &DWCOUNT))
return std::string("Unable to Get Host Name");
Copy link
Member

Choose a reason for hiding this comment

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

return empty string and don't print in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

merging requested changes

#ifndef UNICODE
str = hostname;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you 100.0% sure this is safe?
https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-getcomputernamew

On input, specifies the size of the buffer, in TCHARs. On output, the number of TCHARs copied to the destination buffer, not including the terminating null character.

I'd recommend to use 'returned' DWCOUNT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initialising TCHAR to null and only copying DWCOUNT into string.

#else
std::wstring wStr = hostname;
str = std::string(wStr.begin(), wStr.end());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work? Is this already done elsewhere in the library?
wchar is bigger than char, what makes sure that this isn't lossy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using Standard wstring_convert to fix this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, same comment about DWCOUNT.

#endif
return str;
#else
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#else
#else // defined(BENCHMARK_OS_WINDOWS)
// Catch-all POSIX block.

const unsigned COUNT = 64;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just use HOST_NAME_MAX

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HOST_NAME_MAX was not present on OSx, added local initialization.
The Job on Travis failed in config, can you please have a look at it?

char hostname[COUNT];
int retVal = gethostname(hostname, COUNT);
if (retVal != 0) return std::string("Unable to Get Host Name");
return std::string(hostname);
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

And others?

#if defined(__CYGWIN__)
#define BENCHMARK_OS_CYGWIN 1
#elif defined(_WIN32)
#define BENCHMARK_OS_WINDOWS 1
#if defined(__MINGW32__)
#define BENCHMARK_OS_MINGW 1
#endif
#elif defined(__APPLE__)
#define BENCHMARK_OS_APPLE 1
#include "TargetConditionals.h"
#if defined(TARGET_OS_MAC)
#define BENCHMARK_OS_MACOSX 1
#if defined(TARGET_OS_IPHONE)
#define BENCHMARK_OS_IOS 1
#endif
#endif
#elif defined(__FreeBSD__)
#define BENCHMARK_OS_FREEBSD 1
#elif defined(__NetBSD__)
#define BENCHMARK_OS_NETBSD 1
#elif defined(__OpenBSD__)
#define BENCHMARK_OS_OPENBSD 1
#elif defined(__linux__)
#define BENCHMARK_OS_LINUX 1
#elif defined(__native_client__)
#define BENCHMARK_OS_NACL 1
#elif defined(__EMSCRIPTEN__)
#define BENCHMARK_OS_EMSCRIPTEN 1
#elif defined(__rtems__)
#define BENCHMARK_OS_RTEMS 1
#elif defined(__Fuchsia__)
#define BENCHMARK_OS_FUCHSIA 1
#elif defined (__SVR4) && defined (__sun)
#define BENCHMARK_OS_SOLARIS 1
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only segregation needed here is POSIX vs non POSIX Compliance.
POSIX has gethostname

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#endif
#endif // Catch-all POSIX block.

}

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


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

SystemInformation::SystemInformation() : name(GetSystemName()) {}
} // end namespace benchmark
1 change: 1 addition & 0 deletions test/reporter_output_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ static int AddContextCases() {
AddCases(TC_ConsoleErr,
{
{"%int[-/]%int[-/]%int %int:%int:%int$", MR_Default},
{"System Name", MR_Next},
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is "system name"?
How is this different from "host name"?
If it is exactly the "host name", just use that term?

{"Running .*/reporter_output_test(\\.exe)?$", MR_Next},
{"Run on \\(%int X %float MHz CPU s?\\)", MR_Next},
});
Expand Down