Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upoptimize import desugaring for full builds #3768
Merged
Conversation
|
This looks like a great find! The IDE rebuilds probably don't care as we're never building more than one module there which means we'll need to build the full Env for that module in one go either way. |
|
Really nice work. Thank you very much! |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
Awesome! Thanks all for the reviews |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
colinwahl commentedJan 10, 2020
•
edited
TLDR: This PR introduces an optimization to the import desugaring phase which eliminates duplicated work, and leads to drastically lower build times when you have a large amount of modules.
@natefaubion noticed that clean builds at Awake were starting to take quite a while, so I went and profiled one of them. I ran the results through profiteur and noticed that about 41% of the time (and memory) spent on our build was from the function
externsEnvwithindesugarImportsWithEnv.Some spelunking revealed that
externsEnvwas called once per (transitive) dependency of each module that was being compiled. This resulted in quite a bit of repeated work. For example, if you have modules A, B, C, and D, where the dependency graph looks likeA -> B -> C -> D, then you call externsEnv on the following lists of modules:[A],[A, B],[A, B, C]. Another example is that if every module depends onPrelude, then you are computingexternsEnvonPreludeonce per module.externsEnvcan be decently expensive, and it is easy to imagine that this duplicated work could get out of hand with more modules (we have definitely passed that threshold at Awake, I think we have1300+2000+ modules).I realized that we could use the fact that we are already compiling in topological order along with the fact that
Envs will be disjoint for "non-transitively-dependent" pairs of modules to instead keep a runningEnvwhich is updated as externs files become available, and which never computesexternsEnvon an extern file more than once. In doing this, a tremendous amount of redundant computation is avoided, resulting in a much "healthier" looking profile:Where calls to
externsEnvaccount for 0.2% of the time of a full build, and parsing (~50%), typechecking (~26%) and codegen (~15%) are the dominating items of the profileIn practice, on the Awake codebase, this reduces the time needed for a full build from ~2ms20s to ~1m20s, about a 43% speedup.
The bulk of this change is quite manual and not very invasive - the important bits are in the changes to
buildModule, which will need some eyes on it :) The main change to extends the coordination done inmaketo collect dependencies, and then ensure all dependency's externs are added to theEnvin the correct topological order. Since the coordination to collect dependencies was already done, this ended up being a pretty easy modification.Notes:
make, all other workflows such as psci, purs-ide, etc should not be affected.applyExternsFileToEnvironmentwithinrebuildModule. This may be able to benefit from the same sort of optimization.