Skip to content

How to handle conditional/optional dependencies? #794

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

Open
wz1000 opened this issue Feb 20, 2021 · 9 comments
Open

How to handle conditional/optional dependencies? #794

wz1000 opened this issue Feb 20, 2021 · 9 comments

Comments

@wz1000
Copy link

wz1000 commented Feb 20, 2021

This is in the context of ghcide, but is more of a general query on how to accomplish this sort of thing with shake, so I'm asking here.

A bit of background first -

  • Compilation via GHC can be run in two modes, either we can just typecheck, producing a ModIface, or we can go all the way and do code-gen, producing a Linkable.
  • This process can't be resumed - we can't go from a ModIface to a Linkable, we need to redo the entire compilation pipeline in order to produce a Linkable
  • We only need Linkables for modules that are depended on by something that uses TemplateHaskell
  • So, we have a rule called NeedsCompilation which analyses the the reverse dependencies of a module to check if we need to do code-gen. It returns True if this is the case, or False if we can stop after generating the ModIface.
  • The GetModIface rule generates a pair (ModIface, Maybe Linkable), where the invariant is that the Linkable is guaranteed to be present if NeedsCompilation returned true. (NeedsCompilation is a dependency of GetModIface)

All this seems to have been working fine. However, the complication is introduced with the eval plugin.

To evaluate an expression in the context of a module, we must have Linkables for it and all of its dependencies.

A first approach at supporting this can be:

  1. When evaluating an expression in the context of a file, somehow mark the file as needing compilation in some global state.
  2. Make NeedsCompilation consult this bit of global state when it makes a decision.
  3. Restart the shake build
  4. If all went well, we would have all the Linkables we require, and we can evaluate the expression
  5. Reset the global state so we don't do unnecessary code-gen on subsequent builds.

This looks reasonable at first, but it means we will just throw away all the Linkables we computed for eval on subsequent builds after evaluation, and we will have to rebuild everything, since NeedsCompilation changed from True to False, and it is a dependency of GetModIface.

