darwin: Fix memory metrics support for Apple Silicon (ARM64) #1439

SuCicada · 2024-04-08T09:37:02Z

Fix memory metrics of Apple Silicon (ARM64)
Detailed description: #631

The calculation of memory refers to exelban/stats Modules/RAM/readers.swift

let used = active + inactive + speculative + wired + compressed - purgeable - external

Explorer09 · 2024-04-08T19:22:45Z

darwin/DarwinMachine.h

@@ -19,6 +19,9 @@ typedef struct DarwinMachine_ {

   host_basic_info_data_t host_info;
   vm_statistics_data_t vm_stats;
+#if defined(__arm64__)
+   vm_statistics64_data_t vm_stats64;


This structure does not seem to be exclusive to ARM64. Would it be used in Intel Macs, too? A reference to Apple documentation would be useful.

You are right. According to the documentation:
https://developer.apple.com/documentation/kernel/vm_statistics64_data_t
https://developer.apple.com/documentation/kernel/vm_statistics_data_t

vm_statistics64_data_t can be used after macOS 10.6+
vm_statistics_data_t can be used after macOS 10.0+

@SuCicada I prefer keeping a fallback mechanism when vm_statistics64_data_t is not available. There is no reason to remove 32-bit Mac support (htop documentation doesn't state a minimum required version of macOS).

Haven't looked at the details of the implementation yet, but would it be feasible to check for vm_statistics64_data_t and vm_statistics_data_t in configure and use the 64 bit version everywhere when available? And if none of both is available, we either fall back entirely or bail out at compile time.

@BenBE Since it is said that vm_statistics_data_t is available in OS X 10.0 (the very first Mac OS X), these is no need to check this one in particular in configure. Just checking for vm_statistics64_data_t is enough.

So I will restore support for vm state 32.

Exactly: Use vm_statistics64_data_t when available, otherwise fall back to vm_statistics_data_t.

Explorer09 · 2024-04-08T19:24:53Z

darwin/DarwinMachine.c

@@ -75,6 +85,9 @@ void Machine_scan(Machine* super) {
   host->prev_load = host->curr_load;
   DarwinMachine_allocateCPULoadInfo(&host->curr_load);
   DarwinMachine_getVMStats(&host->vm_stats);
+#if defined(__arm64__)
+   DarwinMachine_getVMStats64(&host->vm_stats64);


Any reason to duplicate the effort of retrieving both vm_stats structures?

Just to prevent affecting existing functionality, but I checked the code again and it didn't affect anywhere else.
Same as above. I fix that.

Explorer09 · 2024-04-08T19:27:20Z

darwin/DarwinMachine.c

@@ -67,6 +67,16 @@ static void DarwinMachine_getVMStats(vm_statistics_t p) {
   }
 }

+#if defined(__arm64__)
+static void DarwinMachine_getVMStats64(vm_statistics64_t p) {


Consider merging this function with DarwinMachine_getVMStats above, so as to avoid duplicate code.

You can use "DarwinMachine* this" as the input argument.

Explorer09 · 2024-04-08T19:30:18Z

darwin/Platform.c

@@ -290,6 +290,24 @@ double Platform_setCPUValues(Meter* mtr, unsigned int cpu) {
   return CLAMP(total, 0.0, 100.0);
 }

+#if defined(__arm64__)
+void Platform_setMemoryValues(Meter *mtr) {


Consider merging two Platform_setMemoryValues functions together.

Explorer09 · 2024-04-08T19:37:20Z

darwin/Platform.c

+   double page_K = (double) vm_page_size / (double) 1024;
+
+   mtr->total = dhost->host_info.max_mem / 1024;
+   int used = vm->active_count + vm->inactive_count +


Is it intentional to use signed int type here? Note that vm->active_count and similar members are in "natural_t" type, which is unsigned.

BenBE · 2024-04-09T15:16:34Z

Two general notes:

Squash the two commits, as commits should show a direct way from A to B without all the steps inside the maze … Mostly split commits, when there are independent changes that work without each other or there's something preparing for a later change
There should be no blank after casts, like this (foo)expression …

Also note the remarks earlier re implementation for both 32 and 64 bit VM structures using conditional compilation.

SuCicada · 2024-04-10T02:31:19Z

Thank you for your guidance, I have resubmitted.

I fix the blank format
I use __LP64__ for conditional compilation instead.
I restored 32-bit support and removed duplicate functions that I had written before.

BenBE · 2024-04-10T05:34:06Z

LGTM.

Any particular reason for checking an architecture setting (64 bit support of the architecture) over explicit testing for availability of a concrete type/structure in configure.ac?

SuCicada · 2024-04-11T02:24:55Z

LGTM.

Any particular reason for checking an architecture setting (64 bit support of the architecture) over explicit testing for availability of a concrete type/structure in configure.ac?

Oh, You are right. It is more explicit to test the availability of a concrete type/structure in configure.ac.
I will try

SuCicada · 2024-04-11T09:48:34Z

Thank you for your guidance, I have resubmitted.

I added checking of vm_statistics64 at configure.ac.
I use HAVE_VM_STATISTICS64 for conditional compilation instead.

What do you think

BenBE · 2024-04-11T09:52:35Z

configure.ac

+
+   AC_MSG_CHECKING([for vm_statistics64 supported])
+   AC_COMPILE_IFELSE([
+      AC_LANG_PROGRAM([[
+#include <mach/mach_host.h>
+#include <mach/vm_statistics.h>
+      ]], [[
+            mach_msg_type_number_t info_size = HOST_VM_INFO64_COUNT;
+            vm_statistics64_data_t vm_stat;
+            host_statistics64(mach_host_self(), HOST_VM_INFO64, (host_info64_t)&vm_stat, &info_size);
+      ]]
+   )],
+   AC_DEFINE([HAVE_VM_STATISTICS64], 1, [The vm_statistics64 features is supported.])
+   AC_MSG_RESULT(yes),
+   AC_MSG_RESULT(no))


Why not use things like this (untested)?

Suggested change

AC_MSG_CHECKING([for vm_statistics64 supported])

AC_COMPILE_IFELSE([

AC_LANG_PROGRAM([[

#include <mach/mach_host.h>

#include <mach/vm_statistics.h>

]], [[

mach_msg_type_number_t info_size = HOST_VM_INFO64_COUNT;

vm_statistics64_data_t vm_stat;

host_statistics64(mach_host_self(), HOST_VM_INFO64, (host_info64_t)&vm_stat, &info_size);

]]

)],

AC_DEFINE([HAVE_VM_STATISTICS64], 1, [The vm_statistics64 features is supported.])

AC_MSG_RESULT(yes),

AC_MSG_RESULT(no))

AC_CHECK_FUNCS([host_statistics64], [], [], [[#include <mach/vm_statistics.h>]])

AC_CHECK_DECLS([vm_statistics64_data_t], [], [], [[#include <mach/vm_statistics.h>]])

@BenBE You and I suggested the same thing at the same time...

By the way, AC_CHECK_DECLS is not for checking type declarations, use AC_CHECK_TYPES instead. See my other comment.

I had a look at the AutoConf docs, but didn't see it there somehow …

Explorer09 · 2024-04-11T10:00:39Z

configure.ac

@@ -330,6 +330,21 @@ if test "$my_htop_platform" = darwin; then
   AC_CHECK_FUNCS([mach_timebase_info])
   AC_CHECK_DECLS([IOMainPort], [], [], [[#include <IOKit/IOKitLib.h>]])
   AC_CHECK_DECLS([IOMasterPort], [], [], [[#include <IOKit/IOKitLib.h>]])
+
+   AC_MSG_CHECKING([for vm_statistics64 supported])


I thought you can use AC_CHECK_TYPES macro to simplify the check.
Maybe this is sufficient:

AC_CHECK_TYPES([struct vm_statistics64], [], [], [[#include <mach/vm_statistics.h>]] dnl This defines HAVE_STRUCT_VM_STATISTICS64

SuCicada · 2024-04-11T16:42:55Z

Makes sense, but this will produce 2 macros. If use
If someone's computer cannot meet host_statistics64 and vm_statistics64_data_t at the same time, (very unlikely)
Then it will happen:

host_statistics64 support	vm_statistics64_data_t support	result
yes	no	can compile and run normally, but the memory value is incorrect
no	yes	can compile normally, but fails to run: `[1] 16517 segmentation fault ./htop`

I am not sure if there is a better solution, or if we can ignore this very unlikely situation.

BenBE · 2024-04-11T20:21:10Z

AutoConf has the ability to nest those macros, i.e. only check the second if the first succeeded. Likewise for the second macro: Only define the feature flag once the second macro succeeded too.

Untested:

   AC_CHECK_FUNCS([host_statistics64], [
      AC_CHECK_TYPES([struct vm_statistics64], [
         AC_DEFINE([HAVE_VM_STATISTICS64], 1, [The vm_statistics64 features is supported.])
      ], [], [[#include <mach/vm_statistics.h>]])
   ], [], [[#include <mach/vm_statistics.h>]])

Check for HAVE_VM_STATISTICS64 in the code …

Explorer09 · 2024-04-11T21:17:33Z

@BenBE
Both AC_CHECK_FUNCS and AC_CHECK_TYPES would define the "HAVE_" C macro names for you. There's no need to use AC_DEFINE manually.

There would be HAVE_STRUCT_VM_STATISTICS64 and HAVE_HOST_STATISTICS64 defined automatically. Just pick a name that sounds better in the C code. And put that macro check as an inner call in configure.ac.

AC_CHECK_TYPES([struct vm_statistics64], [
   AC_CHECK_FUNCS([host_statistics64])
   ], [], [[#include <mach/vm_statistics.h>]])
dnl Then use HAVE_HOST_STATISTICS64 in the C code

BenBE · 2024-04-11T21:20:02Z

I know. The AC_DEFINE was there to use the other code AS-IS and have the implicit AND …

SuCicada · 2024-04-12T02:16:26Z

Thank you for your patient guidance, I have resubmitted.

(In fact, I had previously attempted this method but mistakenly believed it didn't work🫣. After your confirmation, I have re-verified and got the correct compile result.👍)

BenBE

Not sure about the changed memory calculation. There seems something off …

BenBE · 2024-04-12T05:36:14Z

darwin/Platform.c

+#ifdef HAVE_STRUCT_VM_STATISTICS64
+   natural_t used = vm->active_count + vm->inactive_count +
+              vm->speculative_count + vm->wire_count +
+              vm->compressor_page_count - vm->purgeable_count - vm->external_page_count;
+   mtr->values[MEMORY_METER_USED] = (double)(used - vm->compressor_page_count) * page_K;
+#else
   mtr->values[MEMORY_METER_USED] = (double)(vm->active_count + vm->wire_count) * page_K;
+#endif


Any reasons for this calculation to differ between those two places, besides missing fields from the old struct?

Also, vm->compressor_page_count is substracted twice AFAICS.

As #631 and #955 said, the current memory calculation may have a bit problem.

I referenced the code of exelban/stats and GuillaumeGomez/sysinfo(used by ClementTsang/bottom mentioned by #955 )

There are some differences in details, but it seems that the main thing missing is adding vm->compressor_page_count

It seems that many people have raised the same issue of memory parameter display.
So I just want macOS to display the correct memory parameters. But there may be something I haven't considered.

configure.ac

SuCicada marked this pull request as ready for review April 8, 2024 09:50

SuCicada changed the title ~~darwin: Enhance memory metrics support for Apple Silicon (ARM64)~~ darwin: Fix memory metrics support for Apple Silicon (ARM64) Apr 8, 2024

BenBE added bug 🐛 Something isn't working MacOS 🍏 MacOS / Darwin related issues labels Apr 8, 2024

BenBE added this to the 3.4.0 milestone Apr 8, 2024

BenBE linked an issue Apr 8, 2024 that may be closed by this pull request

M1 Mac Ram Usage Incorrect for Application Memory (System Ram, and M1 GPU Ram) #631

Open

Explorer09 reviewed Apr 8, 2024

View reviewed changes

SuCicada deleted the branch htop-dev:main April 10, 2024 02:21

SuCicada closed this Apr 10, 2024

SuCicada deleted the main branch April 10, 2024 02:21

SuCicada restored the main branch April 10, 2024 02:21

SuCicada reopened this Apr 10, 2024

SuCicada force-pushed the main branch from bfa5269 to 72afdaa Compare April 10, 2024 02:26

SuCicada force-pushed the main branch from 09c86e2 to ee496b8 Compare April 11, 2024 09:44

BenBE reviewed Apr 11, 2024

View reviewed changes

Explorer09 reviewed Apr 11, 2024

View reviewed changes

SuCicada force-pushed the main branch from ee496b8 to abf2226 Compare April 12, 2024 02:08

BenBE requested changes Apr 12, 2024

View reviewed changes

SuCicada mentioned this pull request Apr 12, 2024

Consult memory computing issues🙏 exelban/stats#1890

Closed

Explorer09 reviewed Apr 12, 2024

View reviewed changes

configure.ac Show resolved Hide resolved

darwin: Enhance memory metrics support for Apple Silicon (ARM64)

ddd3aba

SuCicada force-pushed the main branch from abf2226 to ddd3aba Compare April 12, 2024 09:52

Explorer09 mentioned this pull request May 18, 2024

not showing accurate memory usage (MacOS Ventura) #1477

Open

BenBE linked an issue May 19, 2024 that may be closed by this pull request

not showing accurate memory usage (MacOS Ventura) #1477

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

darwin: Fix memory metrics support for Apple Silicon (ARM64) #1439

darwin: Fix memory metrics support for Apple Silicon (ARM64) #1439

SuCicada commented Apr 8, 2024

Explorer09 Apr 8, 2024

SuCicada Apr 9, 2024

Explorer09 Apr 9, 2024

BenBE Apr 9, 2024

Explorer09 Apr 9, 2024

SuCicada Apr 10, 2024

BenBE Apr 10, 2024

Explorer09 Apr 8, 2024

SuCicada Apr 9, 2024

Explorer09 Apr 8, 2024

Explorer09 Apr 8, 2024

Explorer09 Apr 8, 2024

BenBE commented Apr 9, 2024

SuCicada commented Apr 10, 2024 •

edited

BenBE commented Apr 10, 2024

SuCicada commented Apr 11, 2024

SuCicada commented Apr 11, 2024

BenBE Apr 11, 2024

Explorer09 Apr 11, 2024

BenBE Apr 11, 2024

Explorer09 Apr 11, 2024

SuCicada commented Apr 11, 2024

BenBE commented Apr 11, 2024

Explorer09 commented Apr 11, 2024

BenBE commented Apr 11, 2024

SuCicada commented Apr 12, 2024

BenBE left a comment

BenBE Apr 12, 2024

SuCicada Apr 12, 2024

darwin: Fix memory metrics support for Apple Silicon (ARM64) #1439

Are you sure you want to change the base?

darwin: Fix memory metrics support for Apple Silicon (ARM64) #1439

Conversation

SuCicada commented Apr 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenBE commented Apr 9, 2024

SuCicada commented Apr 10, 2024 • edited

BenBE commented Apr 10, 2024

SuCicada commented Apr 11, 2024

SuCicada commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuCicada commented Apr 11, 2024

BenBE commented Apr 11, 2024

Explorer09 commented Apr 11, 2024

BenBE commented Apr 11, 2024

SuCicada commented Apr 12, 2024

BenBE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuCicada commented Apr 10, 2024 •

edited