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

easyinstall: Default no. of compiler threads based on available RAM #1239

Merged
merged 1 commit into from
Oct 7, 2023

Conversation

rdmark
Copy link
Member

@rdmark rdmark commented Oct 7, 2023

No description provided.

easyinstall.sh Outdated Show resolved Hide resolved
@rdmark rdmark closed this Oct 7, 2023
@rdmark rdmark reopened this Oct 7, 2023
@rdmark rdmark force-pushed the rdmark-issue-1200 branch 2 times, most recently from 6c5b4db to bd5e3c4 Compare October 7, 2023 08:01
@uweseimet
Copy link
Contributor

@rdmark Just noticed one more thing: This should be 519168, not 512000.

@rdmark
Copy link
Member Author

rdmark commented Oct 7, 2023

@uweseimet FWIW, it seems like low-end RPis actually report less than 512MB MemTotal in /proc/meminfo, according to some random google search results.

F.e. for RPi Zero: 445032KB

https://inokara.hateblo.jp/entry/2016/02/08/004354

It would probably be good to benchmark MemTotal on some key RPi models and adjust the value later.

@uweseimet
Copy link
Contributor

uweseimet commented Oct 7, 2023

@rdmark Would this matter in our case? The current change always assumes at least 1 core, and if less memory is reported than is actually available we would still be better off than before.
Also, this report is from 2016. It may be wrong or outdated, like so many stuff you find in internet forums.

Edit: It SHOULD always assume at least one core. BTW, the report definitely does not apply to buster, bullseye or bookworm, it's too old.

@rdmark
Copy link
Member Author

rdmark commented Oct 7, 2023

@uweseimet It would matter for f.e. the RPi 3B, which has 1GB of memory. This has been my primary development RPi for several years. It benefits hugely from using 2+ cores for compilation. But if MemTotal is less than 2x512MB then it would only get one core by default, defeating the goal of this change.

@uweseimet
Copy link
Contributor

@rdmark Can you test this? If not and you have doubts we should discard the PR and the ticket, because it would not be resolvable if one assumes that wrong memory amounts are reported.

@rdmark
Copy link
Member Author

rdmark commented Oct 7, 2023

I can test it in 2 months. My RPis are all on a long ocean journey right now.

I think we should merge as-is for now. It's an improvement for high end RPis regardless.

My hypothesis is that ~400MB is a better number. In the past, I've used 3 cores with clang and 2 cores with gcc for compiling piscsi on my RPi3B+. But some benchmarking is preferable over anecdotal data.

@uweseimet
Copy link
Contributor

uweseimet commented Oct 7, 2023

@rdmark I'm fine with changing the value from 512 MB to 400 MB or something in between.

@rdmark rdmark changed the title easyinstall: Use 1 compiler thread per 512MB RAM easyinstall: Default no. of compiler threads based on available RAM Oct 7, 2023
@rdmark
Copy link
Member Author

rdmark commented Oct 7, 2023

Using 450MB as divisor now.

@uweseimet
Copy link
Contributor

@rdmark Sounds good. I have already approved, please merge any time.

@rdmark rdmark merged commit 2ced0d3 into develop Oct 7, 2023
6 checks passed
@rdmark rdmark deleted the rdmark-issue-1200 branch October 7, 2023 10:46
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.

2 participants