Skip to content

Bring back conversion of process CPU time on macOS (#1638) #1691

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Bring back conversion of process CPU time on macOS (#1638) #1691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,8 @@ return ptr != &ch; /* Should be 0 (exit success) */
CFLAGS=$htop_save_CFLAGS

if test "$my_htop_platform" = darwin; then
AC_CHECK_FUNCS([mach_timebase_info])

AC_CHECK_FUNCS([host_statistics64], [
AC_CHECK_TYPES([struct vm_statistics64], [], [], [[#include <mach/vm_statistics.h>]])
], [])
Expand Down
4 changes: 2 additions & 2 deletions darwin/DarwinProcess.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,8 +372,8 @@ void DarwinProcess_setFromLibprocPidinfo(DarwinProcess* proc, DarwinProcessTable
const DarwinMachine* dhost = (const DarwinMachine*) proc->super.super.host;

uint64_t total_existing_time_ns = proc->stime + proc->utime;
uint64_t user_time_ns = pti.pti_total_user;
uint64_t system_time_ns = pti.pti_total_system;
uint64_t user_time_ns = Platform_machTicksToNanoseconds(pti.pti_total_user);
uint64_t system_time_ns = Platform_machTicksToNanoseconds(pti.pti_total_system);
uint64_t total_current_time_ns = user_time_ns + system_time_ns;

if (total_existing_time_ns < total_current_time_ns) {
Expand Down
16 changes: 16 additions & 0 deletions darwin/Platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,15 @@ const MeterClass* const Platform_meterTypes[] = {
NULL
};

static uint64_t Platform_nanosecondsPerMachTickNumer = 1;
static uint64_t Platform_nanosecondsPerMachTickDenom = 1;

static double Platform_nanosecondsPerSchedulerTick = -1;

static mach_port_t iokit_port; // the mach port used to initiate communication with IOKit

bool Platform_init(void) {
Platform_calculateNanosecondsPerMachTick(&Platform_nanosecondsPerMachTickNumer, &Platform_nanosecondsPerMachTickDenom);

// Determine the number of scheduler clock ticks per second
errno = 0;
Expand All @@ -168,6 +172,18 @@ bool Platform_init(void) {
return true;
}

// Converts ticks in the Mach "timebase" to nanoseconds.
// See `mach_timebase_info`, as used to define the `Platform_nanosecondsPerMachTick` constant.
uint64_t Platform_machTicksToNanoseconds(uint64_t mach_ticks) {
uint64_t ticks_quot = mach_ticks / Platform_nanosecondsPerMachTickDenom;
uint64_t ticks_rem = mach_ticks % Platform_nanosecondsPerMachTickDenom;

Copy link
Contributor

Choose a reason for hiding this comment

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

   assert(ticks_quot <= UINT64_MAX / Platform_nanosecondsPerMachTickNumer);
   assert(Platform_nanosecondsPerMachTickDenom <= UINT64_MAX / Platform_nanosecondsPerMachTickNumer);

These two constraints are what prevent the overflow from happening in this function. Not sure if these two assertions are worth adding into the function, but I mention these just for completeness sake.

The first assertion is related to a time overflow (like Y2K) that might be out of scope of htop.
The second assertion can always hold true when mach_timebase_info_data_t uses 32-bit integers for both numerator and denominator fields.

Copy link
Member

Choose a reason for hiding this comment

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

They don't exactly hurt, but given this is what ultimately gives the time values used in htop, they should not straight up cause htop to terminate seemingly at random. Instead normal handling that causes a runtime warning (once) could be done instead …

Can you elaborate on the intuition of the second assertion?

Copy link
Contributor

@Explorer09 Explorer09 May 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@BenBE

ticks_rem < Platform_nanosecondsPerMachTickDenom ;
ticks_rem * Platform_nanosecondsPerMachTickNumer < Platform_nanosecondsPerMachTickDenom * Platform_nanosecondsPerMachTickNumer
(or Platform_nanosecondsPerMachTickNumer == 0)

When Platform_nanosecondsPerMachTickDenom * Platform_nanosecondsPerMachTickNumer is less than 2^64, ticks_rem * Platform_nanosecondsPerMachTickNumer will be less than 2^64 as well, i.e. never overflows.

Copy link
Author

Choose a reason for hiding this comment

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

Do you want me to do any changes?

The overflow should only happen when the result in nano-seconds overflows, which would already be a problem in other places in the application. This is after 584 CPU-years.

Copy link
Member

Choose a reason for hiding this comment

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

Optional. I mostly asked for some clarifications here.

If you decided to update the PR, be sure to avoid direct asserts (for the reasons mentioned above), and instead choose a way that is as obvious as possible, that this is some intentional error value.

Copy link
Contributor

Choose a reason for hiding this comment

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

@erikolofsson My view is that the time overflow bug that would happen 584 years later (the overflow on part1) is out of scope, but without an in-code comment, the (ticks_rem * Platform_nanosecondsPerMachTickNumer) in part2 can be mistaken as prone to overflow. (It actually doesn't overflow for the reason I stated above.)

uint64_t part1 = ticks_quot * Platform_nanosecondsPerMachTickNumer;
uint64_t part2 = (ticks_rem * Platform_nanosecondsPerMachTickNumer) / Platform_nanosecondsPerMachTickDenom;

return part1 + part2;
}

// Converts "scheduler ticks" to nanoseconds.
// See `sysconf(_SC_CLK_TCK)`, as used to define the `Platform_nanosecondsPerSchedulerTick` constant.
double Platform_schedulerTicksToNanoseconds(const double scheduler_ticks) {
Expand Down
4 changes: 4 additions & 0 deletions darwin/Platform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ extern const MeterClass* const Platform_meterTypes[];

bool Platform_init(void);

// Converts ticks in the Mach "timebase" to nanoseconds.
// See `mach_timebase_info`, as used to define the `Platform_nanosecondsPerMachTick*` constants.
uint64_t Platform_machTicksToNanoseconds(uint64_t mach_ticks);

// Converts "scheduler ticks" to nanoseconds.
// See `sysconf(_SC_CLK_TCK)`, as used to define the `Platform_nanosecondsPerSchedulerTick` constant.
double Platform_schedulerTicksToNanoseconds(const double scheduler_ticks);
Expand Down
39 changes: 39 additions & 0 deletions darwin/PlatformHelpers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,42 @@ bool Platform_isRunningTranslated(void) {
}
return ret;
}

void Platform_calculateNanosecondsPerMachTick(uint64_t* numer, uint64_t* denom) {
// Check if we can determine the timebase used on this system.
// If the API is unavailable assume we get our timebase in nanoseconds.
#ifdef HAVE_MACH_TIMEBASE_INFO
mach_timebase_info_data_t info;
mach_timebase_info(&info);

*numer = info.numer;
*denom = info.denom;
#else
*numer = 1;
*denom = 1;
#endif

// Taken that Rosetta2 only supports x86_64 we only need to apply the
// workaround if we are running on that architecture.
#ifdef __x86_64__
/* WORKAROUND for `mach_timebase_info` giving incorrect values on M1 under Rosetta 2.
* rdar://FB9546856 http://www.openradar.appspot.com/FB9546856
*
* We don't know exactly what feature/attribute of the M1 chip causes this mistake under Rosetta 2.
* Until we have more Apple ARM chips to compare against, the best we can do is special-case
* the "Apple M1" chip specifically when running under Rosetta 2.
*/

bool isRunningUnderRosetta2 = Platform_isRunningTranslated();

// Kernel version 20.0.0 is macOS 11.0 (Big Sur)
bool isBuggedVersion = 0 <= Platform_CompareKernelVersion((KernelVersion) {20, 0, 0});

if (isRunningUnderRosetta2 && isBuggedVersion) {
// In this case `mach_timebase_info` provides the wrong value, so we hard-code the correct factor,
// as determined from `mach_timebase_info` as if the process was running natively.
*numer = 125;
*denom = 3;
}
#endif
}
3 changes: 3 additions & 0 deletions darwin/PlatformHelpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ in the source distribution for its full text.
*/

#include <stdbool.h>
#include <stdint.h>
#include <sys/types.h>


Expand All @@ -29,6 +30,8 @@ int Platform_CompareKernelVersion(KernelVersion v);
// lowerBound <= currentVersion < upperBound
bool Platform_KernelVersionIsBetween(KernelVersion lowerBound, KernelVersion upperBound);

void Platform_calculateNanosecondsPerMachTick(uint64_t* numer, uint64_t* denom);

void Platform_getCPUBrandString(char* cpuBrandString, size_t cpuBrandStringSize);

bool Platform_isRunningTranslated(void);
Expand Down
Loading