POC: More aggressive dirty module checking

[drathier]

NOTE: Read the last comments before looking at code!

Description of the change

Goal:
I wanted to get closer to the compilation performance of the Elm compiler. This is a POC, it's hacky, it seems to work, it's absolutely not ready to be merged in, it's based of an old compiler version (v0.14.3), I'm not even sure it should be merged in, but I hope it shows that this can be done.

The basic idea:
Elm looks at the public api of each module, and if the public api of a module didn't change after recompiling, no downstream dependencies need to be recompiled, because everything they depend on will still be there, but possibly with a different implementation. This only works if you're not doing general cross-module inlining, which neither Elm nor Purescript seems to do.
Thus, modifying a comment on your prelude should result in just a single module being recompiled. Same for modifying a constant, a function body, or adding something new without touching anything already in the file.

Example performance:

adding a space to the end of an average file
5 files are directly depending on the modified file
19 files transitively depend on the modified file
136 files in the project, most of which don't transitively depend on the modified file
16s before this change
2.8s with this change

adding a space to the end of our custom Prelude
127 files are directly depending on the modified file
135 files transitively depend on the modified file
136 files in the project, most of which don't transitively depend on the modified file
44s before this change
4.4s with this change

Some debug prints when trying it out:

cache-none = src file changed or first time build, recompile for sure
cache-noop = recompiled but all artifacts turned out identical, so this was wasted work
cache-miss = recompiled and files changed, so this was useful work
cache-hit = no dependency changed, so don't bother compiling

---

Checklist:

• Added a file to CHANGELOG.d for this PR (see CHANGELOG.d/README.md)
• Added myself to CONTRIBUTORS.md (if this is my first contribution)
• Linked any existing issues or proposals that this pull request should close
• Updated or added relevant documentation
• Added a test for the contribution (if applicable)

[drathier]

Based it off v0.14.5 now

[joneshf]

Heya! Just swinging by to say that this is really cool!

I tried some similar experiments back on 0.13.x around only looking at direct dependencies instead of needing the whole world. I got as far as making a couple of CLIs to help with it:

• purs-compile - a direct replacement for purs compile that added a flag to manually choose pre-compiled dependencies.
• purs-compile-module - a program meant to be orchestrated by more tooling (like bazel or shake) that compiles a single module at a time.

The hope was to try and upstream the concepts once they were proved out to be have net benefits (or at least have such small drawbacks that it was worth it anyway). But then, life happened. In any case, I'm excited to see someone else taking up similar ideas!

I see that you also came up the the idea of wiping out the source locations in order to keep the churn lower. That makes me feel like the idea wasn't a bad one 😅. There were two issues that I ran into that threw a couple of wrenches into the general idea of cut-off: re-exports and typeclass instances. I forget all of what the issues were with re-exports, but I want to say that it would've required a change in the externs format to make the direct dependencies actually useful. I believe it was a solveable problem, just non-trivial. I also only kind of remember what the issues were with typeclass instances, but I wrote some stuff down in an issue: joneshf/purs-tools#4.

I think one thing that was causing a problem with typeclass instances was that the module where a typeclass instance was defined wasn't in the import list of an extern only at the location of use or something. I can't really remember. All I do remember is that if you had a module A that defined a typeclass instance, and two other modules such that the dependencies were C -> B -> A. If C used the typeclass instance from A, then C implicitly had a direct dependency on A, but there wasn't an explicit reference to it in the same way that every other feature in the language had an explicit reference to its dependencies.

I think the other thing with typeclass instances was that in practical terms, it would only really improve stuff if you never changed a typeclass instance. And given how prevalent typeclasses are in the PS ecosystem, a seemingly innocuous change could make it so that recompilation speed was no better than the standard purs compile. So most non-trivial code bases (where you really need the benefit of cut-off) wouldn't really benefit, since it's rare to find a non-trivial codebase that doesn't define its own typeclass instances and use them widely throughout the codebase.

I haven't really been paying attention to PS since 0.13.x, so I'm not sure if these problems have changed. They very well might have. If they haven't, how are you planning to deal with these two things?

[drathier]

My plan was to make a poc, try it out at work, hope that someone here came along and either did the thing properly or gave me enough input to do it properly myself, so thanks for stopping by @joneshf :)

