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. |
The discussion in #281 might be related. |
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. |
|
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. |
|
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.
natoscott
force-pushed
the
freebsd-memory
branch
from
27ca7b5 to
0a27fef
Compare
FreeBSD required several corrections to memory calculations that proved impossible to reconcile with existing memory categories. Resolves htop-dev#1725
natoscott
force-pushed
the
freebsd-memory
branch
from
0a27fef to
dd24cd6
Compare
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
dd24cd6 to
1ca6213
Compare
5 participants
Resolves #1725