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

Added check for CLK_TCK for darwin #869

Merged
merged 1 commit into from
May 17, 2020
Merged

Conversation

renaynay
Copy link
Contributor

@renaynay renaynay commented May 12, 2020

This PR adds a check for the clock tick of darwin OS as is done for freebsd, linux, and openbsd when calculating CPU stats instead of assuming a static ClocksPerSec of 128.

@Lomanic
Copy link
Collaborator

Lomanic commented May 12, 2020

Shelling out to an external process is to be avoided as it's expensive (relatively slow) and relatively unreliable (output could change over time or between different hosts, recent #867 is a telling example).

In this case and reading code from https://github.com/tklauser/go-sysconf and https://stackoverflow.com/a/10455939, CLK_TCK (same thing as CLOCKS_PER_SEC, at least on Darwin) is defined at build-time and from POSIX to a constant, so there is no need to call getconf at runtime, just define it as a constant, from C headers. Or we should just use go-sysconf.

@gballet
Copy link
Contributor

gballet commented May 13, 2020

@Lomanic that's a fair point, however what if the machine that compiles the code has a different value than the machine it runs on?

The default value was 128 which is different from the 100 we notice on the machine we tried it with. Presumably, the person who set this value noticed something different on their machines.

@Lomanic Lomanic merged commit ee64e05 into shirou:master May 17, 2020
@Lomanic
Copy link
Collaborator

Lomanic commented May 17, 2020

I think the 128 constant is a mistake and should be set to 100 as per my previous comment, not sure this value can change on Darwin (it's defined by POSIX and Darwin is even certified to be compliant). Either way, it's better to obtain the proper value now.

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