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

build: add load average limit to reduce CPU overcommitment #4094

Merged
merged 13 commits into from
Jan 20, 2020

Conversation

dhewg
Copy link
Contributor

@dhewg dhewg commented Jan 5, 2020

For make and ninja based build systems, no new jobs are started if the load
average is greater than number_of_cores * 1.5.

Building an image on a 8-Core CPU, so using a load limit of 10.00 (earlier version of the patch which used * 1.25):

Before After
without-load with-load
without-avail with-avail

(that's with building on tmpfs and autoremove)

@MilhouseVH
Copy link
Contributor

[ -z "${CONCURRENCY_MAKE_LEVEL}" ] && export CONCURRENCY_MAKE_LEVEL=$(nproc)
[ -z "${CONCURRENCY_LOAD}" ] && export CONCURRENCY_LOAD=$(python3 -c "import os; print('%.2f' % (os.cpu_count() * 1.25))")

Is (or maybe, should?) CONCURRENCY_LOAD be linked to CONCURRENCY_MAKE_LEVEL?

For instance, if the user forces CONCURRENCY_MAKE_LEVEL to be 4 (on an 8 core CPU) should the defaultCONCURRENCY_LOAD then become 4 * 1.25, otherwise they'll still be over-commited by default (at 10)?

In which case we could use this:

[ -z "${CONCURRENCY_MAKE_LEVEL}" ] && export CONCURRENCY_MAKE_LEVEL=$(nproc)
[ -z "${CONCURRENCY_LOAD}" ] && export CONCURRENCY_LOAD=$(echo "scale=2; ${CONCURRENCY_MAKE_LEVEL} * 1.25" | bc)

Also, I'd like to allow a way to completely disable CONCURRENCY_LOAD, for example CONCURRENCY_LOAD=0 could mean we don't apply -l at all.

@MilhouseVH
Copy link
Contributor

MilhouseVH commented Jan 5, 2020

With this PR I think we can now lose this line, as it's redundant:

NINJA_OPTS=""

@MilhouseVH
Copy link
Contributor

Could we also replace NINJA_OPTS with NINJA_FLAGS (exporting the latter, then removing from scripts/build, and mariadb and llvm packages), as this is functionally equivalent to MAKEFLAGS: ninja-build/ninja#1399

Alternatively I'd be happy for this to be addressed in a follow-up PR.

@dhewg
Copy link
Contributor Author

dhewg commented Jan 6, 2020

  • dropped the NINJA_OPTS line from config/optimize
  • allow CONCURRENCY_LOAD=0 to disabled load limiting
  • use number_of_cores * 1.5 per default, as build times regressed on some setups

@dhewg
Copy link
Contributor Author

dhewg commented Jan 6, 2020

I didn't use NINJA_FLAGS since that isn't upstream and it looks like it won't get merged

@HiassofT
Copy link
Member

HiassofT commented Jan 6, 2020

I did a test on my laptop (i7-3740QM, 16GB RAM) and RPi4 build time with this PR was about the same as for plain master (01:26:54.453 vs 01:27:43.468 for master).

I recorded available memory (MemAvailable from /proc/meminfo) and load (1minute average, first value from /proc/loadavg) and it looks available memory is nothing we need to worry about much - at least with the 8 logical CPU cores I tested with (could be different for 32/64 core systems). Max memory usage was about 4GB in both cases, with the PR the memory usage peak near the end of the build wasn't observable though.
master
pr4094

@MilhouseVH
Copy link
Contributor

I didn't use NINJA_FLAGS since that isn't upstream and it looks like it won't get merged

Yeah, thanks for being on the ball - I looked at the ninja PR and thought to myself "this seems like a no-brainer" and that it had been merged. Little did I know the drama that was to follow, including an actual fork. Crazy. :(

@dhewg dhewg force-pushed the pull/load branch 5 times, most recently from 493c3de to 4832383 Compare January 11, 2020 14:52
@dhewg
Copy link
Contributor Author

dhewg commented Jan 14, 2020

Updated screen, mame and mame2016 patches.
Because we need to patch that horrific genie we need to fix it's compilation and because of that the cross compilation.

@dhewg dhewg force-pushed the pull/load branch 3 times, most recently from d40ee77 to 0c4d093 Compare January 16, 2020 07:44
dhewg added 13 commits January 17, 2020 09:01
Don't set PTR64=0 on 32bit archs, instead set ARCHITECTURE to an empty
string. That way we don't need to patch out hardcoded -m32 arguments.

While at it, disable the bgfx hw renderers, or that pile of build crap
tries to include X11 headers.
Yay, more missing dependencies:
In file included from ../../../../../src/mame/audio/taito_zm.h:14,
                 from ../../../../../src/mame/drivers/zn.cpp:15:
../../../../../src/devices/cpu/tms57002/tms57002.h:208:10: fatal error: ../../emu/cpu/tms57002/tms57002.hxx: No such file or directory
Replace the current patch with the same approach as libretro-mame:

Don't set PTR64=0 on 32bit archs, instead set ARCHITECTURE to an empty
string. That way we don't need to patch out hardcoded -m32 arguments.

While at it, disable the bgfx hw renderers, or that pile of build crap
tries to include X11 headers.
The make dependecies are a mess, take no chances.
Occasionally it attempts to link the plugin against the library before the
library is linked.
For make and ninja based build systems, no new jobs are started if the load
average is greater than number_of_cores * 1.5.
@HiassofT HiassofT merged commit 2cb65bb into LibreELEC:master Jan 20, 2020
@dhewg dhewg deleted the pull/load branch January 21, 2020 06:31
@eli-schwartz
Copy link

Yeah, thanks for being on the ball - I looked at the ninja PR and thought to myself "this seems like a no-brainer" and that it had been merged. Little did I know the drama that was to follow, including an actual fork. Crazy. :(

The independent Ninja build language reimplementation in C, at https://github.com/michaelforney/samurai, supports this via the SAMUFLAGS environment variable.

@MilhouseVH
Copy link
Contributor

@eli-schwartz

s 😄

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.

4 participants