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] enable concurrent compilation #2888

Merged
merged 1 commit into from
May 20, 2019
Merged

Conversation

ahorek
Copy link
Contributor

@ahorek ahorek commented May 19, 2019

this significantly reduce build time

@ahorek ahorek changed the title enable concurrent compilation [build] enable concurrent compilation May 19, 2019
@mgreter
Copy link
Contributor

mgreter commented May 19, 2019

IMO this is not really portable, what about mingw on windows?

@ahorek
Copy link
Contributor Author

ahorek commented May 19, 2019

yes, I was worried about the same thing. mingw on windows doesn't really support concurrent compilation, but it won't fail, the option is just ignored.

@mgreter
Copy link
Contributor

mgreter commented May 19, 2019

Actually mingw does support concurrent compilation, but nproc is not installed and will therefore fail.

@ahorek
Copy link
Contributor Author

ahorek commented May 19, 2019

-j option is supported, but it has no effect on windows, it's still single threaded (gcc version 8.2.1 mingw)

I've changed that to -j 0 (autodetect), so now the code doesn't rely on nproc

@mgreter
Copy link
Contributor

mgreter commented May 19, 2019

I use gmake quite regularly and used dmake before (it uses -PXX insteal of jXX).
It definitely does compile in parallel, not sure which make flavor you mean that ignores it.
Otherwise I'm fine if 0 means autodetect, but could you please add a more descriptive
title/description to the commit? Always useful to have meaningful git logs! Thanks!

before
real    2m18.077s
user    1m55.516s
sys     0m21.719s

after
real    0m30.610s
user    2m34.688s
sys     0m28.359s
@ahorek
Copy link
Contributor Author

ahorek commented May 19, 2019

done, I've added a benchmark aswell. On my AMD FX-8300 8C it's 4,6x faster.

@xzyfer xzyfer merged commit 21d2d16 into sass:master May 20, 2019
@xzyfer
Copy link
Contributor

xzyfer commented May 20, 2019

Thanks

@richardwhiuk
Copy link

richardwhiuk commented Aug 19, 2019

Surely this is the wrong fix - shouldn't the caller control the fanout by passing make -j 0 if they want this behaviour.

Currently this causes unbounded fan out, which is deeply wrong, and it ignores a caller passed in setting - e.g. make --jobs 1 will be overriden by this change - which is happening when the Rust build attempts to control fanout to the number of CPUs - see compass-rs/sass-rs#53

Compilation on my box now regularly fails with:

STDERR:virtual memory exhausted: Cannot allocate memory
make: *** [src/ast_sel_cmp.o] Error 1
make: *** Waiting for unfinished jobs....
virtual memory exhausted: Cannot allocate memory
virtual memory exhausted: Cannot allocate memory
virtual memory exhausted: Cannot allocate memory
virtual memory exhausted: Cannot allocate memory
virtual memory exhausted: Cannot allocate memory
virtual memory exhausted: Cannot allocate memory
virtual memory exhausted: Cannot allocate memory
make: *** [src/cssize.o] Error 1
make: *** [src/fn_numbers.o] Error 1
make: *** [src/fn_selectors.o] Error 1
make: *** [src/extend.o] Error 1
make: *** [src/inspect.o] Error 1
make: *** [src/remove_placeholders.o] Error 1
make: *** [src/fn_utils.o] Error 1

@xzyfer
Copy link
Contributor

xzyfer commented Aug 19, 2019

Sounds reasonable. Open to reverting this change.

@ahorek
Copy link
Contributor Author

ahorek commented Aug 19, 2019

ok, sorry, I tested again and you're right. -j 0 is indeed unlimited. The original nprocs limit was removed because it's hard to get the value safely on all platforms. I've reverted the change in #2977

btw optflags https://github.com/sass/libsass/pull/2977/files#diff-b67911656ef5d18c4ae36cb6741b7965L19 also can't be overridden.

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