Collect reverse dependencies

[pepeiborra]

This PR makes the following changes:

• extends Shake to collect the reverse dependencies of every 'Result' by performing some bookkeeping on every evaluation. The amount of work done is O(E) on the number of edges in the graph and I have measured it to be around 3% for a very dense graph with 150.000 nodes and 1e8 edges, so it's not bad at all, and could be put behind a flag if desired. Note that in this PR the set of reverse dependencies is overestimated since ephemeral reverse edges are never deleted.

• adds a new function shakeRunDatabaseFromKeys which extends shakeRunDatabase by accepting, in addition to a set of actions (the goals), a set D of keys that have changed since the last run. The transitive closure of D is the set of all keys that have potentially changed since the last run - all keys that do not belong to this set are guaranteed unchanged and can be excluded from the buildRunDependenciesChanged query.

When the set D is small compared to the size of the build graph, this optimisation renders performance gains of one order of magnitude, up to 70X 20X in some cases. I have extended[1] HLS to keep track of what has changed since the last full Shake run and performed several experiments:

• Applied the ghcide benchmark suite to the standard Cabal-3.0.0.0 example and obtained the results in the comments below. The edit experiment runs in 55% of the time . Full results
• Applied the ghcide benchmark suite to the standard lsp-1.0 example and obtained the results in the comments below. The edit experiment runs in 73% of the time. Full results
• Applied the ghcide benchmark suite to a very large example involving a module with >17.000 transitive dependencies and measured the performance of the edit benchmark, which runs in 4.75% of the time i.e. >21X faster.

[1] - https://github.com/pepeiborra/ide/tree/keysChanged

Closes #800

EDIT: updated performance numbers

[pepeiborra]

There is a tricky issue with async exceptions that I am struggling to solve.

To me, shakeRunDatabaseForKeys is an unsound approximation since it bypasses the version number checks. It is possible that a key K is not up to date and yet, because it doesn't belong to the dirty set, gets reused without updating.

In what cases can this happen? The most common one is when a previous call to shakeRunDatabaseForKeys marked K dirty, but the evaluation didn't touch K so it didn't get recomputed. If the current call to shakeRunDatabaseForKeys isn't aware of this, and if the new dirty set doesn't include K but the evaluation uses it, the stale value will be reused.

To avoid this, I have added state to the database to track the dirty set across runs. A key is removed from the persisted dirty set as it is refreshed. The tricky bit with async exceptions is that a call to shakeRunDatabaseForKeys can get interrupted before it has a chance to add the keys to the dirty set.

I'm not sure how to solve this. The obvious solution is to mask, but it seems ugly and out of place in the Shake codebase. We could put the mask in the caller, by changing shakeRunDatabaseForKeys to return an IO (IO ()), but this probably requires some major changes in Shake. An alternative solution is to track the dirty set in the caller (ghcide) - the problem is that the caller doesn't know a) the transitive closure, and b) when to undirty elements. While we could expose shakeDatabase functions to compute the transitive closure and to inspect the keys that did evaluate in the last run, the end result will probably not be a nice, user friendly API.

[pepeiborra]

There is a tricky issue with async exceptions that I am struggling to solve.

To me, shakeRunDatabaseForKeys is an unsound approximation since it bypasses the version number checks. It is possible that a key K is not up to date and yet, because it doesn't belong to the dirty set, gets reused without updating.

In what cases can this happen? The most common one is when a previous call to shakeRunDatabaseForKeys marked K dirty, but the evaluation didn't touch K so it didn't get recomputed. If the current call to shakeRunDatabaseForKeys isn't aware of this, and if the new dirty set doesn't include K but the evaluation uses it, the stale value will be reused.

To avoid this, I have added state to the database to track the dirty set across runs. A key is removed from the persisted dirty set as it is refreshed. The tricky bit with async exceptions is that a call to shakeRunDatabaseForKeys can get interrupted before it has a chance to add the keys to the dirty set.

I'm not sure how to solve this. The obvious solution is to mask, but it seems ugly and out of place in the Shake codebase. We could put the mask in the caller, by changing shakeRunDatabaseForKeys to return an IO (IO ()), but this probably requires some major changes in Shake. An alternative solution is to track the dirty set in the caller (ghcide) - the problem is that the caller doesn't know a) the transitive closure, and b) when to undirty elements. While we could expose shakeDatabase functions to compute the transitive closure and to inspect the keys that did evaluate in the last run, the end result will probably not be a nice, user friendly API.

I opted for masking in Shake. The masking minimal and happens in shakeRunDatabaseForKeys.

[rayshih]

Can't find the caller where Just value is used. Am I missing something?

[pepeiborra]

This is an external API. See for example how it is used in HLS:

https://github.com/pepeiborra/ide/blob/keysChanged/ghcide/src/Development/IDE/Core/Shake.hs#L758

[pepeiborra]

There is a tricky issue with async exceptions that I am struggling to solve.
To me, shakeRunDatabaseForKeys is an unsound approximation since it bypasses the version number checks. It is possible that a key K is not up to date and yet, because it doesn't belong to the dirty set, gets reused without updating.
In what cases can this happen? The most common one is when a previous call to shakeRunDatabaseForKeys marked K dirty, but the evaluation didn't touch K so it didn't get recomputed. If the current call to shakeRunDatabaseForKeys isn't aware of this, and if the new dirty set doesn't include K but the evaluation uses it, the stale value will be reused.
To avoid this, I have added state to the database to track the dirty set across runs. A key is removed from the persisted dirty set as it is refreshed. The tricky bit with async exceptions is that a call to shakeRunDatabaseForKeys can get interrupted before it has a chance to add the keys to the dirty set.
I'm not sure how to solve this. The obvious solution is to mask, but it seems ugly and out of place in the Shake codebase. We could put the mask in the caller, by changing shakeRunDatabaseForKeys to return an IO (IO ()), but this probably requires some major changes in Shake. An alternative solution is to track the dirty set in the caller (ghcide) - the problem is that the caller doesn't know a) the transitive closure, and b) when to undirty elements. While we could expose shakeDatabase functions to compute the transitive closure and to inspect the keys that did evaluate in the last run, the end result will probably not be a nice, user friendly API.

I opted for masking in Shake. The masking minimal and happens in shakeRunDatabaseForKeys.

I think this masking is not essential and can be removed, since the caller is always able to find out whether a rule was actually evaluated and use this information to keep an up-to-date state of the dirty keys on the caller side.

[pepeiborra]

Closing this since the work has moved on to HLS-graph

[ndmitchell]

Thanks for all the hard work doing this @pepeiborra - it's very nice work. I think the end conclusion is that HLS-graph and Shake have diverged significantly enough that doing one for both isn't all that helpful. I shared some more thoughts about the size of build systems and where Shake fits in https://neilmitchell.blogspot.com/2021/09/reflecting-on-shake-build-system.html