Current issues:

• SourcePos absolutely has to be ignored when caching; it will cache miss on absolutely every change otherwise, like adding a space character to the end of the file. Here I'm zeroing them out, and it hasn't broken any dev tools at work so far (we've been running this on 4 devs + ci for about a month now), but I assume they're used somewhere for something, I just don't know what. Comparing the whole Externs file in encoded format isn't that great either; it was just a hack to get things going.
• Warnings should also be cached
• If I change a module without chaing its public api, it shouldn't trigger recompiles downstream (until we have cross-module inlining, which I don't think we should ever have in dev builds, i.e. when this caching matters). This is the last big hurdle for Elm-quality build caching.
• re-exports and type classes can be tricky, I remember thinking about them when writing this PR but I don't remember any conclusions. If you modify a type class, that file should get recompiled, and all usages of that type class should then get recompiled, and so on, but it's possible that we'd need to think twice about how that works transitively
• Q: is file IO for these hundreds of cache files a perf problem? Syscalls are cheap but not free, and when compile times are down to seconds it might matter. It did for Elm.

[drathier]

Don't code while having a fever, kids. Also, don't reply to stuff on github. Feeling better now.

The best part of all of this to me is that BuildPlan.construct is now takes a non-trivial amount of time to run; sometimes more than 50% of the build time. Everything else is so fast now!

Previous status, as reported in the initial PR description:

Dependency chain

M1 <- M2 <- M3, add a space to M1

• Rebuilds M1 (src changed, nothing to diff against, so no caching)
• Rebuilds M2 (dep changed, output was same as before)
• Cache hits M3 (none of its imports changed)

Current status:

Dependency chain

M1 <- M2 <- M3, add a space to M1

