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: spawn make test-ci with -j1 #23733

Merged
merged 1 commit into from
Oct 20, 2018
Merged

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 18, 2018

All the subtasks have internal parallelism, so there is minimal loss.
Also make doesn't do a good enough job of combining the output
streams, or eliminate races.

Fixes: #22006
Ref: #23423 (comment)
Ref: #23286 (comment)

/CC @nodejs/build @nodejs/build-files

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 18, 2018
@refack refack mentioned this pull request Oct 18, 2018
3 tasks
@refack refack self-assigned this Oct 18, 2018
@Trott
Copy link
Member

Trott commented Oct 18, 2018

@refack
Copy link
Contributor Author

refack commented Oct 18, 2018

@mmarchini
Copy link
Contributor

Do we know how much time a CI run takes with and without -j1?

@refack
Copy link
Contributor Author

refack commented Oct 18, 2018

Do we know how much time a CI run takes with and without -j1?

Good question (I don't see any significant change)

with -j1:

Job Build # Duration
node-compile-windows build #21674 ( 46 min )
node-test-binary-windows build #20858 ( 13 min )
node-test-commit build #22448 ( 1 hr 40 min )
node-test-commit-arm build #19316 ( 39 min )
node-test-commit-custom-suites-freestyle (test-worker) build #1730 ( 8 min 16 sec )
node-test-commit-freebsd build #21307 ( 18 min )
node-test-commit-linux build #22483 ( 1 hr 40 min )
node-test-commit-linux-containered build #7947 ( 1 hr 39 min )
node-test-commit-linuxone build #6326 ( 6 min 41 sec )
node-test-commit-osx build #22011 ( 1 hr 3 min )
node-test-commit-plinux build #21033 ( 8 min 32 sec )
node-test-commit-smartos build #21042 ( 31 min )
node-test-commit-windows-fanned build #21620 ( 1 hr 2 min )
node-test-linter build #22905 ( 5 min 2 sec )

Without (daily master

Job Build # Duration
node-compile-windows build #21670 ( 45 min )
node-test-binary-windows build #20854 ( 13 min )
node-test-commit build #22444 ( 1 hr 23 min )
node-test-commit-arm build #19312 ( 36 min )
node-test-commit-custom-suites (test-internet) build #724 ( 1 min 59 sec )
node-test-commit-custom-suites-freestyle (test-worker) build #1725 ( 9 min 5 sec )
node-test-commit-freebsd build #21303 ( 17 min )
node-test-commit-linux build #22479 ( 29 min )
node-test-commit-linux-containered build #7944 ( 1 hr 22 min )
node-test-commit-linuxone build #6319 ( 6 min 27 sec )
node-test-commit-osx build #22007 ( 18 min )
node-test-commit-plinux build #21029 ( 8 min 18 sec )
node-test-commit-smartos build #21038 ( 27 min )
node-test-commit-windows-fanned build #21616 ( 59 min )
node-test-linter build #22901 ( 4 min 25 sec )

Makefile Outdated Show resolved Hide resolved
@mmarchini mmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 19, 2018
@refack refack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 19, 2018
@BridgeAR
Copy link
Member

@refack any reason that this is not author ready?

@Trott
Copy link
Member

Trott commented Oct 19, 2018

@refack any reason that this is not author ready?

No CI since the most recent change?

@refack
Copy link
Contributor Author

refack commented Oct 19, 2018

@refack any reason that this is not author ready?

This need to be followed up in Jenkins, so mainly so this will not be landed by mistake.

All the sub targets have internal parallelism, so no performance loss.
Also `make` doesn't to a good enough job of combining the output
streams, or eliminate races.

PR-URL: nodejs#23733
Fixes: nodejs#22006
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@refack refack merged commit b3ae915 into nodejs:master Oct 20, 2018
@refack refack deleted the safer-test-ci branch October 20, 2018 15:07
@refack refack mentioned this pull request Oct 20, 2018
@refack
Copy link
Contributor Author

refack commented Oct 20, 2018

Since this allowed us to restore -j2 to https://ci.nodejs.org/job/node-test-commit-osx, this seems to make runs with low ccache hits, significantly faster.

@refack refack removed their assignment Oct 20, 2018
jasnell pushed a commit that referenced this pull request Oct 21, 2018
All the sub targets have internal parallelism, so no performance loss.
Also `make` doesn't to a good enough job of combining the output
streams, or eliminate races.

PR-URL: #23733
Fixes: #22006
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@refack
Copy link
Contributor Author

refack commented Oct 23, 2018

@targos I think this should be ported to v10.x
See failures in #23827

MylesBorins pushed a commit that referenced this pull request Oct 29, 2018
All the sub targets have internal parallelism, so no performance loss.
Also `make` doesn't to a good enough job of combining the output
streams, or eliminate races.

PR-URL: #23733
Fixes: #22006
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@MylesBorins
Copy link
Contributor

landed in 10.x in 2cd68be

@rvagg
Copy link
Member

rvagg commented Nov 22, 2018

just catching up on this. Did we end up fixing the addon build make+stdout problems with this change? nice work if so!

@refack
Copy link
Contributor Author

refack commented Nov 23, 2018

Did we end up fixing the addon build make+stdout problems with this change? nice work if so!

Yes, but we have a deeper issue. make node is not idempotent, it unnecessarily builds some stuff for every call. I have a lead on a solution but it's berried in the convoluted #23439.
I really should break that one apart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test: flakiness in building addons with make
9 participants