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

[erikolofsson]

Fixes #1638

[Explorer09]

I don't have any more comment with this code now. But perhaps is good to explain what's the difference between this PR and #1643. Does this supersede #1643?

Cc @aestriplex

[Korrd]

Is this fix expected to be merged for next release? I just had to copy old 3.3.0 htop from another mac to mine in order to make it show the right CPU usage. Something like this that makes the software fail to a point it's not showing the right data should be fixed much quicker than several months, as meanwhile it makes it unreliable and unusable for a quite common use-case, which might make users look for alternatives instead, as having the code done right is useless if the product does not work as intended.

[erikolofsson]

Rebased on main (comment added to prevent GitHub from merging multiple force pushes)

[Explorer09]

@Korrd Please keep in mind I'm just a volunteer. While I can help evaluting the quality of the patches, I don't have all day to do it and I have no guarantee on which patch would be reviewed sooner or later. (And I'm not the maintainer of this project - this is just my personal position and not the maintainers'.)

[aestriplex]

I don't have any more comment with this code now. But perhaps is good to explain what's the difference between this PR and #1643. Does this supersede #1643?

Cc @aestriplex

@Explorer09 yes, this PR supersede #1643. If it's one with anyone, I can close it

[Explorer09]

I slightly reordered the logic of the Platform_calculateNanosecondsPerMachTick function:

static 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.
   *numer = 1;
   *denom = 1;

#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.
    *
    *    Rosetta 2 only supports x86-64, so skip this workaround when building for other architectures.
    */

   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;
      return;
   }
#endif

#ifdef HAVE_MACH_TIMEBASE_INFO
   mach_timebase_info_data_t info;
   if (mach_timebase_info(&info) == KERN_SUCCESS) {
      *numer = info.numer;
      *denom = info.denom;
      return;
   }
#endif
}

I feel the code path like this would be easier to follow. LGTM for other parts of the code.

[BenBE]

I slightly reordered the logic of the Platform_calculateNanosecondsPerMachTick function:

I feel the code path like this would be easier to follow. LGTM for other parts of the code.

Minor caveat, as with the current way the return;s are written, it looks like there's a path without return value, which looks odd. Instead, a slightly clearer variant to order would be:

static void Platform_calculateNanosecondsPerMachTick(uint64_t* numer, uint64_t* denom) {
   // Check if we can determine the timebase used on this system.

#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.
    *
    *    Rosetta 2 only supports x86-64, so skip this workaround when building for other architectures.
    */

   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;
      return;
   }
#endif

#ifdef HAVE_MACH_TIMEBASE_INFO
   mach_timebase_info_data_t info = { 0 };
   if (mach_timebase_info(&info) == KERN_SUCCESS) {
      *numer = info.numer;
      *denom = info.denom;
      return;
   }
#endif

   // No info on actual timebase found; assume timebase in nanoseconds.
   *numer = 1;
   *denom = 1;
}

That way, the fallback is clearly visible. This also highlights, that both numer and denom need to be assigned for each return path; although with this version it's no longer enforced as well as with the other variant.

[erikolofsson]

That way, the fallback is clearly visible. This also highlights, that both numer and denom need to be assigned for each return path; although with this version it's no longer enforced as well as with the other variant.

I applied the suggested fixes

[Korrd]

Thanks a lot guys! <3

My love to you all!