bottom
bottom copied to clipboard
add gpu ram collector for nvidia feature flag
Description
Adds multiple GPU VRAM support with styling by default for up to 7 cards.
Does NOT include GPU processes, power usage, core frequency or graph toggles
Extra changes:
-
Show
SWP
only when enabled in basic mode -
Use
zfs_keys_read
inget_arc_data
-
Toggle
ARC
with label instead of point
Screens:
Windows with swap, single gpu, no arc in basic mode
Linux swap, 8 gpus, with arc
Linux swap, 8 gpus, with arc in basic mode (percentage toggled)
Linux no swap, no gpus, no arc
Freebsd no swap, no gpu, no arc in basic mode
Issue
- Mentioned in issue #298 and other issues that GPU stats should include graph toggles, gpu core frequency, gpu processes, gpu power usage, and be hidden by default (like battery).
Which of these are required for this pull request to be merged?
Should the current temperature readings for gpus be hidden? (currently only shown when data is detected at runtime)
-
Mentioned in issue 787 that heim might be removed during refactor, so the duplication of
get_gpu_data
may not matter? (nvml only supports nvidia on windows/linux) -
Memory bars in basic mode share a layout with network.
Is balancing the widgets drawn between memory and network in basic mode needed? This may complicate the layout if more network features are added in the future. (more than a few gpus is probably uncommon)
Testing
If relevant, please state how this was tested. All changes must be tested to work:
If this is a code change, please also indicate which platforms were tested:
- [X] Windows
- [ ] macOS
- [X] Linux
- [X] freeBSD
Checklist
If relevant, ensure the following have been met:
- [X] Areas your change affects have been linted using rustfmt (
cargo fmt
) - [X] The change has been tested and doesn't appear to cause any unintended breakage
- [ ] Documentation has been added/updated if needed (
README.md
, help menu, etc.) - [ ] The pull request passes the provided CI pipeline
- [X] There are no merge conflicts
- [ ] If relevant, new tests were added (don't worry too much about coverage)
I did NOT test on MAC
I did NOT test with a real multi device or SLI setup (see https://github.com/jamartin9/bottom/commit/052eee28efb56676e7273ef750ff444d444377cf for the test code of the images that duplicates an existing gpu report with data changed to add 7 gpus for style cycling with defaults)
If any of the Issues should be addressed or a different approach used please let me know.
Feel free to close this PR if it is unwanted.
Codecov Report
Base: 18.97% // Head: 18.69% // Decreases project coverage by -0.27%
:warning:
Coverage data is based on head (
882744a
) compared to base (8b72a33
). Patch coverage: 3.18% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@
## master #794 +/- ##
==========================================
- Coverage 18.97% 18.69% -0.28%
==========================================
Files 71 71
Lines 13247 13499 +252
==========================================
+ Hits 2513 2524 +11
- Misses 10734 10975 +241
Impacted Files | Coverage Δ | |
---|---|---|
src/app/data_farmer.rs | 0.00% <0.00%> (ø) |
|
src/app/data_harvester.rs | 0.00% <0.00%> (ø) |
|
src/app/data_harvester/memory/general/heim.rs | 0.00% <0.00%> (ø) |
|
src/app/data_harvester/temperature/nvidia.rs | 0.00% <0.00%> (ø) |
|
src/bin/main.rs | 40.42% <0.00%> (-1.11%) |
:arrow_down: |
src/canvas.rs | 0.00% <0.00%> (ø) |
|
src/canvas/widgets/mem_basic.rs | 0.00% <0.00%> (ø) |
|
src/canvas/widgets/mem_graph.rs | 0.00% <0.00%> (ø) |
|
src/constants.rs | 0.00% <0.00%> (ø) |
|
src/data_conversion.rs | 12.52% <0.00%> (-1.98%) |
:arrow_down: |
... and 6 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Apologizes for pushing extra commits but I noticed a few bugs.
Take your time with review!
Just a heads up, I'm definitely interested in merging this - just might take me a bit of time to get to reviewing it due to some IRL stuff.
Looking through it now, sorry for the delay, and thanks for the updates after some other PRs were merged.
Tested on my M1 macOS and it seems to work fine (that is, it does nothing and bottom continues on working, which seems to make sense in this context).
Thanks for taking some time to review. The latest changes were to resolve the merge conflicts and tidy up the code a bit. I did a quick check of the changes on the linux system with a gpu again. I will try to address comments when time permits.
Thanks for the feedback. I tried to address your comments. Please let me know if more needs to be done. For posterity's sake here are some more changes in this pr induced from the merge:
- build on freebsd again
- re-add mem basic mode percentage toggle
I gave it another quick build and run on linux w/gpu and freebsd w/o (I no longer have access to windows w/gpu)
One more thing I would probably want after some thought is a config option to enable/disable this feature on runtime. If you want, I can add this myself after merging this PR - lmk if you would prefer that.
Thanks. I think it would be preferable for ergonomics if you were to add the toggle in another pr. I can assist in testing if needed.
Sounds good.