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

Add an option to amalgamate sources before compilation #2979

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

glebm
Copy link
Contributor

@glebm glebm commented Aug 23, 2019

Amalgamation reduces single-core compilation time by 50% and the shared library size by 10%.

SQLite has a great writeup on amalgamation here: https://www.sqlite.org/amalgamation.html.

Before After
compile time, 1 core 60s 30s
compile time, 8 cores 16s 30s
Travis CI non-autotools 3m 2m
Max RAM usage (RSS) 321M 747M
lib/libsass.so size 3.3M 3.0M
$ g++ --version
g++ (Ubuntu 8.3.0-6ubuntu1) 8.3.0

Commands for measurement:

With amalgamation:

rm -f script/amalgamate/build/amalgamate && make clean AMALGAM=1 && \
  \time -v make lib/libsass.so AMALGAM=1 && du -sh lib/libsass.so

Without amalgamation:

make clean AMALGAM=0 && \time -v make lib/libsass.so AMALGAM=0 && \
  du -sh lib/libsass.so

Also enables amalgamation on CI for non-autotools builds (to make it faster and also test the amalgamated build).


A couple of minor source changes were necessary:

  1. In sass2scss, Sass namespace has been renamed to Sass2Scss to avoid conflicts (upstream PR Rename namespace to Sass2Scss mgreter/sass2scss#43).
  2. Allow src/cencode.c to compile under C++.

See also request for this feature in the Ruby bindings repo: sass/sassc-ruby#132.

@glebm glebm force-pushed the amalgamate branch 7 times, most recently from 0c7df44 to 3692c40 Compare August 24, 2019 07:47
@mgreter
Copy link
Contributor

mgreter commented Aug 25, 2019

AFAIKS these builds are normally called unity builds. Only reason we haven't adopted it so far is memory constraints. Can you @glebm give some numbers how much free memory is needed to compile the "amalgamate" sources? See #2888 (comment)

https://stackoverflow.com/questions/543697/include-all-cpp-files-into-a-single-compilation-unit

@glebm
Copy link
Contributor Author

glebm commented Aug 25, 2019

@mgreter I've added max RSS usage to the comparison table. It's 747M (without it: 321M). Note that in this PR amalgamation is disabled by default.

@glebm glebm force-pushed the amalgamate branch 15 times, most recently from a7be68a to 210fd5a Compare August 27, 2019 08:22
@glebm glebm requested a review from mgreter August 27, 2019 18:05
@mgreter
Copy link
Contributor

mgreter commented Aug 27, 2019

For the review request, I have three main questions:

  • Do we still test the old "separated" build in CI (preferably in all OS configs)?
  • Why can't we just create a compile unit which has #include "source.cpp" for all files?
  • Aka, why the need to the custom/complex amalgamation script?
  • Can we pass it an option to optimize this for e.g. 8 cores=8 compile units of similar size?

1. Avoid include cycles. This isn't necessary for amalgamation but makes
   things more obvious.

2. Rename sass2scss namespace to Sass2Scss (upstream PR
   mgreter/sass2scss#43).

   This is necessary for amalgamation.
Sources are #included instead of inlined by default.
@glebm
Copy link
Contributor Author

glebm commented Aug 28, 2019

Do we still test the old "separated" build in CI (preferably in all OS configs)?

Not in all configs, but we do test the separated build with AUTOTOOLS on Travis and with MSVC on AppVeyor.

Do we want to double the number of test configurations?
I don't think it's really worth doubling the CI time.

Why can't we just create a compile unit which has #include "source.cpp" for all files?
Aka, why the need to the custom/complex amalgamation script?

This is actually what we do by default. The default can be changed by passing AMALGAM_INLINE=1 to make, which passes --inline=true.

The --inline=true mode is for creating a stanadalone libsass.cpp file that can be simply dropped into a project (the include/ directory is also needed as the headers there do not get inlined).

I've now added a description of the --include flag to script/amalgamate/README.md.

Can we pass it an option to optimize this for e.g. 8 cores=8 compile units of similar size?

No but I hope this won't be too difficult to add in the future, though distributing the #includes correctly may be tricky, because here size(A + B) != size(A) + size(B).

@liamwhite
Copy link

Any update?

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.

3 participants