Make/build cut-off #4477
Make/build cut-off #4477
Conversation
|
I haven't looked at the code yet, but I wonder: why a new PR instead of building on top of #4339? That code has been running in production for months now. |
This PR uses mostly a different approach. For example, it doesn't assume changes in externs format, etc. It provides an overview of concrete changes and explains those decisions. Also, if I'm not mistaken #4339 was refered as abandoned lately by the author. |
|
Sorry. I've failed to explain it properly, because multiple people have looked at the code in that PR without reading the comment thread first. I'd be more than happy for you to take a 4th stab at this, with or without the code I've written. The important thing is that we get a faster compiler. https://github.com/drathier/purserl is where the caching fork lives, since about 6 months. It's a rewrite of the linked PR into something that's more likely to be mergable, but I've given up on getting it merged. Plus I've merged in the purerl code gen into the same binary, halfing compile times for erlang projects. We're using it daily at work and it's very useful. Merging in main every release is much less work than getting it merged, so that's what I'm doing. |
No problem, I understand that you are using your solution in your fork, I just read that you said that code in PR itself was abandoned. I've been working on this for quite a while, even before I saw #4339. I first started working on this because I wanted to cut off the loading of unnecessary extern files during re-builds, then I saw that I could cut off the build plan itself. In #4339 I saw a solution and design decisions that were not clear to me (something that is called
You could build you version on top of this PR and compare the results. @drathier And you also are probably the person who can competently review this PR. |
|
Yes, I'll try to find the time to review it this week. The main idea behind the caching strategy we're using is:
Step 1 and 2 are very hard to validate, as the information is lost during compilation (e.g. how do you know what type alises are you depending on when they've all been desugared away?). https://youtu.be/BQVT6wiwCxM?t=762 is a useful high-level reference table for different caching strategies. It's also in the paper https://www.microsoft.com/en-us/research/uploads/prod/2018/03/build-systems.pdf . |
|
If to explain the strategy I use in a couple of sentences: When we have externs of a module then know all the things that dependants can use, and by comparing with previous externs results we know what exported things have been changed/removed/added. When we decide if to recompile a module, we have all this information about its dependencies, so we need to determine if the module uses any of those things (exported from dependencies) that have changed. Cached things that we already have here: previously built products with externs files. |
There was a problem hiding this comment.
I'm sure this will be better than all of the previous attempts, and I'm looking forward to getting this merged into main. The base approach seems to be the same as the one I was using, and lots of changes overlap between the two. There's lots of tests, which makes me confident that it's going to be fairly stable from day one. Main focus of this review at this stage is to make the code easier to read, so that others can understand it quickly later.
In academic software literature, a constant review speed of 200 lines an hour across all languages sure sounds slow, but here I am, spending 5 hours reviewing 1240 lines of code, some of which are renames. It keeps being spot on.
| -- | Traverses imports and returns a set of refs to be searched though the | ||
| -- module. Returns Nothing if removed refs found in imports (no need to search | ||
| -- through the module). If an empty set is returned then no changes apply to the | ||
| -- module. |
There was a problem hiding this comment.
"no need to search through the module" please be explicit about why
| -- | Check if type name is a type class dictionary name. | ||
| isDictName :: P.ProperName a -> Bool | ||
| isDictName = | ||
| T.isInfixOf "$" . P.runProperName |
There was a problem hiding this comment.
is this stable over time? perhaps add a comment where the $ is inserted into dict names explaining that this part of the code relies on it being there?
There was a problem hiding this comment.
code seems to be deleted from pr? so it's probably done
tests/TestMake.hs
Outdated
| -- recompiled. | ||
| go optsCorefnOnly `shouldReturn` moduleNames ["Module"] | ||
| go optsCoreFnOnly `shouldReturn` moduleNames ["Module"] | ||
|
|
There was a problem hiding this comment.
Lots of new unit tests, which is nice. Please steal as many as you can from https://github.com/drathier/purserl/blob/single-binary/tests/TestMake.hs . Some of them are from live bugs in that implementation, such as tracking transitive type alises across partially failed builds, or type class definition dependencies kinda not being a breaking change until you expand dicts.
| let modulePath = sourcesDir </> "Module.purs" | ||
| let mPath = sourcesDir </> "Module.purs" |
There was a problem hiding this comment.
This is already a large-ish PR, and I would prefer it if renames like this weren't included, at least as part of this work. (I'm not arguing that one or the other name is better; just that it's not so bad the way that it is that it needs changing.)
| writeFileWithTimestamp :: FilePath -> UTCTime -> T.Text -> IO () | ||
| writeFileWithTimestamp path mtime contents = do | ||
| writeFile :: FilePath -> UTCTime -> T.Text -> IO () | ||
| writeFile path mtime contents = do |
src/Language/PureScript/Make.hs
Outdated
| -- again. | ||
| -- | ||
| -- This version will collect an return all externs of all passed modules. | ||
| make' :: forall m. (MonadBaseControl IO m, MonadError MultipleErrors m, MonadWriter MultipleErrors m) |
There was a problem hiding this comment.
Doesn't this case correspond to the original behavior of make? If so, this function should be called make and the other one makeAndReturnUsed (or something); I don't think the ' is clear enough in this case to distinguish the two behaviors, particularly if the ' is the old behavior and the bare make is the new one.
I would also be fine with having makeImp be the new make and using one or the other of the constructors of ExternsFileCollectionPolicy (per Filip's suggestion; or whatever name you end up using) at every call site, instead of going through these two auxiliary functions.
|
❗ The thing to discuss: currently I have added a progress message Also, there is question maybe we should somehow output the reason why the module is not skipped (output a thing changed used) - don't know if it is feasable. I think this is needed just for debug purposes. |
I don't think messages about skipping are particularly useful outside of debugging. Compiling is useful because it's an indicator of work being done. |
|
Printing the reason why something recompiled helps people debug slow compile times. Likewise printing if compilation of a particular module took more than e.g. 10s, or printing the 10 slowest-to-compile modules. If it's easy to add something like this in, I think we should do it. Otherwise, it's a nice change in its own PR. We could add lots of debug info to the output and enable that feature using an envvar or cli flag, for example. |
|
Make method API question for discussion. Currenlty from To speed up the process, we may skip loading/parsing of significant parts of externs that are not needed for the build (preload externs only for updated modules and their transitive dependencies), which is implemented in this PR and there is an option to run make that will preload and return externs for all the modules. Currently
Here is where returned externs are used:
The question is what would be the better API to accommodate existing cases. Here are some options:
Upd:
|
wclr
force-pushed
the
make-cutoff
branch
2 times, most recently
from
42c9b5f
to
d42172a
Compare
|
Lmk when you want another review pass :) Please fix or reply to all review comments before |
I believe I answered all the comments, not sure if I skipped some. I made updates to the code so you may review it. |
|
Hey all. Once the row error message improvements get in, this is something else I plan to look at. |
|
Sorry about the late response. Life happened.
Other than that, there's a bunch of code quality stuff, but I would be happy to adress those in another PR and get this merged and beta-shipped. Rewriting history really messes with code review change tracking, so it looks like there's 30 or so comments which are incorrectly marked as outdated. Please don't rebase/squash going forward, because of this problem |
I don't remember squashing or rebasing after the comments were made, I added a couple of new commits to address the problems. |
|
To clarify, in case there was a misunderstanding. In order to merge this, I'd like at least the linked tests to be ported over, as they're actual regressions seen in that other implementation. @wclr There are many comments without replies, which I'd be happy to see a reply to too, but the regression tests are the minimum imo. |
|
Yes, I need to review the tests, as I mentioned in some reply to comments all the tests from your fork have passed, I will look closer at those you linked above.
I think I addressed all the meaningful problems mentioned in the comments, maybe I accidentally missed some. You can probably review your comments again and resolve those that are not actual anymore. |
|
@wclr I did a pass over my review comments to see which ones were done. I'm apparently not allowed to mark my own comments as resolved, and it felt weird deleting comments instead, so I've commented on them again pointing out if they're fixed or not. Main thing missing is porting the regression tests linked here: #4477 (comment) After that, I'd be happy to merge this, and I'm really looking forward to having much better caching in the mainline compiler! |
|
@wclr Should this be reviewed again? |
|
I believe @MonoidMusician had started reviewing this recently. From reading above I think there's still an outstanding point about some ported regression tests |
|
Yup I've been looking at this. @drathier are you still unable to resolve your own comments / would you like me to mark them as resolved? |
There was a problem hiding this comment.
I'm still going through some of the logic, but it's looking good to me so far.
- Regarding logs: I'd be happy to have a log file in
./$OUTPUT_DIR/last_build.txtspecifically for debugging why modules are being rebuilt or skipped. That way users can have some visibility into what's going on, and it can always exist so they can debug a build retroactively and we don't have to ask them to reproduce it. - I still need to think about how it interacts with cacheDb. Is there a particular reason to use timestamps on the externs as well? I guess better interaction with ide?
- Mostly a note to self: need to think about how this interacts with builds canceled with ctrl+c.
- From our testing, builds that fail with errors seem to rebuild everything downstream. I guess that's because those modules get deleted from the cacheDb? Would extending it with a field indicating failure help?
A few specific code comments inline.
| stripCtorType x = x | ||
|
|
||
| searches' = S.map (map stripCtorType) searches | ||
| check = (\x -> [x | x]) . flip S.member searches' . toSearched |
There was a problem hiding this comment.
I believe this code should use Any instead of [Bool]
|
@MonoidMusician Thanks for reviewing this. Regarding your comments/suggestions:
I'm not against such a log. To do this we need to decide on the information needed in this file. I may propose something like this: To help further review, I will point out two main reasons why cut/off may fall short:
These are the points that should be thoroughly tested, though eventually it should not be difficult to have a complete set of cases, the current set should be reviewed/revised.
Cache-db contains information on source modules, timestamps and hashes so we can check if we have already performed the build for the current module state. Timestamps on externs are used to determine previous result compilation time, so we can check if dependencies where built after their parents or not. This may be the case, for example, when purs-ide performs fast-rebuild of a changed module, and then during the build, although the hash for the module in cache-db is valid, we should invalidate all its dependencies.
It will produce the results for some modules and cache-db won't be changed, the next build will fix everything, but all the modules need to be rebuilt again. This leads to the question of necessity and convenience of a single cache-db file. I think we can eventually deprecate it in favor of per module meta files. Though we will probably need a single build result summary file for other purposes (like ide checks). This is just a thought for the future.
That's right, after the build with errors failed modules are removed from cache-db as well as their dependencies (as they are treated as skipped jobs). So the next time we run the build it will invalidate all the modules that are not in cache-db. It is an important case that I didn't address, but I did encounter it periodically. This should be easy to fix though, we will just not remove those entries from cache-db and will treat them as successful compilations if the module is up-to-date. |
…tests updated and commented.
|
Thanks for the updates! That sort of information looks good for the build log. I still need to review some of the overall logic. But the details are looking good so far. For the declaration traversals, I found a fair bit that was missing from them. I would like to see this use the existing traversals, Here's what I'm thinking of: diff --git a/src/Language/PureScript/Make/ExternsDiff.hs b/src/Language/PureScript/Make/ExternsDiff.hs
index 910fd1a9..165e81c5 100644
--- a/src/Language/PureScript/Make/ExternsDiff.hs
+++ b/src/Language/PureScript/Make/ExternsDiff.hs
@@ -241,21 +241,16 @@ checkDiffs (P.Module _ _ _ decls exports) diffs
-- Goes though the module and try to find any usage of the refs.
-- Takes a set of refs to search in module's declarations, if found returns True.
checkUsage :: Set (Maybe ModuleName, Ref) -> [P.Declaration] -> Bool
-checkUsage searches decls = foldMap findUsage decls /= mempty
+checkUsage searches decls = anyUsages
where
- findUsage decl =
- let (extr, _, _, _, _) = P.everythingWithScope goDecl goExpr goBinder mempty mempty
- in extr mempty decl
-
- toSearched = (,) <$> P.getQual <*> P.disqualify
-
- -- To check data constructors we remove an origin type from it.
+ -- To check data constructors we remove an origin type from it (see `checkCtor`).
+ searches' = S.map (map stripCtorType) searches
emptyName = P.ProperName ""
stripCtorType (ConstructorRef _ n) = ConstructorRef emptyName n
stripCtorType x = x
- searches' = S.map (map stripCtorType) searches
- check = Any . flip S.member searches' . toSearched
+ -- Search using the `Any` monoid (disjunction)
+ check q = Any $ S.member (P.getQual q, P.disqualify q) searches'
checkType = check . map TypeRef
checkTypeOp = check . map TypeOpRef
@@ -264,31 +259,30 @@ checkUsage searches decls = foldMap findUsage decls /= mempty
checkCtor = check . map (ConstructorRef emptyName)
checkClass = check . map TypeClassRef
- onTypes = P.everythingOnTypes (<>) $ \case
- P.TypeConstructor _ n -> checkType n
- P.TypeOp _ n -> checkTypeOp n
- P.ConstrainedType _ c _ -> checkClass (P.constraintClass c)
- _ -> mempty
+ -- Two traversals: one to pick up usages of types, one for the rest
+ Any anyUsages =
+ foldMap checkUsageInTypes decls
+ <> foldMap checkOtherUsages decls
- foldCtor f (P.DataConstructorDeclaration _ _ vars) =
- foldMap (f . snd) vars
+ -- A nested traversal: pick up types in the module then traverse the structure of the types
+ (checkUsageInTypes, _, _, _, _) =
+ P.accumTypes $ P.everythingOnTypes (<>) $ \case
+ P.TypeConstructor _ n -> checkType n
+ P.TypeOp _ n -> checkTypeOp n
+ P.ConstrainedType _ c _ -> checkClass (P.constraintClass c)
+ _ -> mempty
- constraintTypes =
- foldMap (\c -> P.constraintArgs c <> P.constraintKindArgs c)
+ checkOtherUsages =
+ let (extr, _, _, _, _) = P.everythingWithScope goDecl goExpr goBinder mempty mempty
+ in extr mempty
goDecl _ = \case
- P.TypeDeclaration t -> onTypes (P.tydeclType t)
- P.DataDeclaration _ _ _ _ ctors -> foldMap (foldCtor onTypes) ctors
- P.TypeSynonymDeclaration _ _ _ t -> onTypes t
- P.KindDeclaration _ _ _ t -> onTypes t
P.FixityDeclaration _ (Right (P.TypeFixity _ tn _)) ->
checkType tn
P.FixityDeclaration _ (Left (P.ValueFixity _ (P.Qualified by val) _)) ->
either (checkValue . P.Qualified by) (checkCtor . P.Qualified by) val
- P.TypeClassDeclaration _ _ _ cs _ _ ->
- foldMap onTypes (constraintTypes cs)
- P.TypeInstanceDeclaration _ _ _ _ _ cs tc sts _ ->
- foldMap onTypes (constraintTypes cs <> sts) <> checkClass tc
+ P.TypeInstanceDeclaration _ _ _ _ _ _ tc _ _ ->
+ checkClass tc
_ -> mempty
isLocal scope ident = P.LocalIdent ident `S.member` scope
@@ -298,7 +292,6 @@ checkUsage searches decls = foldMap findUsage decls /= mempty
| otherwise -> checkValue n
P.Constructor _ n -> checkCtor n
P.Op _ n -> checkValueOp n
- P.TypedValue _ _ t -> onTypes t
_ -> mempty
goBinder _ binder = case binder ofA couple tests (the second case of a type in a pattern was not handled): -- Type synonym change in value.
recompile2 it "type synonym changed in value"
( "module A where\ntype SynA = Int\n"
, "module A where\ntype SynA = String\n"
, "module B where\nimport A as A\nvalue = ([] :: Array A.SynA)\n"
)
-- Type synonym change in pattern.
recompile2 it "type synonym changed in pattern"
( "module A where\ntype SynA = Int\n"
, "module A where\ntype SynA = String\n"
, "module B where\nimport A as A\nfn = \\(_ :: Array A.SynA) -> 0\n"
)For full coverage, we would need similar tests for foreign imports (values and types) and kind annotations. |
|
@MonoidMusician I've added your suggestions, I agree that it would be better not to duplicate the logic if possible. Added some tests.
I believe those couple tests didn't pass because the finding types logic was not complete, now it should be (as |
This is an attempt to address the problem concerned in #3724. It was aslo addressed lately in #4339. With this change, the compiler will only rebuild downstream modules if they are affected by changes introduced after the previous compilation. As this is a significant build infrastructure related change I will explain the meaningful details and design decisions below.
How build/make process currently works
makegets that list of parsed modules, builds the modules dependency graph, constructs a build plan (Language.PureScript.Makemodule).Language.PureScript.Make.BuildPlanmodule), it loads existing externs files for up-to-date modules (unchanged since the previous compilation - by checking file hashes againstcahce-db) and gathers the timestamps (previous compilation time). Based on the timestamps it determines which modules should be rebuilt. Loaded externs are used as prebuilt results in the build process.makecompiles modules to be rebuilt.Proposed changes in the make/build process
While constructing the build plan, avoid loading/parsing extern files that are not needed. We just gather timestamps (compilation time) that are needed to determine what modules have to be built.
Then preload only needed externs: transitive dependencies of modules that need to be rebuilt, and also previously built results for modules to be rebuilt (that will be used if the module does need to be recompiled or for checking if externs have changed).
While module's rebuild phase check if externs of any of its dependencies have effectively changed, if not then (if the previously built result is available) we skip the compilation step.
Avoiding loading exessive externs files
Currently externs for all the modules are loaded during construction of a build plan. Though loading/parsing those extern files may require significant (perceived) time though they are not even required for the build environment.
I noticed this problem when I added to my project a dependency with large externs files with lots of definitions (externs ~30MB in size). After just adding this package, the empty build running time (without any compilation needed) increased to more than 3 sec. With the change introduced (when those externs are not loaded) empty buuid for the described case runs for ~1 sec now.
Changes to make API
There is a small change required (because of the above change) that adds two different make methods, that returns only updated/used externs and externs for all modules (which is needed for PSCI).
CacheDb and illegal compiler version invalidation
Currently using info from cache-db we invalidate changed files (using timestamp/hash). Build products of incorrect compiler versions are invalidated while loading extern files. However, in the proposed changes, we also cut off the loading of not needed externs files, and we need a way to invalideate wrong compiler version products. The proposed implemented method does this by placing a compiler version in the cache-db file.
Externs Diff
This is probably the most intricate part of the change, though essentially it is quite obvious. To know if the module has some changes, we compare externs and store the difference in so called
ExternsDiff. When building a module we check if any of its dependencies have changed and if it uses any of those changes, if it does we rebuild it, if not we skip it. So, there are two problems to solve:correctly determine the changes in externs - this is done by comparing structure of old and new extern's entries and considering special cases of indirect dependencies such as reexports and changes in type instances.
correctly check if any of the changes in dependencies affect the module - to solve this we check if the module effectively uses any of the changed elements from the imported dependencies by traversing the module's content and searching for the first occurrence of a changed element.
Tests
To test this change comprehensively it is needed for all the elements that can be exported to check that their changes cause the rebuild for the downstream. Also check that non-effective changes do not cause the rebuild.
Another thing that needs to be tested is that while traversing the module it doesn't skip the usage of a changed element.
Existing tests have been updated to make them a bit more compact and readable.
Status
The PR is ready for review and testing. The test spec for the change is probably quite complete, though it will need a further clean up and probably some additions considering the findings and suggestions.
Checklist: