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: Improve consistency of parallelism flags among make->python dependencies #20065

Closed
chrismilleruk opened this issue Apr 16, 2018 · 3 comments

Comments

@chrismilleruk
Copy link
Contributor

  • Version: 10.0.0-rc
  • Platform: all
  • Subsystem: build

make can run multiple jobs in parallel by using the MAKEFLAGS env var or the -j flag (e.g. make -j4).
test.py runs in parallel through use of the JOBS env var or the -J or -j flags.

When test.py is called from make it uses a few different mechanisms to pass the parallelism factor which makes the build hard to reason about.

We should make the usage within Makefile more consistent so that it is easier to ensure that the build is working as fast as possible.

See also: #19919 (comment)

@chrismilleruk
Copy link
Contributor Author

@rvagg commented:

You can -j X with make but it doesn't propagate to tools/test.py, it only does the build in parallel. The call to tools/test.py is given a -J, it doesn't pass X on. Here's what test.py does when it's run with -J:

If JOBS is set in the current environment then use that as the parallel count
If there is no sensible JOBS env var then use the Python multiprocessing.cpu_count() call which can be inaccurate on some VMs or containers (like os.cpus().length .. SmartOS is one of these).
So, what we tend to do in CI is be explicit about it, with something like:

if [ -z ${JOBS+x} ]; then
  JOBS=$(getconf _NPROCESSORS_ONLN) #reported cores, kind of like `os.cpus().length`
fi
JOBS=$JOBS make test-ci -j $JOBS

And in some configurations we explicitly set a JOBS, like on SmartOS and our 96-core ARMv8 hosts that shouldn't be running with a parallellism of 96.

My wish is that -j X was definitive for both building and testing, but I don't believe you can fetch the -j value from inside a Makefile and you'd have to do some ppid hackery to look it up from the tools/test.py subprocess.

@chrismilleruk
Copy link
Contributor Author

chrismilleruk commented Apr 16, 2018

I noticed three different methods for calling test.py from Makefile

(1) Called with the -J flag (4 times)
e.g. $(PYTHON) tools/test.py --mode=release parallel -J

(2) Called with PARALLEL_ARGS (3 times)
e.g. $(PYTHON) tools/test.py $(PARALLEL_ARGS) ...

(3) Called without -J or PARALLEL_ARGS (17 times)
e.g. $(PYTHON) tools/test.py --mode=debug,release

The -J flag is defined here:

  if options.J:
    # inherit JOBS from environment if provided. some virtualised systems
    # tends to exaggerate the number of available cpus/cores.
    cores = os.environ.get('JOBS')
    options.j = int(cores) if cores is not None else multiprocessing.cpu_count()

PARALLEL_ARGS is defined here:

ifdef JOBS
  PARALLEL_ARGS = -j $(JOBS)
endif

These appear to be more or less the same except that -J falls back to a sensible default (number of cores).

So I have two key questions:
(1) Should -J and PARALLEL_ARGS be merged / normalised or is there a good reason to keep both?
(2) Should the non-parallel calls be made parallel (for consistency and/or performance)?

@rvagg
Copy link
Member

rvagg commented Apr 18, 2018

Yeah, I think it could be normalised. Maybe we go with PARALLEL_ARGS and default it to (a) -j$JOBS then (b) -J. It looks like tools/lint-js.js takes both, like tools/test.py so that should be safe I think. I'm not seeing a good reason why we wouldn't just pass on PARALLEL_ARGS to all instances of test.py but perhaps I'm overlooking something.

chrismilleruk added a commit to chrismilleruk/node that referenced this issue Apr 18, 2018
jasnell pushed a commit that referenced this issue Apr 23, 2018
PR-URL: #20124
Fixes: #20065
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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 a pull request may close this issue.

2 participants