darwin: order the memory classes so the used ones come first - #2074
darwin: order the memory classes so the used ones come first#2074ravi-arnan wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe macOS memory categories now appear as wired, active, compressed, speculative, purgeable, and inactive. Color assignments were updated to match this sequence. No exported or public declarations changed. Assessment against linked issues
Poem
Merge Risk: ⚪ Minimal · up to This change only reorders Darwin memory classes and their associated colors so used memory appears contiguously while preserving text and bar display consistency; no actionable merge-blocking risk remains beyond normal checks and review. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
natoscott
left a comment
There was a problem hiding this comment.
@ravi-arnan does Platform_memoryClasses need to change here? the values from the enum are baked into it's declaration - isn't it sufficient to just change the enum? Thanks!
|
Good question, and you are right about the ordering: the array uses designated initializers, so the enum alone decides both the storage slot and the display order. Moving the initializer lines is cosmetic. I did it only so the file reads in the order it renders, and I am happy to drop that part if you would rather keep the diff minimal. The
I did build the enum-only version first. On this Mac it renders
Those are the SGR runs decoded out of the captures rather than eyeballed, from one binary in one sitting. Moving So, your call:
I lean 1 or 2. 4 is worth considering if not changing what existing users see matters more than the size of the change, and I am glad to do it either way. |
The memory meter draws the categories in the order they appear in Platform_memoryClasses, and on Darwin that order interleaves the three classes that count as used with the three that count as cache: wired, speculative, active, purgeable, compressed, inactive The used total the meter prints is the sum of the countsAsUsed classes and is correct, but speculative and purgeable are drawn between wired, active and compressed, so that total does not correspond to a contiguous run of bars from the left-hand edge. Darwin is the only platform that interleaves the two kinds. linux, freebsd, dragonflybsd, netbsd, openbsd and solaris all list every countsAsUsed class before any cache class. Reorder the enum so the used classes come first, keeping the relative order within each group, and move each .color along with it so that the colours stay in MEMORY_1..MEMORY_6 sequence. That sequence is not cosmetic: BarMeterMode_draw() colours segment i with MemoryMeter_attributes[i], which is MEMORY_(i+1) by position and ignores .color entirely, while MemoryMeter_display() uses .color. Every platform currently keeps the two in step, so the modes agree; leaving .color attached to its old label here would have made the bar and the text disagree about what colour, say, speculative is. Closes htop-dev#2073 Assisted-by: Claude:opus-5
0600da7 to
a3c0579
Compare
Closes #2073.
darwin/Platform.cis the only platform whosePlatform_memoryClasses[]interleaves the classes that count as used with the ones that count as cache, sospeculativeandpurgeableare drawn betweenwired,activeandcompressed. The used total the meter prints is the sum of thecountsAsUsedclasses and is correct; it just does not correspond to a contiguous run of bars from the left-hand edge, which is what the FAQ describes. The cross-platform table is in the triage comment on the issue.This reorders the enum so the three used classes come first, keeping the relative order inside each group.
The colours had to move with them
.coloris not the only thing that decides a segment's colour, which was not obvious to me until I looked at the two render paths:MemoryMeter_display()(text mode) colours each value withPlatform_memoryClasses[i].color.BarMeterMode_draw()(Meter.c:181) colours segmentiwithMeter_attributes(this)[i], which for this meter isMemoryMeter_attributes[]=MEMORY_1 .. MEMORY_6by position, and never looks at.color.Every platform currently keeps
.colorin step with the array position, so the two paths agree and the difference is invisible. That means "reorder the classes but let every label keep the colour it has today" is not actually on the table: bar mode would recolour by position anyway while text mode would not, and the two modes would then disagree about what colourspeculativeis. So.colormoves with the class and stays inMEMORY_1 .. MEMORY_6sequence, which keeps the existing invariant.With the default colour scheme that means, in both modes:
If you would rather nothing changed colour, the alternative is to make bar/graph/LED mode honour
.colorinstead of the positionalMemoryMeter_attributes[]. That is a change to shared meter code rather than to this platform, so I have not done it here, but say the word and I will.Verification
Built from
main(6f33ddd) and from this branch on a MacBookPro12,1 running macOS 12.7.6, captured back to back. Not an M-series machine, so #2073 is not Apple Silicon specific.Text mode:
Bar mode, with the SGR runs decoded so the class behind each segment is visible (
compressedis 0K on this machine, so it has zero width in both):so the used classes are one contiguous run from the left edge afterwards, and the text and bar modes agree on every class's colour in both builds.
Not touched
pcp/Platform.ccarries a second Darwin table (Darwin_memoryClasses[], memcpy'd intoPlatform_memoryClasseswhen the PCP target reportsDarwin) with the same interleaving. I left it alone because its flags disagree with this file,speculativeandinactivearecountsAsUsed = truethere, so "used first" does not mean the same thing, and I have no PCP Darwin host to test against. Happy to follow up if you want it aligned.