bottom icon indicating copy to clipboard operation
bottom copied to clipboard

add gpu ram collector for nvidia feature flag

Open jamartin9 opened this issue 2 years ago • 3 comments

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:

  1. Show SWP only when enabled in basic mode

  2. Use zfs_keys_read in get_arc_data

  3. Toggle ARC with label instead of point

Screens:

Windows with swap, single gpu, no arc in basic mode

bottom-gpu

Linux swap, 8 gpus, with arc

linux-bottom

Linux swap, 8 gpus, with arc in basic mode (percentage toggled)

linux-basic

Linux no swap, no gpus, no arc

fedora

Freebsd no swap, no gpu, no arc in basic mode

freebsd-basic

Issue

  1. 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)

  1. 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)

  2. 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.

jamartin9 avatar Aug 29 '22 22:08 jamartin9

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.

codecov-commenter avatar Aug 29 '22 22:08 codecov-commenter

Apologizes for pushing extra commits but I noticed a few bugs.

Take your time with review!

jamartin9 avatar Aug 30 '22 22:08 jamartin9

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.

ClementTsang avatar Sep 05 '22 23:09 ClementTsang

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).

ClementTsang avatar Oct 14 '22 04:10 ClementTsang

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.

jamartin9 avatar Oct 14 '22 06:10 jamartin9

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:

  1. build on freebsd again
  2. 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)

jamartin9 avatar Oct 14 '22 19:10 jamartin9

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.

jamartin9 avatar Oct 15 '22 01:10 jamartin9

Sounds good.

ClementTsang avatar Oct 15 '22 19:10 ClementTsang