Add buffer and cache memory #1063

aragonnetje6 · 2023-03-13T02:56:23Z

Description

Added a line in the memory graph for the cache and buffer portions of memory. Not sure if this should be stacked on top of the RAM for clarity that this takes up the same space or not. Implementation mostly taken from the RAM implementation. No tests were added since the existing similar parts also do not have tests. The line will always be 0 on windows due to platform-dependent behaviour of sysinfo. perhaps it should be left out there?

Issue

Feature: Add buffer and cache memory

Closes: #533

Testing

If relevant, please state how this was tested. All changes must be tested to work:

No new tests were added as the existing memory components also have no tests. I'm willing to add these, just let me know if this is desired. All existing tests still pass, and running the program works just fine

If this is a code change, please also indicate which platforms were tested:

Windows
macOS
Linux

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, doc pages, 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)

…rom RAM and swap implementations

…p implementations. placed cache as second in the list as it is more similar to the RAM than any other item in the list

ClementTsang · 2023-03-13T03:01:42Z

Thanks for this! Whenever it's ready for review, feel free to let me know.

The line will always be 0 on windows due to platform-dependent behaviour of sysinfo. perhaps it should be left out there?

Yeah, it might be better to leave it out through cfg flags if it'll always be a useless stat.

One other comment based on the current pic is I kinda prefer to have the legends aligned, so either spacing is needed or I guess we can abbreviate "CACHE"? "CHE"? I don't know tbh.

aragonnetje6 · 2023-03-13T03:06:36Z

Yeah, it might be better to leave it out through cfg flags if it'll always be a useless stat.

Will do!

One other comment based on the current pic is I kinda prefer to have the legends aligned, so either spacing is needed or I guess we can abbreviate "CACHE"? "CHE"? I don't know tbh.

It would be nice to have all entries at 3 letters, and i suppose CHE works.

Also, it was mentioned that this line should perhaps be off by default to prevent cluttering the graph, do you agree? I could add it to clap

codecov · 2023-03-13T03:07:57Z

Codecov Report

Patch coverage: 18.63% and project coverage change: -0.67 ⚠️

Comparison is base (c8c64b0) 26.44% compared to head (911e9d0) 25.78%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1063      +/-   ##
==========================================
- Coverage   26.44%   25.78%   -0.67%     
==========================================
  Files          83       94      +11     
  Lines       14733    15492     +759     
==========================================
+ Hits         3896     3994      +98     
- Misses      10837    11498     +661

Flag	Coverage Δ
macos-12	`27.14% <17.36%> (-0.16%)`	⬇️
ubuntu-latest	`27.11% <16.77%> (-0.02%)`	⬇️
windows-2019	`27.19% <27.18%> (+0.18%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/app.rs	`0.14% <ø> (ø)`
src/app/data_harvester.rs	`0.00% <0.00%> (ø)`
src/app/data_harvester/memory.rs	`100.00% <ø> (ø)`
src/app/data_harvester/memory/sysinfo.rs	`0.00% <0.00%> (ø)`
src/app/layout_manager.rs	`81.17% <ø> (ø)`
src/bin/main.rs	`35.94% <0.00%> (-1.56%)`	⬇️
src/canvas.rs	`0.00% <0.00%> (ø)`
src/canvas/canvas_styling/colour_utils.rs	`97.53% <ø> (ø)`
src/canvas/widgets/mem_basic.rs	`0.00% <0.00%> (ø)`
src/canvas/widgets/mem_graph.rs	`0.00% <0.00%> (ø)`
... and 7 more

... and 22 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

… src/data_conversion.rs convert_mem_label() to convert a single label instead of all at once

… default

aragonnetje6 · 2023-03-13T22:04:28Z

I have 2 potential versions now, both working properly, passing tests, and at least compiling for windows (using cross). The version in the master branch just has the cache line on by default, while the version in the "clap" branch it's off by default but can be enabled through the --enable-cache-memory flag. I would love to hear which is preferred!

I also renamed the tag to CHE, since all the others have 3 letters as well.

not sure how i messed that up

ClementTsang · 2023-03-19T06:40:25Z

Sorry for getting back to you late.

I think for now leaving it off by default might be fine; in the future I might make it easier to access through a config menu.

aragonnetje6 · 2023-03-19T22:53:57Z

alright, that version has been merged in, documentation has been updated, and i tested on windows in addition to linux. i don't have access to mac, so i can't really test that

ClementTsang · 2023-03-21T23:39:42Z

i don't have access to mac, so i can't really test that

All good, will test that and get back to you.

ClementTsang

Sorry for taking so long. A few nit comments and some small suggestions.

EDIT: Note that you might need to do a rebase, as there were some changes to the unit stored by MemHarvest (it's now in bytes).

docs/content/configuration/command-line-flags.md

docs/content/configuration/config-file/flags.md

src/canvas/canvas_styling.rs

src/app/data_harvester/memory/sysinfo.rs

# Conflicts: # src/data_conversion.rs

aragonnetje6 · 2023-03-31T02:01:38Z

well that close was unintentional, merge conflicts with the main repo happened but that's fixed now. i hope the changes are implemented satisfactorily!

…GPU colour

This reverts commit 72698f1.

ClementTsang

Just a few small nits. Otherwise, looks good to me. If you don't think these are worth changing that's also fine, just let me know and I'll approve + merge.

Also, if possible, it would be good to update the default config (sample_configs/default_config.toml) and it's corresponding default file in the application, but I can handle that in a separate commit if you want.

src/app/data_harvester/memory/sysinfo.rs

src/canvas/widgets/mem_basic.rs

Co-authored-by: Clement Tsang <34804052+ClementTsang@users.noreply.github.com>

ClementTsang

Looks good, I'll merge sometime today or tomorrow. Thanks!

ClementTsang · 2023-04-12T05:18:56Z

@all-contributors please add @aragonnetje6 for code.

allcontributors · 2023-04-12T05:19:04Z

@ClementTsang

I've put up a pull request to add @aragonnetje6! 🎉

aragonnetje6 added 4 commits March 13, 2023 03:27

First implementation of cache memory data collection, mostly copied f…

ba94509

…rom RAM and swap implementations

First implementation of cache memory display, copied from RAM and swa…

dbed441

…p implementations. placed cache as second in the list as it is more similar to the RAM than any other item in the list

expanded comment to explain method.

1c3bfdb

rustfmt

4afd719

aragonnetje6 changed the title ~~Draft:~~ Draft: Add buffer and cache memory Mar 13, 2023

aragonnetje6 and others added 4 commits March 13, 2023 22:00

all cache-related code excluded on windows, in the process refactored…

1300ade

… src/data_conversion.rs convert_mem_label() to convert a single label instead of all at once

Merge branch 'ClementTsang:master' into master

0b0c1b0

better factoring-out of cache memory logic to allow individual disabling

69e1783

added --enable_cache_memory flag, disabled cache memory collection by…

e6fc69b

… default

aragonnetje6 and others added 3 commits March 14, 2023 00:00

renamed CCH to CHE

5aff40b

not sure how i messed that up

Merge branch 'master' into clap

f6f4526

Merge branch 'master' into master

5db60e7

aragonnetje6 changed the title ~~Draft: Add buffer and cache memory~~ Add buffer and cache memory Mar 19, 2023

aragonnetje6 and others added 5 commits March 19, 2023 21:46

changelog updated

09f30e6

Added command line flag documentation

0842c56

Merge branch 'master' into clap

ca87be9

updated config file documentation

db8f013

Merge branch 'master' into master

7b684e8

aragonnetje6 marked this pull request as ready for review March 19, 2023 20:53

specified that buffer and cache memory display does not work on windows

8cf5a92

ClementTsang self-assigned this Mar 23, 2023

ClementTsang requested changes Mar 26, 2023

View reviewed changes

aragonnetje6 closed this Mar 31, 2023

aragonnetje6 added 3 commits March 31, 2023 03:18

Merge remote-tracking branch 'origin/master' into re-merge

f6a0b55

# Conflicts: # src/data_conversion.rs

resolved merge conflicts

bd9e576

added documentation to cache memory data collection

feb3e68

aragonnetje6 reopened this Mar 31, 2023

aragonnetje6 added 2 commits March 31, 2023 03:50

capitalized Windows

e06b1b1

implemented missing canvas styling logic

d7f17eb

aragonnetje6 and others added 7 commits March 31, 2023 04:15

fixed misplaced no-windows flag

de75557

reduced colour collisions, as cache colour was the same as the first …

4c557e1

…GPU colour

made FIFTH_COLOUR constant windows-only

72698f1

Revert "made FIFTH_COLOUR constant windows-only"

3112caf

This reverts commit 72698f1.

made FIFTH_COLOUR constant non-windows-only

9e7a1f4

Merge branch 'master' into master

f9e2076

minor fix for basic mode row count

8405aa0

ClementTsang requested changes Apr 4, 2023

View reviewed changes

src/app/data_harvester/memory/sysinfo.rs Outdated Show resolved Hide resolved

src/canvas/widgets/mem_basic.rs Outdated Show resolved Hide resolved

aragonnetje6 and others added 5 commits April 10, 2023 18:38

Update src/app/data_harvester/memory/sysinfo.rs

8778561

Co-authored-by: Clement Tsang <34804052+ClementTsang@users.noreply.github.com>

Update src/canvas/widgets/mem_basic.rs

7aba978

Co-authored-by: Clement Tsang <34804052+ClementTsang@users.noreply.github.com>

updated default_config.toml

7faed1f

Merge remote-tracking branch 'origin/master'

d2101bb

formatting

911e9d0

ClementTsang self-requested a review April 11, 2023 09:08

ClementTsang approved these changes Apr 12, 2023

View reviewed changes

allcontributors bot mentioned this pull request Apr 12, 2023

docs: add aragonnetje6 as a contributor for code #1096

Merged

ClementTsang merged commit 1b1e80e into ClementTsang:master Apr 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add buffer and cache memory #1063

Add buffer and cache memory #1063

aragonnetje6 commented Mar 13, 2023 •

edited

Loading

ClementTsang commented Mar 13, 2023

aragonnetje6 commented Mar 13, 2023

codecov bot commented Mar 13, 2023 •

edited

Loading

aragonnetje6 commented Mar 13, 2023 •

edited

Loading

ClementTsang commented Mar 19, 2023

aragonnetje6 commented Mar 19, 2023

ClementTsang commented Mar 21, 2023

ClementTsang left a comment •

edited

Loading

aragonnetje6 commented Mar 31, 2023

ClementTsang left a comment •

edited

Loading

ClementTsang left a comment •

edited

Loading

ClementTsang commented Apr 12, 2023

allcontributors bot commented Apr 12, 2023

Add buffer and cache memory #1063

Add buffer and cache memory #1063

Conversation

aragonnetje6 commented Mar 13, 2023 • edited Loading

Description

Issue

Testing

Checklist

ClementTsang commented Mar 13, 2023

aragonnetje6 commented Mar 13, 2023

codecov bot commented Mar 13, 2023 • edited Loading

Codecov Report

aragonnetje6 commented Mar 13, 2023 • edited Loading

ClementTsang commented Mar 19, 2023

aragonnetje6 commented Mar 19, 2023

ClementTsang commented Mar 21, 2023

ClementTsang left a comment • edited Loading

Choose a reason for hiding this comment

aragonnetje6 commented Mar 31, 2023

ClementTsang left a comment • edited Loading

Choose a reason for hiding this comment

ClementTsang left a comment • edited Loading

Choose a reason for hiding this comment

ClementTsang commented Apr 12, 2023

allcontributors bot commented Apr 12, 2023

aragonnetje6 commented Mar 13, 2023 •

edited

Loading

codecov bot commented Mar 13, 2023 •

edited

Loading

aragonnetje6 commented Mar 13, 2023 •

edited

Loading

ClementTsang left a comment •

edited

Loading

ClementTsang left a comment •

edited

Loading

ClementTsang left a comment •

edited

Loading