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

Fix RAM display on non-Linux hosts #788

Merged
merged 6 commits into from Oct 15, 2023
Merged

Fix RAM display on non-Linux hosts #788

merged 6 commits into from Oct 15, 2023

Conversation

Rycieos
Copy link
Collaborator

@Rycieos Rycieos commented Sep 1, 2023

A number of small bugs that worked together to make Linux RAM display correct, but MacOS and BSD off by a factor of 1024.

Fixes #787

Note that this was not caught by the tests, as the tests only check what __lp_ram_bytes() returns, not what _lp_ram() returns. Would be good to have that added @nojhan if you want to do that. Specifically the values of $lp_ram and $lp_ram_human.

A number of small bugs worked together to make Linux RAM display
correct, but MacOS and BSD off by a factor of 1024

Fixes #787
@Rycieos Rycieos added this to the v2.2 milestone Sep 1, 2023
@Rycieos Rycieos requested a review from nojhan September 1, 2023 19:33
@tomhoover
Copy link

I reset LP_ENABLE_RAM=1 with your proposed fix. I'm now seeing M32.00GiB in my prompt, even though my MacBook Pro has only 16GB.

@tomhoover
Copy link

More info:

Pressing return multiple times resulted in the following:

⏚ M225.17GiB [:~] 1 %
⏚ M47.56GiB [:~] 1 %
⏚ M62.39GiB [:~] 1 %
⏚ M256.98GiB [:~] 1 %
⏚ M218.29GiB [:~] 1 %
⏚ M198.43GiB [:~] 1 %
⏚ M149.60GiB [:~] 1 %
⏚ M134.96GiB [:~] 1 %
⏚ M128.87GiB [:~] 1 %
⏚ M122.40GiB [:~] 1 %
⏚ M110.14GiB [:~] 1 %
⏚ M114.84GiB [:~] 1 %
⏚ M111.43GiB [:~] 1 %
⏚ M108.31GiB [:~] 1 %

I'll be happy to run any tests you'd like.

@nojhan
Copy link
Collaborator

nojhan commented Oct 8, 2023

I added tests on lp_ram_* and (hopefully) fixed the MacOS mem counter. On my mac it seems to be (more or less) consistent with the activity monitor.

@Rycieos
Copy link
Collaborator Author

Rycieos commented Oct 9, 2023

Looks like your fix was a step in the right direction. I found an issue with adding the two fields, which I fixed. I also fixed the test case. I added my own Mac as a test case, and fixed an issue I discovered on Cygwin.

Copy link
Collaborator

@nojhan nojhan left a comment

Choose a reason for hiding this comment

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

Seems good, works well on my setups.

@Rycieos Rycieos merged commit 23a667f into master Oct 15, 2023
9 checks passed
@Rycieos Rycieos deleted the fix/ram branch October 15, 2023 15:05
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.

Memory display on Darwin is inaccurate
3 participants