• Rebuilds M1
• Cache hits M2 (M1 output files didn't change after being rebuilt)
• Cache hits M3 (none of its imports were rebuilt)

Modifying custom prelude, renaming ctor in exposed type

• Rebuilds custom prelude, and everything that directly depends on custom prelude
• Cache hits for 11 files which do not import custom prelude

Modifying custom prelude, adding new non-exposed data type

• Rebuilds custom prelude
• Cache hits for everything else (exports are explicit, this type isn't in it)

Things to test:

• modify type class definition
• modify type class instance
• what else?

And how do we write tests for all of this?

[drathier]

It's now tracking the shape of types through the externs files to figure out of we need to recompile, e.g. because a type alias transitively refers to something that changed. Also handling re-exports, which it didn't before. It's quite far from optimal, and I haven't looked at the type class problem @joneshf mentioned, or even figured out what it is or if it's still a thing. I'm still very much exploring the compiler and learning as I go, with many dead ends, but I'm getting there. I'm also intentionally not planning this out ahead of time, to learn my way around the compiler code base, and to keep this not feeling like work.

[nwolverson]

🤔

[i-am-the-slime]

Five times faster?

[drathier]

It's how I keep track of which compiler version is running where. purs --version just reports e.g. 0.15.4 and changing that breaks all of the tooling.

[drathier]

If anyone wants to try it out, there are releases over here: https://github.com/drathier/purescript/releases . I'm using these daily when developing, with the purerl backend. I would say they're alpha quality atm; every once in a while it gets a cache hit when it shouldn't, but it's more helpful than not.

[i-am-the-slime]

@drathier I'd love to try this but ideally with 0.15.7. Any chance you could merge master and only if there's no conflicts cut a new release?

[drathier]

@i-am-the-slime I'll have a release ready soon, I think. This was for trying things out, but the code quality isn't what I'd be happy to merge, so I reimplemented the changes with a more systematic bottom-up approach, fully completing things as I go instead of messing around and fixing stuff that came up. I've cleaned things up over at drathier#5 and based it on a recent master. Tracking changes to type aliases (type synonyms) is tricky as they're all erased by this point, and I'm still invalidating the cache a bit too often. I also haven't implemented re-exports there, but I'm working on it.

[drathier]

0.15.7 based release available at https://github.com/drathier/purescript/releases/tag/v0.15.7-caching-15
0.0.19 based purerl release available at https://github.com/drathier/purerl/releases/tag/v0.0.19-caching-15

• note that that is built from a different branch than this is
• passes ci tests which builds the entire package set
• note that the externs format has changed. Editors usually run whatever purs ide is in path, so it should just work fine if it's in path. You don't have to clear any folders, you'll just get cache misses once any time you switch compiler binary.
• it should track all sorts of changes; operator fixity, whether ctors are used or just the data type, type class declarations and implementations, type aliases (type synonyms), transitive use, re-exports, foreign values and data, etc. I don't expect there to be any category of things which hasn't been implemented, but please do let me know if you find one!

Goals of this release

• find examples of things that don't work and convert them into test cases. The compiler will pretty print a whole bunch of debug info if it ever finds something that looks wrong. I want that info :)
• PURS_DISABLE_EXPERIMENTAL_CACHE=1 causes the compiler to behave like the old compiler, but it will report false positive cache hits, i.e. when the caching logic said it was a cache hit, but then we recompiled, and it turned out it shouldn't have been a cache hit after all. This will find more issues than running the caching, so please test with this too.

Non-goals of this release

• code review and getting things ready to be merged in. Feel free to do a very high level review on the new branch (see release) if you feel like it, but the main goal here is to increase test coverage. We could easily be missing 100 test cases on this change.
• production readiness; feel free to use this when developing locally, and even in ci, but please don't use it in your release builds

[MonoidMusician]

I've been giving it a try today, it seems to work well! Haven't looked at the code yet.

This was fun:

Building...
[  1 of 540] Compiling V15 Erl.Kernel.File
[376 of 540] Compiling V15 OtherModule
Warning 1 of 5:
  ...

[drathier]

Current status of this POC

We're still using this for all development, fyi. I'm mostly waiting for the combination of:

• me having enough free time to work on merging this in
• core team having enough free time to review a change like this
• core team having enough free time to help me decide if this is "good enough" or not. There are quite likely some edge cases where the caching is too optimistic. The repl? Type aliases? Idk. We're deliberately building releases with an empty cache atm, but incorrect cache hits are extremely rare.

Current repo and releases

New development on this is made on https://github.com/drathier/purserl/releases/tag/v0.15.8-caching-30

• that repo has the erlang codegen code added from purerl, as --codegen=erl
• it has all the caching logic from the previous test releases linked from this repo
• it's up to date with the main repo, 0.15.8 at time of writing
• it's actively being developed and used every day by our entire team
• you should be able to run it no problem with a non-erl --codegen too, and get the caching benefits.

[wclr]

As I was too recently dealing with this problem, I have reviewed the code for this PR. Here are some thoughts about it:

• You have to explain the whole solution scheme and design desisions in words: how it works now and how it will work with the changes, providing meaningful implementation details. Without such explanations the problems you are trying to describe here (and asking for opinions) can hardly be understood.

• The code looks quite dirty, with a lot of "dev stuff", it has to be cleaned up. You can have whatever you need for debugging in your dev fork, but not in the PR submitted to be merged/reviewed.

• Overall impression: you took quite a complicated route and you are trying to cope with a very large scope without providing a proven and comprehensive solution. This is what follows from your comments here and the code I saw. Though the general direction of solving the problem is right, the implementation seems a bit too messy.

[rhendric]

Since you were expressing disillusionment with getting your PRs merged on Discourse, here's what's up:

• me having enough free time to work on merging this in

Entirely up to you, friend.

• core team having enough free time to review a change like this

Was there any point in the history of this PR when it didn't come with comments saying ‘don't review this yet’? I don't think so. I'll review it when you ask for a review, unless someone else beats me to it. Feel free to @ me.

• core team having enough free time to help me decide if this is "good enough" or not. There are quite likely some edge cases where the caching is too optimistic. The repl? Type aliases? Idk. We're deliberately building releases with an empty cache atm, but incorrect cache hits are extremely rare.

Not going to pre-review this before it's ready for review, but yeah, that's part of a review. Regardless of whether this approach makes it into the compiler, you wouldn't go to this trouble unless you were experiencing a problem, and either we'll work with you on this approach (for as long as you stay engaged, at least) until it's ‘good enough’, or we'll point you in a different direction and have a conversation with you about whether that alternative would address your problem as well.

We ❤️ contributors and we want you to succeed!