Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add gpu ram collector for nvidia feature flag #794

Merged
merged 9 commits into from
Oct 15, 2022

Conversation

jamartin9
Copy link
Contributor

@jamartin9 jamartin9 commented Aug 29, 2022

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 GPU and VRAM load #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:

  • Windows
  • macOS
  • Linux
  • freeBSD

Checklist

If relevant, ensure the following have been met:

  • Areas your change affects have been linted using rustfmt (cargo fmt)
  • 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
  • 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 jamartin9@052eee2 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-commenter
Copy link

codecov-commenter commented Aug 29, 2022

Codecov Report

Base: 18.97% // Head: 18.69% // Decreases project coverage by -0.27% ⚠️

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%) ⬇️
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%) ⬇️
... 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.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ClementTsang ClementTsang self-assigned this Aug 30, 2022
@jamartin9
Copy link
Contributor Author

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

Take your time with review!

@ClementTsang
Copy link
Owner

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
Copy link
Owner

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

@jamartin9
Copy link
Contributor Author

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.

Copy link
Owner

@ClementTsang ClementTsang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments. For the most part it looks fine, just some suggestions.

src/app/data_harvester/memory/general/heim.rs Outdated Show resolved Hide resolved
src/canvas/widgets/mem_basic.rs Outdated Show resolved Hide resolved
src/canvas/widgets/mem_basic.rs Outdated Show resolved Hide resolved
@jamartin9
Copy link
Contributor Author

jamartin9 commented Oct 14, 2022

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)

Copy link
Owner

@ClementTsang ClementTsang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me, thanks for the changes!

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.

@jamartin9
Copy link
Contributor Author

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.

@ClementTsang
Copy link
Owner

ClementTsang commented Oct 15, 2022

Sounds good.

Repository owner deleted a comment from allcontributors bot Oct 15, 2022
@ClementTsang ClementTsang merged commit bd35bbd into ClementTsang:master Oct 15, 2022
ClementTsang added a commit that referenced this pull request Oct 15, 2022
Follow-up to #794, this makes GPU memory collection toggleable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants