Corrections to the MemoryMeter for all non-Linux platforms #1824
Conversation
natoscott
force-pushed
the
freebsd-memory
branch
from
2736ba0 to
0e60ca3
Compare
natoscott
left a comment
natoscott
left a comment
There was a problem hiding this comment.
I'm not sure representing buffers the way you have is ideal (I note the current implementation sets buffer=0 and says "not applicable to FreeBSD") - it is going to be difficult to maintain over time, and likely to be confusing to users.
There are some options, however. One option would be adding an additional MemoryMeterValues enum category as a container for this additional FreeBSD class of memory (which is always zero on other platforms - a bit like "compressed" is always zero on FreeBSD) - which can exist in this used/cached duality without ifdefs and in a way that can be understood by users (?).
Alternatively, if we really cannot shoe-horn the FreeBSD memory model into the high-level used/shared/cached/buffers/free model of the MemoryMeter code, we could have a freebsd/MemoryMeter.[ch] implementation of this Meter that behaves in a way that is more meaningful for that platform, and not use the MemoryMeter code from all other platforms. Seems a bit extreme to me, but its an option.
|
@Explorer09 @PierreMarieBaty it's a complicated situation, which Pierre-Marie posted the following to over in issue #1725
This new interpretation of "buffers" as something that is also "used" isn't really working out too well, IMO from looking at the patch - the increasing ifdef use is a big red flag. I'll update the PR and add some review comments - let's continue this discussion there.
That is because, as I said, each operating system has its own memory classes, with its own terminology for each, and sometimes the meaning of a label (here, "buffers") differs from operating system A to operating system B. I can't stress this out enough. The meaning (and thus the handling - whether to count it as "used" memory or not, whether to display it or not when "show cache" is ticked) of class "X" is simply not the same on OS A compared to OS B. For instance, the meaning of "buffered" memory on FreeBSD is not the same as on Linux, and not the same as on e.g. NetBSD. In FreeBSD you count "buffered" memory among used memory, because it's a subclass of the "wired" memory. In Linux you don't count "buffered" memory among the used set, because "buffered" has a different meaning there. I'm not even talking about other operating systems such as QNX here because you'd probably be pulling your hair... As I said elsewhere, trying to map each of these OS-specific classes into a unified set of categories is wrong design IMO, and doomed to end like this. Sooner or later you'll hit an OS where the meaning of a label will be totally different, and that class will need a special treatment different of the rest of the crowd. What to do then? Add an OS-specific new memory category? That defeats the purpose of the unified list (portability). Rename the label just for that OS? That's basically moving the #ifdefs elsewhere. Or redesign the whole memory presentation logic in a totally OS-specific way? Besides, I restate that the purpose of htop is to be an "improved top", and in this regard - in an ideal world - it should be critical that the semantics of the host OS be respected when displaying memory information. Else the users will be confused. For example you'd be tempted on FreeBSD to stuff the "buffered" memory into htop's "cache" category. Yet htop also has a "buffered" category. The FreeBSD user will look at htop's memory readings and think: where's my buffered memory gone? So, I think we're all cornered to admit the only right choice is the most unpleasant one here... As for me, I now have a working htop memory meter for FreeBSD and I'm happy with it - even if the code isn't the clean design paradise the maintainers would dream of, and even if the semantics of its memory classes isn't quite the FreeBSD one, it gets the job done and the categories are mapped almost correctly (as far as FreeBSD is concerned, in htop I'd separate "wired" and "active" memory with a red color for the "wired" one as it's almost always kernel pages whereas "active" are userland pages, but that'll be for another day.) |
|
@PierreMarieBaty I'm largely agreeing with you, right up until you say we are cornered and have to accept a mediocre solution for FreeBSD. If we compromise in the way this PR suggests now, the situation is unlikely to improve any time soon.
Today is another day! Let's improve this for all FreeBSD htop users. As I described earlier, and you wrote here...
Yes, these questions are spot on (the answers don't give the complete picture, but no matter). As I see it, there are two practical approaches:
My preference is to start with option 1 and fallback to option 2. Either way, the end result should be the improved Memory reporting on FreeBSD that you described. |
|
@PierreMarieBaty @natoscott I need some more explanation on the differences between "buffer" memory as used in Linux vs. "buffer" memory used in FreeBSD. For now, your arguments do not convince me. Yes, the same terminology used differently in different operating systems can be confusing, but neither of the solutions you (in particular natoscott) proposed are usable in the long run. No, there shouldn't be any OS-specific category for MemoryMeter. The main reason is that it would be hellish to document them in the man page, where there would be no OS-specific descriptions. If I understand it correctly, the "buffer" memory in FreeBSD is a subcategory of "used" memory and cannot be freed out by the kernel (on demand), then how about this: Remove the "buffer" memory from display for FreeBSD htop, count the "buffer" memory as part of "used", and add in-code comments to explain the reason this decision. |
|
There are already operating-system specific categories in MemoryMeter (compressed). And FWLIW, "buffers" doesn't mean anything sensible anymore for at least two modern Linux filesystems (btrfs and xfs). There's no reason not to add categories to help people make sense of memory use on FreeBSD. |
There is no documentation of the Memory Meter on the man page, or any of the categories of memory it displays. |
|
I need some more explanation on the differences between "buffer" memory as used in Linux vs. "buffer" memory used in FreeBSD.
I don’t know what this means in Linux. In FreeBSD, this is the part of kernel memory where all disk I/O are processed for all filesystems that have a "soft updates"-like caching mechanism - except for ZFS, where FreeBSD’s top maintains a separate line for ZFS stats. Buffer memory is not freeable on FreeBSD and must be counted as "used" memory.
For now, your arguments do not convince me. Yes, the same terminology used differently in different operating systems can be confusing, but neither of the solutions you (in particular natoscott) proposed are usable in the long run. No, there shouldn't be any OS-specific category for MemoryMeter. The main reason is that it would be hellish to document them in the man page, where there would be no OS-specific descriptions.
In my humble opinion, if htop’s man page covers memory readings, then, precisely, the man page should say different things for different operating systems.
The question boils down to this : was htop meant to be an "improved top", or just yet another generic common-ground multiplatform program?
If htop is an improved top, it should not say less than what the OS-specific top says. Else, the promise is not kept.
If I understand it correctly, the "buffer" memory in FreeBSD is a subcategory of "used" memory and cannot be freed out by the kernel (on demand), then how about this: Remove the "buffer" memory from display for FreeBSD htop, count the "buffer" memory as part of "used", and add in-code comments to explain the reason this decision.
That’s doing less than what top says, and that’s doing less than what the patch does, whereas the possibility exists. At this point, why not just leave only 2 categories for all the operating systems, used and free? Everyone would agree, and that would be OS-agnostic and portable.
From the very moment you add extra categories to "used" and "free", you are OS-specific.
Let’s be honest: htop was initially a Linux tool, and the memory classes design, as it is, wasn’t made with portability in mind but simply to reflect Linux’s memory classes, so saying "this and that will make the code not OS-agnostic", on a part of the code that really isn’t from day one, can’t be a real argument.
natoscott said:
we instead create a freebsd/MemoryMeter.[ch] that implements the optimal FreeBSD memory meter layouts. Then for FreeBSD builds we use this implementation rather than the cross-platform one (via some straightforward Makefile.am changes). We use a similar approach in other places already - see linux/*Meter* source code for example, which provides Linux-specific Meters.
I agree. Every supported OS flavor of htop should do that.
Memory information cannot be OS-agnostic and it is a dead end to continue pushing in the same direction, which has never been an OS-agnostic one but the simple legacy of a Linux-centric tool.
|
|
FYI I refreshed my memory on what "buffered" means for FreeBSD vs. Linux. In a nutshell:
FreeBSD buffers == Linux buffers+cache
FreeBSD uses a unified cache and both categories are not separable.
|
|
| FreeBSD uses a unified cache and both categories are not separable. This is often true on Linux also - xfs and btrfs use a unified page cache approach. TBH, "buffers" could probably be merged into "cached" on Linux as well. |
|
With the additional caveat that on one OS that cache should be counted as "free" memory and on the other OS it should be counted as "used". And on a third one, well.. it’ll depend.
I see no other way out than to go full OS-specific here.
|
I'd say it's the latter - a generic multiplatform program. Of course you can argue that htop was a Linux utility - I agree, but that doesn't change the fact that there was no promise in the first place. By the way, there's an entry in htop's FAQ that explains a difference between
Ditto. There is no promise.
Oh hell. I've worked with a mess when GPUMeter was OS-specific before. Someone proposed a GPUMeter for macOS/Darwin, and I had to rework that meter to make it usable OS-agnostic. Please look at the code of GPUMeter.{c,h} (for example cddacad) for what I mean.
I kind of understood the situation.
|
|
The FreeBSD package maintainer wrote an incorrect package description then:
I thought I read that claim that "htop was an enhanced version of top" elsewhere and that was somewhat endorsed by the htop maintainers. My bad if that's never been the case. I have been misled. (edit: trying to find where I got this wrong information in the first place, I searched Google for the exact phrase "htop is an enhanced version of top". It yielded 8 pages. It looks like everybody and his cat are firmly convinced of it. Even the Wikipedia article gets it wrong! Alright, I've been misled, but I feel less alone now... 😇) (edit 2: aha, found it! On Hisham Muhammad's old page @ https://web.archive.org/web/20140121201425/http://hisham.hm/htop/index.php?page=faq :
Anything that doesn't feel like a functional regression from the patch looks good to me. I'm okay with it. What you did for the GPU meter was a good thing. I don't believe doing the same thing for memory would be more complicated. I might propose a patch doing this for all the platforms I can compile htop on (all except Solaris) if I can secure enough time these days for that. |
One more question: Does FreeBSD |
You could argue on the semantics here, but "enhanced" only means there are some added features or other changes. Strictly speaking it's not automatically better suited for every user and use case …
The origins of htop are rooted in the experiments from a Brazilian developer exploring how top works, guided by a design philosophy we still continue to honor. While htop focuses on exposing system details, it doesn’t strive to be an exact, fully faithful clone.
In the link above @hishamhm talks a bit more about the origins of htop. And it's a mixture of curiosity and wanting to improve over the existing tools. How much of this means following each OS to the letter is practically speaking an open question.
That's also mostly my stand on things. The assignments should be reasonable, predictable and sensible. If they furthermore are also somewhat consistent across platforms ("used" doesn't randomly mean "swapped out filesystem cache" on some platform) it's even better. Thus if we can somehow find a good middle ground of categories of memory metrics we want/need to show to the user, this would be ideal. Looking into things, we can even add the "Wired"/"Fixed" category on Linux, as that's a category of allocations that's currently missing entirely on Linux.
Looking forward. And no need to hurry. |
|
With some code refactoring, I removed the need of the https://github.com/Explorer09/htop-1/commits/freebsd-memory/ The "used" numbers in MemoryMeter are now handled in platform-specific code, and the behavior of For a UX reason, in FreeBSD, if the |
|
IMO this is a good step in the right direction ! About your earlier question, FreeBSD's top has no explicit notion of "used" memory: all the available memory is supposed to be used all the time, even if the reality is more subtle. But it is correct to say that "buffer" memory, being not releasable, counts as "used" memory in the sense any other traditional memory monitoring tool understands it. |
|
@PierreMarieBaty I'm not asking about the "used" memory in FreeBSD top (I know FreeBSD doesn't use that term, but "Active" and "Wired"). My question was whether the "Wired" memory shown there includes the "Buf" memory or not. |
Yes. In FreeBSD's top, the number displayed next to "Wired" includes the number displayed next to "Buf". This can be confusing. In other words, "Buf" is a subset of "Wired". |
Oh well. That's gonna be tricky. You can notice that in my implementation (Explorer09@f8e62e6), the "used" memory (in Text meter mode) will not include the "buffers" number unless the users turns off the "show cached memory" option. This is to ease the bar drawing. I don't wish users complain about that. htop's memory display cannot be 1:1 matching FreeBSD top's display. |
Not being 1:1 is okay (at ~200-300Mb off nobody will complain) but with that behaviour you're likely to get significantly different numbers, and new bug reports. |
htop's memory display is not going to be fine-tuned to fit all operating systems' needs. |
|
| htop's memory display cannot be 1:1 matching FreeBSD top's display. Sure it can. I'm not saying it has to, though (looks like good progress toward the first option I described has been made - so I'm all for that), but if what top displays is the exact information that makes most sense and provides the best insights to performance for FreeBSD users, then this is what htop should display. Its just code, we can change internal mappings, macros, colours, etc as we need to. |
I decided it was best to strike the iron while it's hot, and I'm 90% done. My htop now displays the OS-specific memory classes for Darwin/macOS, DragonflyBSD, FreeBSD, Linux (no aspect change, just refactoring on this one), NetBSD and OpenBSD. This fixed several issues as a side-effect. In each platform-specific folder 4 files have changed, and this cleans the code a lot. Much less math: the numbers retrieved by sysctls (or from a VM stats struct) are just passed around. And the help page also displays the OS-specific memory classes in the memory bar, selectively whether their OS-specific "this category can be assimilated to a maskable cache" flag is set, and with their OS-specific color. No #ifdefs anywhere. My MemoryMeter.h now looks like this: And in each platform's Platform.c, there is something like (here e.g. for Darwin): The platform-agnostic MemoryMeter.c is now very simple: I'm currently fiddling with an OpenIndiana VM to mount a NFS share on which my htop source tree is, to be able to provide a complete patch that will compile on Solaris. Argh, this is really $*€ me off... Does anyone have some experience with this OS? I could mount that NFS share from all my other Linux and *BSD VMs, but Solaris apparently decided that root was not trustworthy. |
|
@PierreMarieBaty You're over-engineering things. And there's one problem with your approach that, color scheme designers (theme designers) are losing the capability of customizing colors for the memory fields. That's why I said earlier this was a bad idea. Unlike GPU meter where items are by driver (or, in the user's perspective, GPU vendor), memory meter has common categories that are best colored consistently across platforms. Even we can tune the wording for each category by platform, let's not mess up with the color scheme or the ordering of the categories (e.g. "cached" memory categories should be after the "used" memory categories). |
Nope. The DYNAMIC_xxxx colors are themable - and already themed. Look in CRT.c, it's already done. Even on black/white displays, they fallback to different ASCII characters. I tested it on all the OSes on which I could compile.
I'm sorry to differ but no it has not. Apart from "used" and "free", it has not. I can only advise you to have a dive in it yourself: install these operating systems in a VM, look at what "top" displays, look at their ways to extract memory information, so as to understand how they do thing. I have the strong feeling that you are under the illusion that every OS must be doing it in a roughly similar way than what Windows and Linux do. Nothing could be more wrong...
No again, because "cached" memory will have a totally different meaning across 3 different systems! On some OS it's reclaimable, on some others it's not. On some OS it's optional, on some others it's mandatory. On some it caches filesystem metadata only, on some others it caches buffers, on some others it's part of a unified cache, on some others it's ONE of the various caching mechanisms, on some others it's part of a memory zone... I really don't want to sound offensive, but you need to get more understanding on how differently the operating systems grasp memory management! |
I will even go further: even these 2 categories are not generalizable. Look at how Darwin computes "used" memory. It counts inactive pages, i.e. allocations that have been freed and are literally no longer used by any program, in the used set! |
I bet you didn't have the idea on where the ASCII characters drawn on the bar meter are defined.
You said "used" and "free". I take that as your admission that there are common categories. Yes you can make subcategories under "used", e.g. kernel pages vs. process pages, and some pages are even shared across processed. But objectively speaking they are all "used" categories.
The best thing we should do is to examine why the "show cached memory" option was introduced in the first place. From the htop FAQ:
Buffers and Cache memory in the Linux kernel are reclaimable and not actually "unavailable" for reallocating to processes. It was a clever mechanism for Linux to prefetch data and put those memory space to use or otherwise it would be waste. Since then we had bug reports requesting those two fields be hidden in Graph mode. So the option wasn't actually about "cache", but "cache memory that is reclaimable on demand". With this purpose in mind, "cache" memory in operating systems that is not reclaimable shouldn't be covered under the toggle option. Note that I don't care what kind of "cache" the kernels are going to use for, what matters is whether the memory is reclaimable or not. That is, when a new process starts, whether the kernel can flush those cache pages immediately and reallocate them to processes that need them. |
|
Hi folks, I see some old quotes were dug up, which is always a fun exercise, so I thought I'd bring more context, in case this is in any way helpful now or in the future. About the "quote-unquote better top" thing. Let's keep a few things in mind that will help put those quotes in context:
So let's take these old quotes and the conversation around them charitably and not tear them apart for semantic gotchas — but to try to clarify: To the best of my recollection (and also from what I see in the quotes above), every time I compared htop to top, I never said that htop is a top (better or otherwise, whatever that means). I used aspirational terms such as "aims to be a better top", "tried to make a better top", etc., and "better top" in that context meant only "a process viewer that I enjoyed using more that the circa-2004 version of top that shipped in Linux distros by default". (At the risk of using a flawed metaphor, think like if someone is running for office and says they "want to be a better mayor", that doesn't mean they want to be a backwards-compatible version of the previous mayor. :) — what I mean is that I looked at top as a category of application, not a particular piece of software that I was emulating/extending, since to the best of my knowledge there were quite different different pieces of code called "top" back then, and Wikipedia seems to back my memory up). But specifically, I am pretty sure that I never referred to htop as "a version of top", let alone "an enhanced version of top", like that FreeBSD ports quote above. That is some downstream maintainer's assessment, which I would consider technically incorrect. I know for a fact that I never wanted to deal with "top does X, therefore htop must do X"-type reports, therefore I never claimed compatibility. Also, over time, I recall I did tone down the comparisons with top; first, because there's no point in being confrontational with another piece of free software (I was in my early 20s then, I know better now — today, it's endearing to see the next generation come along and say "htop? xyztop is where it's at now!" (cue The Lion King's "Circle of Life" song in the background)). Second, because top also changed over time (in many ways becoming more similar to htop!). Imagine if one could go back in time and show 2004-me that top could look like this in 2025, down to the meters drawn I guess the lesson there is that no piece of software is a static goalpost, and everyone can benefit from each other's past and current work. When the time came to add portability to htop, I did the same I did to the UI in general. When I designed the htop UI, I did pick some elements from top for the sake for users' and (and my own!) familiarity, but deviated from it when I felt it made sense — some key bindings matched, but not others, default columns, etc. When I first implemented support for other OSes, I did the same, with the added complexity bit that by then htop was well-established enough so that many users looking for it in other OSes were already familiar with it on Linux, and I believe they expected some degree of consistency across systems (I certainly did!). So, yes, there was a balancing act there between addressing the differences in data provided by each new OS, but also to the familiarity expectations of htop users across OSes. Having said that, my home platform was (and still is!) Linux, so the ports often relied from the userbase, both in terms of code and behavior feedback. But ultimately everything is a judgment call. All I wanted to clarify is that htop was never meant as an "(extended) top clone", but just a process viewer that's enjoyable to use, providing an experience more similar to that from GUI process viewers in other OSes. This is made clearer by the fact that I never kept chasing new top features as other top implementations changed over time, but just kept evolving htop following its own path. Regarding this PR specifically I have nothing to comment on, and as usual I leave it to the amazing htop dev team! Cheers <3 |
|
| I have emailed you the tarball. Thank you for your interest in it, it's a relief to know these few hours of coding for free might benefit others. I've looked over it now, thanks @PierreMarieBaty - I believe its headed in the right direction. I'll finish the solaris and pcp porting aspects and push it here shortly. Thanks again for sticking with it. @BenBE do you recall why the MEMORY_METER_AVAILABLE category was added in that unusual way OOC? (i.e. why does it not have a color like all the other categories? the static_assert aspect, and the "don't display in bar mode" comment are all quite difficult to follow for the casual observer - all handling Linux-specific quirks I guess). |
|
Will have to take a closer look, but IIRC we had some issues with how memory for tmpfs is showing up and which part of the memory could be swapped out/discarded easily. Can tell more later when I had a chance to revisit that section of the code. |
The discussion in #281 might be related. |
FreeBSD required several corrections to memory calculations that proved impossible to reconcile with existing memory categories. Resolves htop-dev#1725
Extensions to the changes Pierre-Marie made to generalise the memory meter for different platforms: - allow memory meter colors to be reflected in the different color schemes - use a proper color setting for Linux available memory instead of "text" - use sysname metric in PCP to drive correct memory selection and display - add missing PCP metrics for several missing Darwin memory categories - provide Solaris support for setting its own memory categories (noticed previous "buffers" and "cached" on Solaris to be a bit of a guess, and incorrectly ignoring "free" memory) - remove comments relating to platform-specific swap metrics on platforms that do not support those metrics.
natoscott
force-pushed
the
freebsd-memory
branch
from
0e60ca3 to
27ca7b5
Compare
|
I've updated this PR now with the platform-driven MemoryMeter ideas discussed here (and mostly implemented by @PierreMarieBaty as reflected in the commits), while also ensuring the color-scheme aspect is unchanged and adding PCP and Solaris platorms (code from me). Having spent a bunch of time reviewing each of the platforms memory implementations, I'm convinced by Pierre-Marie's points - this to me is clearly the superior approach. It was quite surprising (shocking even) to see some of the attempts to map missing and just plain different information onto the previous memory categories... IMO this is 100x cleaner and no longer misleading on all the non-Linux platforms. |
natoscott
changed the title
I don't think this is anything cleaner. And I find technical issues regarding its extensibility.
typedef enum MemoryMetric_ {
// Linux
MEMORY_CLASS_USED = 0,
MEMORY_CLASS_SHARED = 1,
MEMORY_CLASS_BUFFERS = 2,
MEMORY_CLASS_CACHE = 3,
MEMORY_CLASS_COMPRESSED = 4,
MEMORY_CLASS_AVAILABLE = 5,
// Darwin
MEMORY_CLASS_WIRED = 0,
MEMORY_CLASS_SPECULATIVE = 1,
MEMORY_CLASS_ACTIVE = 2,
MEMORY_CLASS_PURGEABLE = 3,
MEMORY_CLASS_INACTIVE = 5,
// Maximum
MEMORY_CLASS_LIMIT = 6
} MemoryMetric;... Then there should be little problems to define a color scheme to cover the whole union set, instead of
|
Despite that opinion I'm hoping you've noted all the things that have been done now which previously said were "not possible" - htop memory meter can match top exactly, if that's what is best for a platform. And the color schemes work exactly as they did before. For me, there's no doubt definitely is a better way to go, after reviewing the stats we have from the FreeBSD and Solaris virtual memory subsystems. The values displayed make sense for users on that platform, and can sum up to total memory properly. So we shouldn't see these issues anymore when users are trying to make sense of conflicting reports from different tools.
This is the number 1 issue? OK, I think we're getting pretty close then.
That's correct - pcp-htop can monitor a Linux machine remotely from macOS and vice-versa. And in due course, when pcp-htop --archive option is completed, we'll need to handle recorded data from one platform being replayed on the other. IMO we should focus on making the other platforms work as well as possible, and then consider how to make PCP fit into the htop code subsequently.
There's no reason to do that. It's much simpler the way I have it here IMO, than trying to create a full union in the color scheme. It also maps 1:1 with how the colors worked before, so this change is a no-op for Linux users - htop just works better on all the other platforms now, which is a great outcome.
You mentioned it, but TBH that's not a problem in the implementation. Its more a problem of refusing to acknowledge the FreeBSD issues that Pierre-Marie explained originally.
Worth consideration subsequently, after we've tackled the bugs in the existing code through this PR - there's certainly ways to tackle that but doing so now, without any concrete use case, seems like feature creep to me. OOC, does any platform actually export such metrics?
Heh, so ... not serious on the other parts? ;-)
Yep, quite possibly. @BenBE has a good eye for these things too, so I'll wait to hear his thoughts before digging into this aspect further. Thanks for taking a look! I'll be away for a few weeks, but please continue the discussion and I'll look into any potential improvements identified. |
Oh my. I think you misinterpreted what I mean by "not possible". What I mean was not without so many disruptive changes. If people want htop to behave exactly like other And this PR just showed how disruptive the change could be. This PR has gone beyond just fixing the FreeBSD memory display bug and is now trying to dictate how memory categories should display for each OS, which IMHO doesn't have a "one, true" way to do it. I might have rejected the number discrepancies between htop and other tops as a non-bug. Pierre-Marie identified one problem with FreeBSD (which is one bug), but there is no problem with the memory meter display on the other OSes -- at least not until I see a bug report about it. It's a solution in search of a problem.
No. You didn't realise how disruptive this has become. When I take screenshots of htop, and identify the colors of the memory bars, without looking at which OS the user uses, I used to able to say that brown color 🟫 denotes caches, blue 🟦 denote buffers and gray 🩶 denotes compressed pages. It's also useful in documentations (including blog posts of people teaching users how to use htop)! Now after this PR change, the cross-OS color consistency of memory items is gone! We became forced to adopt what color scheme that the OS's native top came up with, or, in case the OS's top does not suggest a color scheme, be more arbitrary on the color choices. The latter is a negative to me.
By "better" please define it. I deny this is "more useful" as Pierre-Marie suggested (and now you too), as you and he didn't come up with a good definition for it. I mean how much does this align with htop project's goals? I'm not sure htop maintainers have thoroughly discussed this part -- it seemed like a one-person suggestion backed by one of the maintainers without consulting the whole team. (I know @hishamhm has resigned from maintaining htop now, but I mention him to take a look. If this were Hisham's project I doubt this kind of change could match the old vision of htop. It seems like something is changed after the maintainership change. It now makes me wonder whether I should keep contributing (as a volunteer) to htop anymore.)
Same reply as above. (Now it's a hell to document which color does what in manuals and web tutorials.)
It's a UX issue, which conflicts with what Pierre-Marie tried to do. I say this creates a number inconsistency on the graph display. (Users has a reason to expect the "total used" number on the bar mode matches what is plotted on the graph mode, when the "show cached memory" is off.) In short, you created another issue while trying a "fix" to another.
I mentioned this for the sake of the "extensibility" argument. Outside htop, I had maintained a code like this in my formerly employed job, and I can tell how inflexible this structure can be on the software design. I just don't want that to happen on an open source project like htop. So, what if, other than the
I believe with the callback functionality we can ditch the whole Specifically three callbacks:
You're welcome. |
|
|
||
| void Platform_setMemoryValues(Meter* this) { | ||
| const Machine* host = this->host; | ||
| const LinuxMachine* lhost = (const LinuxMachine*) host; |
There was a problem hiding this comment.
The "available" memory in Linux used to be never plotted in bar and graph. For now, without an explicit setting of this->curItems = 5, the "available" memory becomes drawn in the bar meter mode! This is a regression for me.
Please also take note of this comment of mine about the overall issue with the code design - specifically about the inextensibility of the new MemoryClass structure.
There was a problem hiding this comment.
Good one. It looks like this array got one slot more than necessary. The fact that countsAsCache is false for the "cache" category and true for the "available" one that follows it hints that the "available" slot should have been deleted and the original intent was to set countsAsCache to true for the "cache" category. AFAICT the "available" category doesn't count in any memory calculation in the Linux part of the code. The availableMem member in the LinuxMachine struct can also be deleted as well as all related code.
About your second paragraph, this objection is a joke. When the need arises, you wrap the array in a struct, and there you go.
| const LinuxMachine* lhost = (const LinuxMachine*) host; | ||
|
|
||
| this->total = lhost->totalMem; | ||
| this->total = host->totalMem; |
There was a problem hiding this comment.
Moving the totalMem member from {Platform}Machine to Machine is better addressed as a separate commit. This small change is less destructive than the other changes on the memory meter code.
| proc->m_resident = kproc->p_vm_rssize; | ||
|
|
||
| proc->percent_mem = (proc->m_resident * nhost->pageSizeKB) / (double)(nhost->totalMem) * 100.0; | ||
| proc->percent_mem = (proc->m_resident * nhost->pageSizeKB) / (double)(host->totalMem) * 100.0; |
There was a problem hiding this comment.
Ditto. (Make a separate commit for moving the totalMem member from {Platform}Machine to Machine.)
|
|
||
| extern const MemoryClass Platform_memoryClasses[]; | ||
|
|
||
| extern const unsigned int Platform_numberOfMemoryClasses; |
There was a problem hiding this comment.
For better optimization I would like this Platform_numberOfMemoryClasses to go away.
The purpose of that global constant can be replaced with ARRAYSIZE(Platform_memoryClasses) even though letting the sizeof operator work would require exposing the table to global scope.
There was a problem hiding this comment.
Agreed.
(edit) ... in an ideal world I agree. But since Platform_memoryClasses is declared as an extern const array of unspecified size (because that size is platform-specific), the compiler would error out with things like this:
error: invalid application of 'sizeof' to an incomplete type 'const MemoryClass[]' (aka 'const struct MemoryClass_s[]')
So I think it's best to leave it this way. Besides, that's already what the htop code does for other platform-specific fixed size arrays, e.g. Platform_signals[] whose size is given by a const unsigned int Platform_numberOfSignals = ARRAYSIZE(Platform_signals).
| #define MEMORY_CLASS_INACTIVE 5 | ||
| { .label = "inactive", .countsAsUsed = true, .countsAsCache = true, .color = DYNAMIC_GRAY }, // pages no longer used; macOS counts them as "used" anyway... | ||
| { .label = "wired", .countsAsUsed = true, .countsAsCache = false, .color = MEMORY_1 }, // pages wired down to physical memory (kernel) | ||
| { .label = "speculative", .countsAsUsed = true, .countsAsCache = true, .color = MEMORY_2 }, // readahead optimization caches |
There was a problem hiding this comment.
If you're bothered to make an enum, it shouldn't be a problem to fix the table index like this:
| { .label = "speculative", .countsAsUsed = true, .countsAsCache = true, .color = MEMORY_2 }, // readahead optimization caches | |
| [MEMORY_CLASS_SPECULATIVE] = { .label = "speculative", .countsAsUsed = true, .countsAsCache = true, .color = MEMORY_2 }, // readahead optimization caches |
There was a problem hiding this comment.
Agreed. A C99 compiler is required to compile htop already, so it's logical to use C99 initializers.
There was a problem hiding this comment.
Especially using the array index notation is used elsewhere already. Makes things more readable.
There was a problem hiding this comment.
Now I'm reading this again. I personally have objections to the enum whose values are platform specific. A problem with such happed before in my previous job, and I remember I solved that with a struct definition and offsets rather than enum and array.
| super->usedSwap = super->totalSwap - swapFreeMem - super->cachedSwap; | ||
| } | ||
|
|
||
| static void PCPMachine_updateDarwinMemoryInfo(PCPMachine* this, Settings* settings) { |
There was a problem hiding this comment.
Any way to reuse the Platform_setMemoryValues code in darwin/Platform.c for this?
I smell a WET (write everything twice). For every platform besides PCP, there now seems to be two copies of memory calculation formula, and I don't like this.
There was a problem hiding this comment.
Agreed. Platform-specific callback functions could be used here.
| free(sysname.cp); | ||
| } | ||
|
|
||
| static void PCPMachine_updateLinuxMemoryInfo(PCPMachine* this) { |
There was a problem hiding this comment.
Ditto. (Consider reusing the Platform_setMemoryValues code in linux/Platform.c and don't write everything twice (WET).)
|
I'll reply only to the points that make sense.
All optimizing compilers unroll these sort of loops at build time, unless you build with -O0 (debug). This is demonstrable in decompiled code. The loop over a fixed array here is akin to a preprocessor technique and exploited for code brevity and readability.
Correct. What is exposed here is an array. Arrays are not structs. Woe is me! I'm doomed! Now seriously... Ain't life beautiful? |
Well, I don't deny that the compiler can unroll it. (I'm not requesting an assembly or binary proof of that.) But my argument of this being an anti-pattern remains. (Note: If this were a table that allows a random access of records/rows, then your structure definition can make sense. If the table is only meant to be accessed sequentially, then the code would be more maintainable by turning each table record into a function or macro call, unrolling the loop, and eliminating the structure definition altogether. Hence it's an anti-pattern.)
Variable length arrays are not allowed in non-last members in a structure definition in C. You probably would want to use a pointer instead. And I know htop has a lot of "subclass" (pseudo-OOP) definitions, so your idea is not new. But you missed what I'm saying: If I want to add (1) Update the This is error prone if you're not careful (see 7a9eceb, where I did the thing like this before.) |
It's no longer an anti-pattern since the compiler takes care of it. You don't deny it. And you can't deny either that it's shorter and more readable written this way than otherwise - which would be impossible anyway because you can't unroll in a platform-agnostic part of the code a fixed array whose size is defined in the platform-specific part of the code. So it's no longer an argument.
True. I just copy/pasted to show what should be done to extend the data representation to other types. If that really matters, the memory classes array should be added to the end of the struct - but is that important?
Of course. I'm simply reminding you that the solution to the problem you raised is 1°) simple, and 2°) in plenty of places in htop's code already.
Indeed the sole argument I see for which you brake on all wheels is this one: "it's disruptive and it complicates things!" I agree. But sticking to the current situation doesn't solve the problem. The change I proposed does - and I don't want to enter again in that discussion where you deny the existence of any problem, or prepare for a socratic list of questions, because I came to the conviction that you were not of good faith on this matter. Eh. Going cross-platform necessarily makes the code more complicated. Is the role of a maintainer to be a museum curator? |
No. It's not an anti-pattern when there is a necessity to access records out of order, which is not the case here. Whether the compiler can unroll the loop is irrelevant. Of course the compiler can unroll a loop-switch sequence, the problem is future developers / new contributors may miss that the table is only meant to be accessed sequentially. It's not an event queue where a loop-swich sequence is necessary (as the order of events can't be known ahead of time).
Use platform-specific functions! That's why I suggested the three "callback" functions in a comment above! They work like hooks so that platform-agnostic code can perform platform-specific actions without needing any table/structure along the way.
I don't want to argue about extending an anti-pattern with another anti-pattern. I've had enough.
What problem are you referring to? (Remind you that I said it's a "solution in search of a problem" before.)
I didn't observe the problem really. |
|
Yep, that's what I diagnosed. This discussion is a waste of time. I unsubscribe from this thread. Do as you wish. |
|
Perhaps Pierre-Marie can look carefully at the code reviews I've done to Nathan's commit above. Those are the technical issues (including a regression) with Pierre-Marie's approach going forward. |
|
|
||
| void Platform_setMemoryValues(Meter* this) { | ||
| const Machine* host = this->host; | ||
| const LinuxMachine* lhost = (const LinuxMachine*) host; |
There was a problem hiding this comment.
Good one. It looks like this array got one slot more than necessary. The fact that countsAsCache is false for the "cache" category and true for the "available" one that follows it hints that the "available" slot should have been deleted and the original intent was to set countsAsCache to true for the "cache" category. AFAICT the "available" category doesn't count in any memory calculation in the Linux part of the code. The availableMem member in the LinuxMachine struct can also be deleted as well as all related code.
About your second paragraph, this objection is a joke. When the need arises, you wrap the array in a struct, and there you go.
| const LinuxMachine* lhost = (const LinuxMachine*) host; | ||
|
|
||
| this->total = lhost->totalMem; | ||
| this->total = host->totalMem; |
| super->usedSwap = super->totalSwap - swapFreeMem - super->cachedSwap; | ||
| } | ||
|
|
||
| static void PCPMachine_updateDarwinMemoryInfo(PCPMachine* this, Settings* settings) { |
There was a problem hiding this comment.
Agreed. Platform-specific callback functions could be used here.
| free(sysname.cp); | ||
| } | ||
|
|
||
| static void PCPMachine_updateLinuxMemoryInfo(PCPMachine* this) { |
| MEMORY_3, | ||
| MEMORY_4, | ||
| MEMORY_5, | ||
| MEMORY_6 |
There was a problem hiding this comment.
I preferred when explicit color names were used (DYNAMIC_RED, etc). Explicit color names gave at least a chance for the contributors wishing to bring in support for another platform to map the same colors to roughly equivalent memory classes where that possibility exist, and use other colors where it doesn't. Having numbered opaque names is uninformative and thus unhelpful IMO. If you don't want explicit color names, perhaps give more descriptive labels that correspond to the ASCII character that's drawn by ncurses on monochrome terminals. e.g. MEMORY_CHAR_HASH, MEMORY_CHAR_DOLLAR, etc. It makes tracking similar choices easier.
There was a problem hiding this comment.
@PierreMarieBaty The ASCII characters drawn in monochrome mode of htop is not assigned by ncurses, but by htop itself. The character list is defined in Meter.c.
| mtr->values[MEMORY_CLASS_WIRED] = page_K * vm->wire_count; | ||
| if (settings->showCachedMemory) { | ||
| mtr->values[MEMORY_CLASS_SPECULATIVE] = page_K * vm->speculative_count; | ||
| mtr->values[MEMORY_CLASS_ACTIVE] = page_K * (vm->active_count - vm->purgeable_count - vm->external_page_count); // external pages are pages swapped out | ||
| mtr->values[MEMORY_CLASS_PURGEABLE] = page_K * vm->purgeable_count; // purgeable pages are flagged in the active pages | ||
| } | ||
| else { // if showCachedMemory is disabled, merge speculative and purgeable into the active pages | ||
| mtr->values[MEMORY_CLASS_SPECULATIVE] = 0; | ||
| mtr->values[MEMORY_CLASS_ACTIVE] = page_K * (vm->speculative_count + vm->active_count - vm->external_page_count); // external pages are pages swapped out | ||
| mtr->values[MEMORY_CLASS_PURGEABLE] = 0; | ||
| } | ||
| mtr->values[MEMORY_CLASS_COMPRESSED] = page_K * vm->compressor_page_count; | ||
| mtr->values[MEMORY_CLASS_INACTIVE] = page_K * vm->inactive_count; // for some reason macOS counts inactive pages in the "used" memory... |
There was a problem hiding this comment.
Since I'm using macOS right now (macOS Tahoe 26.1), I fail to recognize how the new set of memory categories would be better than the old set.
macOS top (screenshot below) doesn't display "speculative" or "inactive" memory.
There was a problem hiding this comment.
Define "better" please. See? You're not the only one who can play this game.
Here's my position on this point.
On macOS, as on systems that come with a bundled GUI, the canonical method for monitoring memory usage is not the command line (and thus not "top") but the system-supplied resource monitoring GUI app. On macOS this is Activity Monitor.
The new code yields readings that are consistent with what the Activity Monitor app displays. "top"'s readings are not. "top" is not the authority on macOS, Activity Monitor is.
Ergo, this code is better for macOS. Quod erat demonstrandum.
There was a problem hiding this comment.
Define "better" please. See? You're not the only one who can play this game.
Here's my position on this point.
On macOS, as on systems that come with a bundled GUI, the canonical method for monitoring memory usage is not the command line (and thus not "top") but the system-supplied resource monitoring GUI app. On macOS this is Activity Monitor.
The new code yields readings that are consistent with what the Activity Monitor app displays. "top"'s readings are not. "top" is not the authority on macOS, Activity Monitor is.
Ergo, this code is better for macOS. Quod erat demonstrandum.
Without code comments I can't tell you are referring to the Activity Monitor for the RAM fields.
And... while I don't want to argue about the definition of "better" (I didn't claim which reference is "better", by the way), here's a cropped screenshot of the Activity Monitor on my Mac, and please tell me where can I see the memory fields of, e.g. "speculative" memory?
|
With just a few days gone due to numerous other things to do I'm now faced with a huge wall of text to work through. And that's even before looking at the actual code changes (which I, like with the historic context about But first things first: The
An (compile-time) array that you can iterate over is usually more extensible than some weird fixed memory structure like we have at the moment with dozens of fields that you need to copy over and have to iterate over (unrolled) in very repeating patterns. Effectively having that compile time array usually helps to keep your code DRY, with extensibility basically depending on just adding another entry in that array. There are some anti-patterns related to doing your extensibility like this, like when you iterate over the array and based on which entry you process include one or more actions. But even then there's a good chance that this concentrated algorithm still might be more readable and maintainable than your average unrolled spaghetti code that these style guides are usually born from. With style guides you need to understand the why, not the how to make them effective. Similar with our style guide: The reason >90% of PRs fail the initial code review is not bad code, but consistency with existing code, which is there to minimize surprises when reading things later.. If all there is left in a PR is one blank at the wrong location, it's usually easier to just fix the PR directly. But in many cases that blank was only one of several other comments (usually technical ones), so you have these kinds of round-trips anyway. But back to topic.
Even though we effectively have "C with emulated classes" in the htop code, we should try to keep that interface slim. That said, while there is an argument to be made that Overall, when dealing with patterns you don't design your code to "follow a pattern", but rather when looking at your code and trying to analyze what it does, you group the way it's implemented into these patterns. With a code base that is well-maintained you'll then find, that all places will usually follow only a very limited set of patterns, but will follow these patterns consistently OR intentionally break a pattern i just a few distinct places. That's the difference between "programming by patterns" and "keeping your code organized": In organized code, patterns will emerge automatically. But okay, back to the question involved with this discourse: Having functions for each aspect of the update process for a meter makes things unnecessarily complex and complicated. If you need static labels: Put this into the static memory class information. If you need dynamic labels, you might just put it with the other platform update code, which also indicates the intent of providing a "consistent"/"atomic" view of the data. I think this wall of text to answer the wall of text from the previous few days should be enough for now; I'll likely write another wall of text once I actually had the time to look at the latest code changes. |
BenBE
left a comment
BenBE
left a comment
There was a problem hiding this comment.
Some minor code style notes plus one change to split the VM_STATISTICS64 change for darwin into its own commit. Apart from that LGTM.
| #ifdef HAVE_STRUCT_VM_STATISTICS64 | ||
| mach_msg_type_number_t info_size = HOST_VM_INFO64_COUNT; | ||
|
|
||
| if (host_statistics64(mach_host_self(), HOST_VM_INFO64, (host_info_t)&this->vm_stats, &info_size) != 0) { | ||
| CRT_fatalError("Unable to retrieve VM statistics64"); | ||
| } | ||
| #else | ||
| mach_msg_type_number_t info_size = HOST_VM_INFO_COUNT; | ||
|
|
||
| if (host_statistics(mach_host_self(), HOST_VM_INFO, (host_info_t)&this->vm_stats, &info_size) != 0) { | ||
| CRT_fatalError("Unable to retrieve VM statistics"); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
How does this affect the baseline OS requirements?
Also, can we split off this change into its own commit to make reviewing this change easier?
There was a problem hiding this comment.
We have someone who wants to port htop to iOS 6 (PR #1828) recently. And I don't like this PR contains changes that unnecessarily bump Darwin OS requirements.
I bet Nathan (@natoscott) and Pierre-Marie don't have the idea what they're doing. This doesn't count the fact that the memory display now becomes different from what's displayed in macOS top. (this comment)
There was a problem hiding this comment.
If you bet I have no idea what I'm doing, I could claim I know you don't have the skill level to understand most of what is being discussed here - your inaptitude to understand the problem being the only proof I need. See? I too can make derogatory comments: admire how it adds constructively to the discussion. Want to continue? I'd have plenty of things to say on the way you manage benevolent contributions to your project from other people, of which a psychoanalyst would likely diagnose a certain sense of intellectual insecurity. How does it feel? Want to continue again? Or we stop here and engage in a more productive conversation?
Facts.
- Darwin vm_statistics64 was introduced in OS/X Snow Leopard in 2009, that is - 16 years ago.
- At the time when I sent this code to Nathan Scott, no iOS branch existed in htop's source code, and more generally no 32-bit build for Darwin was possible. I logically concluded this #ifdef was the reliquate of an age long gone and could be disposed of, especially considering the temper tantrum you made about #ifdefs when I suggested to add some for FreeBSD in your platform-agnostic code. And guess what: I even supposed it would please you.
- You are unable to ascertain precisely the baseline OS requirement for all of the htop's ports because they are documented nowhere. The best you can come up with is "try building it, if it works that's okay, if it doesn't we'll look into it", and now you are arguing against removing support for a version of the Darwin kernel that's obsolete for more than a decade? Continue entertaining the readers with your posts. At this point, why not bring back support for paleolithic kernels in all the other branches, Linux, FreeBSD, NetBSD?
Now on a more professional tone. This deletion was made in a spirit of simplification. If adding 32-bit iOS support to the htop code should be considered, then simply put these #ifdefs back. No need to insult or defame the contributors.
One last remark: if someone wants in Q4 2025 to add support to an ancient version of an OS on a deprecated architecture, he/she likely already has the skills to do it himself and add the missing pieces back. Any code that's #ifdef'ed out 99% of the time the project is built and that you leave here for legacy reasons is a technological debt, that you will bear the responsibility to maintain. I could troll you seriously here, with more irony about your role as a maintainer, please note that I choose not to. I expect you to do the same, or adopt the only responsible alternative.
There was a problem hiding this comment.
Darwin vm_statistics64 was introduced in OS/X Snow Leopard in 2009, that is - 16 years ago.
This deletion was made in a spirit of simplification. If adding 32-bit iOS support to the htop code should be considered, then simply put these #ifdefs back. No need to insult or defame the contributors.
Dropping compatibility to legacy systems without stating a good reason is a bad attitude to many free and open source projects.
We have even had a pull request to make compatibility to OS X Tiger (#1687)! Yes, and that unreasoned drop of the old code is to make the life of other contributors harder!
I'm aware that the legacy code may be a "technological debt", but other than the reasons of "it's broken for a long time" and "it's a substantial effort to maintain the compatibility code that it may not seems worth it", I'm not seeing any other good reason for removing it.
I never support the attitude from those big, proprietary corporations that they can arbitrary drop support for old hardware or systems (look at Microsoft and Windows 11, for example), and expect consumers/end-users to get over it.
There was a problem hiding this comment.
I'm aware that the legacy code may be a "technological debt", but other than the reasons of "it's broken for a long time" and "it's a substantial effort to maintain the compatibility code that it may not seems worth it", I'm not seeing any other good reason for removing it.
These are, precisely, two most excellent and self-sufficient, reasons to do it.
But as I said elsewhere, this is your project - you do what you want.
There was a problem hiding this comment.
To both of you: Please at least try to stay professional for a moment. (@Explorer09 We had a discussion about these kinds of comments off-site. I'm not here to babysit you and would appreciate if we keep things without personal attacks/insults.)
Back for the actual topic. Personally I'm open to move forward in either direction, without any preference. Thus my initial question in this thread, how dropping the #ifdef affects compatibility. Regardless of what we are going to decide on this question though, this should be its own commit, if we decide to keep it, as it's a change unrelated to the other changes. After all the commit history should tell you about the various changes performed to reach a given goal and squashing this in with the other changes obscures the visibility of this backwards compatibility related change.
Quick aside on the backward compatibility: For the Linux port we intentionally keep support for kernels without support for openat, which was introduced in Linux 2.6.16. And while we do not adhere to strict BC for all eternity, we at least try to maintain BC on a best-effort basis. Thus anything that was around when htop came around is potentially an option for having it supported, if people are actively asking for it AND supporting it doesn't cause other complications elsewhere. The memory structure handling subject of this comment is easy enough to keep around, so there's no pressure to touch that code AFAICS.
| #define MEMORY_CLASS_INACTIVE 5 | ||
| { .label = "inactive", .countsAsUsed = true, .countsAsCache = true, .color = DYNAMIC_GRAY }, // pages no longer used; macOS counts them as "used" anyway... | ||
| { .label = "wired", .countsAsUsed = true, .countsAsCache = false, .color = MEMORY_1 }, // pages wired down to physical memory (kernel) | ||
| { .label = "speculative", .countsAsUsed = true, .countsAsCache = true, .color = MEMORY_2 }, // readahead optimization caches |
There was a problem hiding this comment.
Especially using the array index notation is used elsewhere already. Makes things more readable.
| if ((sysctl(MIB_vfs_bufspace, 2, &(buffersMem), &len, NULL, 0) == 0) && (buffersMem > 0)) { | ||
| this->buffersMem = buffersMem / 1024; | ||
| this->wiredMem -= this->buffersMem; // substract (NB: "buffers" can't be larger than "wired") | ||
| } | ||
| else | ||
| this->buffersMem = 0; |
There was a problem hiding this comment.
| if ((sysctl(MIB_vfs_bufspace, 2, &(buffersMem), &len, NULL, 0) == 0) && (buffersMem > 0)) { | |
| this->buffersMem = buffersMem / 1024; | |
| this->wiredMem -= this->buffersMem; // substract (NB: "buffers" can't be larger than "wired") | |
| } | |
| else | |
| this->buffersMem = 0; | |
| if ((sysctl(MIB_vfs_bufspace, 2, &(buffersMem), &len, NULL, 0) == 0) && (buffersMem > 0)) | |
| this->buffersMem = buffersMem / 1024; | |
| else | |
| this->buffersMem = 0; | |
| this->wiredMem -= this->buffersMem; // substract (NB: "buffers" can't be larger than "wired") |
| } | ||
| else { // if showCachedMemory is disabled, merge buffers into the wired pages |
There was a problem hiding this comment.
| } | |
| else { // if showCachedMemory is disabled, merge buffers into the wired pages | |
| } else { // if showCachedMemory is disabled, merge buffers into the wired pages |
| if ((sysctl(MIB_vfs_bufspace, 2, &(buffersMem), &len, NULL, 0) == 0) && (buffersMem > 0)) { | ||
| this->buffersMem = buffersMem / 1024; | ||
| this->wiredMem -= this->buffersMem; // substract (NB: "buffers" can't be larger than "wired") | ||
| } | ||
| else | ||
| this->buffersMem = 0; |
There was a problem hiding this comment.
| if ((sysctl(MIB_vfs_bufspace, 2, &(buffersMem), &len, NULL, 0) == 0) && (buffersMem > 0)) { | |
| this->buffersMem = buffersMem / 1024; | |
| this->wiredMem -= this->buffersMem; // substract (NB: "buffers" can't be larger than "wired") | |
| } | |
| else | |
| this->buffersMem = 0; | |
| if ((sysctl(MIB_vfs_bufspace, 2, &(buffersMem), &len, NULL, 0) == 0) && (buffersMem > 0)) | |
| this->buffersMem = buffersMem / 1024; | |
| else | |
| this->buffersMem = 0; | |
| this->wiredMem -= this->buffersMem; // substract (NB: "buffers" can't be larger than "wired") |
| if (Metric_values(PCP_MEM_WIRED, &value, 1, PM_TYPE_U64) != NULL) | ||
| this->memValue[MEMORY_CLASS_WIRED] = value.ull; | ||
| if (Metric_values(PCP_MEM_ACTIVE, &value, 1, PM_TYPE_U64) != NULL) | ||
| activeMem = value.ull; | ||
| if (Metric_values(PCP_MEM_EXTERNAL, &value, 1, PM_TYPE_U64) != NULL) | ||
| externalMem = value.ull; | ||
| if (Metric_values(PCP_MEM_PURGEABLE, &value, 1, PM_TYPE_U64) != NULL) | ||
| purgeableMem = value.ull; | ||
| if (Metric_values(PCP_MEM_SPECULATIVE, &value, 1, PM_TYPE_U64) != NULL) | ||
| speculativeMem = value.ull; | ||
| if (settings->showCachedMemory) { | ||
| this->memValue[MEMORY_CLASS_SPECULATIVE] = speculativeMem; | ||
| this->memValue[MEMORY_CLASS_ACTIVE] = (activeMem - purgeableMem - externalMem); | ||
| this->memValue[MEMORY_CLASS_PURGEABLE] = purgeableMem; | ||
| } | ||
| else { | ||
| this->memValue[MEMORY_CLASS_SPECULATIVE] = 0; | ||
| this->memValue[MEMORY_CLASS_ACTIVE] = (speculativeMem + activeMem - externalMem); | ||
| this->memValue[MEMORY_CLASS_PURGEABLE] = 0; | ||
| } | ||
| if (Metric_values(PCP_MEM_COMPRESSED, &value, 1, PM_TYPE_U64) != NULL) | ||
| this->memValue[MEMORY_CLASS_COMPRESSED] = value.ull; | ||
| if (Metric_values(PCP_MEM_INACTIVE, &value, 1, PM_TYPE_U64) != NULL) | ||
| this->memValue[MEMORY_CLASS_INACTIVE] = value.ull; |
There was a problem hiding this comment.
| if (Metric_values(PCP_MEM_WIRED, &value, 1, PM_TYPE_U64) != NULL) | |
| this->memValue[MEMORY_CLASS_WIRED] = value.ull; | |
| if (Metric_values(PCP_MEM_ACTIVE, &value, 1, PM_TYPE_U64) != NULL) | |
| activeMem = value.ull; | |
| if (Metric_values(PCP_MEM_EXTERNAL, &value, 1, PM_TYPE_U64) != NULL) | |
| externalMem = value.ull; | |
| if (Metric_values(PCP_MEM_PURGEABLE, &value, 1, PM_TYPE_U64) != NULL) | |
| purgeableMem = value.ull; | |
| if (Metric_values(PCP_MEM_SPECULATIVE, &value, 1, PM_TYPE_U64) != NULL) | |
| speculativeMem = value.ull; | |
| if (settings->showCachedMemory) { | |
| this->memValue[MEMORY_CLASS_SPECULATIVE] = speculativeMem; | |
| this->memValue[MEMORY_CLASS_ACTIVE] = (activeMem - purgeableMem - externalMem); | |
| this->memValue[MEMORY_CLASS_PURGEABLE] = purgeableMem; | |
| } | |
| else { | |
| this->memValue[MEMORY_CLASS_SPECULATIVE] = 0; | |
| this->memValue[MEMORY_CLASS_ACTIVE] = (speculativeMem + activeMem - externalMem); | |
| this->memValue[MEMORY_CLASS_PURGEABLE] = 0; | |
| } | |
| if (Metric_values(PCP_MEM_COMPRESSED, &value, 1, PM_TYPE_U64) != NULL) | |
| this->memValue[MEMORY_CLASS_COMPRESSED] = value.ull; | |
| if (Metric_values(PCP_MEM_INACTIVE, &value, 1, PM_TYPE_U64) != NULL) | |
| this->memValue[MEMORY_CLASS_INACTIVE] = value.ull; | |
| if (Metric_values(PCP_MEM_WIRED, &value, 1, PM_TYPE_U64) != NULL) | |
| this->memValue[MEMORY_CLASS_WIRED] = value.ull; | |
| if (Metric_values(PCP_MEM_ACTIVE, &value, 1, PM_TYPE_U64) != NULL) | |
| activeMem = value.ull; | |
| if (Metric_values(PCP_MEM_EXTERNAL, &value, 1, PM_TYPE_U64) != NULL) | |
| externalMem = value.ull; | |
| if (Metric_values(PCP_MEM_PURGEABLE, &value, 1, PM_TYPE_U64) != NULL) | |
| purgeableMem = value.ull; | |
| if (Metric_values(PCP_MEM_SPECULATIVE, &value, 1, PM_TYPE_U64) != NULL) | |
| speculativeMem = value.ull; | |
| if (settings->showCachedMemory) { | |
| this->memValue[MEMORY_CLASS_SPECULATIVE] = speculativeMem; | |
| this->memValue[MEMORY_CLASS_ACTIVE] = (activeMem - purgeableMem - externalMem); | |
| this->memValue[MEMORY_CLASS_PURGEABLE] = purgeableMem; | |
| } else { | |
| this->memValue[MEMORY_CLASS_SPECULATIVE] = 0; | |
| this->memValue[MEMORY_CLASS_ACTIVE] = (speculativeMem + activeMem - externalMem); | |
| this->memValue[MEMORY_CLASS_PURGEABLE] = 0; | |
| } | |
| if (Metric_values(PCP_MEM_COMPRESSED, &value, 1, PM_TYPE_U64) != NULL) | |
| this->memValue[MEMORY_CLASS_COMPRESSED] = value.ull; | |
| if (Metric_values(PCP_MEM_INACTIVE, &value, 1, PM_TYPE_U64) != NULL) | |
| this->memValue[MEMORY_CLASS_INACTIVE] = value.ull; |
Some visual space doesn't hurt.
| strcat(pcp->release, " "); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
No need for second blank line here …
| for (unsigned int memoryClassIdx = 0; memoryClassIdx < Platform_numberOfMemoryClasses; memoryClassIdx++) { | ||
| if (Platform_memoryClasses[memoryClassIdx].countsAsUsed) | ||
| used += this->values[memoryClassIdx]; | ||
| } |
There was a problem hiding this comment.
Prefer with all braces to be consistent with below:
| for (unsigned int memoryClassIdx = 0; memoryClassIdx < Platform_numberOfMemoryClasses; memoryClassIdx++) { | |
| if (Platform_memoryClasses[memoryClassIdx].countsAsUsed) | |
| used += this->values[memoryClassIdx]; | |
| } | |
| for (unsigned int memoryClassIdx = 0; memoryClassIdx < Platform_numberOfMemoryClasses; memoryClassIdx++) { | |
| if (Platform_memoryClasses[memoryClassIdx].countsAsUsed) { | |
| used += this->values[memoryClassIdx]; | |
| } | |
| } |
| for (unsigned int memoryClassIdx = 0; memoryClassIdx < Platform_numberOfMemoryClasses; memoryClassIdx++) { | ||
| if (Platform_memoryClasses[memoryClassIdx].countsAsCache) | ||
| this->values[memoryClassIdx] = NAN; | ||
| } |
There was a problem hiding this comment.
| for (unsigned int memoryClassIdx = 0; memoryClassIdx < Platform_numberOfMemoryClasses; memoryClassIdx++) { | |
| if (Platform_memoryClasses[memoryClassIdx].countsAsCache) | |
| this->values[memoryClassIdx] = NAN; | |
| } | |
| for (unsigned int memoryClassIdx = 0; memoryClassIdx < Platform_numberOfMemoryClasses; memoryClassIdx++) { | |
| if (Platform_memoryClasses[memoryClassIdx].countsAsCache) { | |
| this->values[memoryClassIdx] = NAN; | |
| } | |
| } |
| if (!settings->showCachedMemory && Platform_memoryClasses[memoryClassIdx].countsAsCache) | ||
| continue; // skip reclaimable cache memory classes if "show cached memory" is not ticked |
There was a problem hiding this comment.
Some visual space doesn't hurt ^^
| if (!settings->showCachedMemory && Platform_memoryClasses[memoryClassIdx].countsAsCache) | |
| continue; // skip reclaimable cache memory classes if "show cached memory" is not ticked | |
| if (!settings->showCachedMemory && Platform_memoryClasses[memoryClassIdx].countsAsCache) | |
| continue; // skip reclaimable cache memory classes if "show cached memory" is not ticked | |
5 participants
Resolves #1725