Ideally, as long as all dependencies of GetModIface except NeedsCompilation are unchanged, we would not need to recompute the ModIface (The extra Linkable doesn't really hurt, and it could be used by subsequent evaluations). If some dependency of GetModIface other than NeedsCompilation did change, then we want to do a rebuild, but without producing a
Linkable.

Also, we only want to have this behavior when NeedsCompilation flips from True to False. If it flips from False to True, then we really do need to rebuild.

While going through the shake documentation, I came across the ChangedStore constructor of RunChanged. Its documentation reads "The stored value has changed, but in a way that should be considered identical (used rarely)." which does seem like it somewhat fits this situation. However, I'm still not sure if this is its purpose, and if using it would work in this scenario.

Is something like this possible with the shake model? Is ChangedStore the way to accomplish this?

/cc @pepeiborra

@pepeiborra
Copy link
Contributor

Compilation via GHC can be run in two modes, either we can just typecheck, producing a ModIface, or we can go all the way and do code-gen, producing a Linkable.
This process can't be resumed - we can't go from a ModIface to a Linkable, we need to redo the entire compilation pipeline in order to produce a Linkable

Do you know why this process cannot be resumed? Is it something that could be fixed at the ghc-api level, or would it require fundamental changes to the compiler?

@wz1000
Copy link
Author

wz1000 commented Feb 20, 2021

Do you know why this process cannot be resumed? Is it something that could be fixed at the ghc-api level, or would it require fundamental changes to the compiler?

We would need "Fat interface files" to be implemented: https://mail.haskell.org/pipermail/ghc-devs/2020-October/019324.html

Also, haskell/haskell-language-server#732

@pepeiborra
Copy link
Contributor

Why couldn't the api return a ModIface and a delayed IO Linkable?

@wz1000
Copy link
Author

wz1000 commented Feb 20, 2021

Why couldn't the api return a ModIface and a delayed IO Linkable?

This doesn't really solve the problem, since the delayed IO Linkable would close over the Core of the program, which we don't want to keep alive for memory consumption reasons.

@ndmitchell
Copy link
Owner

So the idea of some kind of dependency with this structure seems plausible and fairly sensible. You depend on a value that is ordered, and if the new value of your dependency is lower than the current value, you don't need to recompute. I quite like it.

As for whether ChangedStore can accomplish it, I don't think ChangedStore is particularly relevant. ChangedStore says that the bits stored have changed, but pretend they haven't - its used for when you do modtime OR digest changes, and the modtime changes. You mark the rule as having not changed at all, but update its bits on disk. Not quite what we want here.

However, the very closely related ChangedRecomputeDiff/ChangedRecomputeSame might be enough. When we compute a value, given the previous value, if it goes up you'd return Diff, if it goes down, you'd return Same. I think for ghcide, you'd enhance defineEarlyCutoff with a v -> v -> Bool which says if two things that identical, defaulting to ==, and replace the use of == at https://github.com/haskell/haskell-language-server/blob/1327b9b39582fa6835485f7d3c5b4052e0ef30f6/ghcide/src/Development/IDE/Core/Shake.hs#L932-L933. I think that would work out pretty nicely.

@ndmitchell
Copy link
Owner

In fact that equality is on the encoded bytestring, and we never include a decode from bytestring method, so you might want to do the equality check of the previous encoded bytestring against the currently value - making it ByteString -> v -> Bool or similar. I note that for the NeedsCompilation rule the digest of True/False is calculated as its Hash. I imagine in order to keep the boolean information, you probably want that to encode as 1/0 or similar.

@wz1000
Copy link
Author

wz1000 commented Feb 21, 2021

Yeah, I was planning to encode the value as 0,1,2. There are in fact 3 values it can return - Nothing, Just BCOLinkable and Just ObjectLinkable, for no compilation needed, code-gen via bytecode, and code-gen via object code respectively.
However, the same principle applies, if we already have object code, there is no need to go and recompute bytecode.
But I would keep the comparison to be between bytestrings, and introduce defineEarlyCutoffOrdered with the new semantics.

Abstractly, I guess it would be nice to just require a partial order on values, but since we don't really need that as of now, I think it would be simplest to just stick to a total order on bytestrings.

I'm assuming that if some other dependency of GetModIface changes, the value returned by use NeedsCompilation will be the "current", up to date value? I guess there is no other way, since shake doesn't actually keep track of the old value, only the hash. Whatever value is in the Values map will be the one returned.

@wz1000
Copy link
Author

wz1000 commented Feb 21, 2021

If the rule returns ChangedRecomputeSame, I'm assuming shake will still update the value of the ByteString in its store?

@ndmitchell
Copy link
Owner

But I would keep the comparison to be between bytestrings, and introduce defineEarlyCutoffOrdered with the new semantics.

That makes a lot of sense. Good plan.

I'm assuming that if some other dependency of GetModIface changes, the value returned by use NeedsCompilation will be the "current", up to date value?

Yep, if the rule reruns for other reasons, it will all work out.

If the rule returns ChangedRecomputeSame, I'm assuming shake will still update the value of the ByteString in its store?

Yes. The value might be the same, but the dependencies and profiling info are likely to be different, so it stores the new ByteString.

pepeiborra added a commit to haskell/haskell-language-server that referenced this issue Oct 23, 2021
In addition, we tune the newness check of the redefined NeedsCompilation rule so that the generated linkables are not thrown away unnecessarily, as described in:

ndmitchell/shake#794
pepeiborra added a commit to haskell/haskell-language-server that referenced this issue Oct 24, 2021
…hen Evaluating

In addition, we tune the newness check of the redefined NeedsCompilation rule so that the generated linkables are not thrown away unnecessarily, as described in:

ndmitchell/shake#794
pepeiborra added a commit to haskell/haskell-language-server that referenced this issue Oct 24, 2021
…hen Evaluating

In addition, we tune the newness check of the redefined NeedsCompilation rule so that the generated linkables are not thrown away unnecessarily, as described in:

ndmitchell/shake#794
pepeiborra added a commit to haskell/haskell-language-server that referenced this issue Oct 24, 2021
…hen Evaluating

In addition, we tune the newness check of the redefined NeedsCompilation rule so that the generated linkables are not thrown away unnecessarily, as described in:

ndmitchell/shake#794
pepeiborra added a commit to haskell/haskell-language-server that referenced this issue Oct 24, 2021
…hen Evaluating

In addition, we tune the newness check of the redefined NeedsCompilation rule so that the generated linkables are not thrown away unnecessarily, as described in:

ndmitchell/shake#794
pepeiborra added a commit to haskell/haskell-language-server that referenced this issue Oct 24, 2021
…hen Evaluating

In addition, we tune the newness check of the redefined NeedsCompilation rule so that the generated linkables are not thrown away unnecessarily, as described in:

ndmitchell/shake#794
mergify bot pushed a commit to haskell/haskell-language-server that referenced this issue Oct 25, 2021
* [hls-graph] clean up databaseDirtySet

When I ported https://github.com/ndmitchell/shake/pull/802/files to hls-graph, I changed the encoding of the dirty set. Instead, Dirty became a constructor in the Status union. But the databaseDirtySet stayed around accidentally, leading to some confusion.

* extract GetEvalComments rule

* override NeedsCompilation rule in eval plugin to generate linkables when Evaluating

In addition, we tune the newness check of the redefined NeedsCompilation rule so that the generated linkables are not thrown away unnecessarily, as described in:

ndmitchell/shake#794

* getLastBuildKeys

* Test that the linkables are being produced

* honor LSP_TEST_LOG_STDERR

* add comments and use custom newness check in ghcide too

* fix build

* fix 9.0 build
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

No branches or pull requests

3 participants