This repository was archived by the owner on Jan 2, 2021. It is now read-only.
First attempt at multi component support#361
Closed
cocreature wants to merge 5 commits intomasterfrom
Closed
Conversation
Contributor
Is the problem memory usage or performance? |
Collaborator
|
Multiple |
Collaborator
Author
|
It uses more memory, is slower (if nothing else than just because the GC will be slower with more memory being used) and a bit fragile as @pepeiborra pointed out although I think the runtime linker is mostly an issue with TH and I would expect that merging it into one HscEnv doesn’t help with any way since at that point it’s definitely not isolated between components. And if components are compatible the fact that it’s global state shouldn’t be an issue I believe. |
This implements multi-component using separate HscEnv’s which is not great but semes to work somehow. The biggest issue atm is files it `autogen` directories which need to be explicitly specified in hie.yaml which is rather annoying. We need to find some solution for that and stop spinning up one HscEnv per component before merging this.
cocreature
force-pushed
the
multi-component
branch
from
fdd90f1 to
e185393
Compare
Contributor
Sounds like that's better than no multi-cradle support :) |
Collaborator
Author
|
Not sure when I’ll have time to continue working on this so if someone wants to pick it up, just leave a comment here. Currently, it’s a bit in an awkward state where it tries to use a single |
Contributor
|
@cocreature would it makes sense to merge multi-component support in current state? I don't mind using extra memory and adding the autogen directories, opposed to current alternative which is that it doesn't work at all :) |
Collaborator
Author
|
In the current state this is probably going to cause more problems than it solves. I’d rather implement this properly than merge some half-baked thing. |
Collaborator
|
Do we have anyone with knowledge and interest to be able to do this task justice? |
Contributor
|
I am going to pick this patch up. |
Contributor
|
One thing to note is that cabal uses inplace dependencies for components but also other packages specified in the EDIT: It does appear to work after trying again, it was a .ghci file which was messing things up I think. |
Contributor
|
@cocreature How well do you think this will work?
This scheme avoids the need to know about all the components up front. |
Collaborator
Author
|
@mpickering that sounds very sensible and like it should give us a nice UX. Not quite sure how easy the unloading is. You probably have to recompile all the modules from the component. |
Contributor
|
It might be that we have to create a new |
Contributor
|
If there is anyone brave, you can try my branch - https://github.com/mpickering/ghcide/tree/wip/multi Jumping between components will only work once you have opened one file from each component in your editor. |
|
@mpickering Many thanks for working on this! I've just tried out your branch on our monorepo and, after creating a suitable multi-cradle |
Contributor
|
It seems to work, but I get interesting output from running it in my terminal. For example, I see: Note that for - path: "quoting/tests"
config: { cradle: { cabal: { component: "quoting:test:tests" } } }
- path: "quoting/exe-compare"
config: { cradle: { cabal: { component: "exe:quoting-compare" } } }
- path: "quoting/exe-picofactory-cost-model"
config: { cradle: { cabal: { component: "exe:picofactory-cost-model" } } }Is this to be expected? |
Contributor
|
It doesn't even need a Cabal file to have multiple components. Here's the output of https://gist.github.com/ocharles/db84fe5d56e8e569a9cfbfad6bb67fec Notice that it called Cabal twice |
Contributor
|
@ocharles |
Contributor
|
Haha, wow. You're entirely correct - this was the problem. So not only is |
Contributor
|
Ok, the next problem I'm having seems to be some instances have vanished: But this is rather strange, given: https://hackage.haskell.org/package/postgresql-simple-0.6.2/docs/Database-PostgreSQL-Simple-Errors.html#t:ConstraintViolation - there is Here's how the component was configured: |
Contributor
|
I have seen this happen but I thought I eliminated it. The issue is that when a new component is created it creates a new HscEnv which means everything needs to be typechecked again. It seems that one of the dependencies is not being typechecked again for some reason. |
Contributor
|
I think there might also be a problem with |
Contributor
|
@mpickering I don't believe so, but other components are able to be loaded by hie, when using cabal new style builds. |
|
Is 45 seconds unsurprising for running this ghcide branch on a 2 project, 2 lines of code, repo? What I have is: Package x: Package y: |
Contributor
|
@TomMD Loading two components from |
Contributor
|
Updated branch - https://github.com/mpickering/ghcide/tree/wip/multi-rebase Can you try it again please @ocharles? If you are also using Template Haskell then you probably should also try |
Collaborator
|
I got an issue with basic TH stuff and stack. Works fine with a cabal cradle. cradle:
stack:Lib.hs: module Lib where
main :: IO ()
main = putStrLn "Hello, World"Everything seems to work fine. module Lib where
import Paths_package
main :: IO ()
main = putStrLn "Hello, World"then every hover request forces a recompile of the package, slowing hover down a lot. Logs:Output of `ghcide`Everything else is pretty stable so far! |
|
@mpickering If it were just a sub-minute time I wouldn't be too concerned. What I'm seeing isn't just a 20-40 second delay, but perhaps an Let's take a smaller example two to four packages with linear dependencies. I have hosted a tiny example repository and script for this purpose. There are four packages w, z, y x with the dependency tree of Observation: ghcide is not deterministic even if I remove dist-newstyle and run fresh. In a clean run there are varying numbers of configurations each time. Running the script ( Phrased another way we have the function: Example output of the script with today's rebased branch is: Perhaps this isn't surprising to you (I'm curious here if this is expected behavior), but it is a blocker for ghcide being useful on the majority of projects for which one would really want an IDE. |
Contributor
|
@TomMD The branch has barely been working for 6 hours, further refinements are possible but for much better performance, the build tools will have to be changed which is a much longer project. |
Contributor
|
The link in your comment appears to be dead, and also it seems that you are running ghcide on the command line rather than in the editor. Things may be much more sensible if you load the project into an editor as it will only load components if you open a file from them. |
|
@mpickering Link fixed by edit (https://github.com/TomMD/ghcide-examples) but I don't think that matters at this point. I didn't consider that ghcide on the command line isn't a suitable evaluation. I do see this works in a suitable timeframe in my current workflow. Thank you! |
Contributor
The issue appears to be two things.
|
Contributor
There are quite a few reasons now how the command line has diverged from the usage as a language server. If you want to test using the command line then the best way in my experience is to specify just a few files you want to test loading, for example, one from each component. Please let me know if you run into any issues during normal usage. |
|
I'm trying to hover a symbol in production code (large-ish codebase) without success. I understand what the problem is but the behavior is errant and probably interesting to you (?). My hie.yaml mentions the cabal Now the odd part. If I hover again then allegedly the configuration of many of my local dependencies have changed (packages in this project & repo as well as packages acquired via git as specified in the cabal.project). The hover again configures and builds the package and again loads up a new HscEnv with a growing set of As an aside, adding a |
Contributor
|
@TomMD Can you try a |
|
@mpickering If I entirely remove the hie.yaml then things work well editing the library (I can hover) but for the executable there is no hover information and a log of: If there's something more to specify cabal but not the target that would help then I'm game (I don't see that in the hie-bios docs) but there doesn't seem to be any issue in determining to use cabal, just in getting the right target. |
|
I'm seeing |
Contributor
That would be because the |
|
Yes indeed -- I run up the cli just to make sure stuff is working -- I have to generate the hie.yaml as our repo is so large -- I'm now working with it behind lsp in emacs. I'd assumed that |
|
Does the multi version take a performance penalty (vs the current version) when being used on a single component project? I'm seeing huge resident sets (8Gb on the ghcide project itself) and very slow response times. |
Contributor
It is possible, what are you observing? |
|
Resident set size in excess of 8Gb and response time for type hovers > 3 secs when working with the |
Contributor
|
That doesn't sound normal, perhaps it is the same issue as @fendor reported with stack not returning all targets. Do you observe the same with cabal? |
|
I just want to report that the mpickering/wip/multi branch (1f64d52) works perfectly for a project (runeksvendsen/crypto-orderbook-db@1c2d760) that contains two However, if I remove the above |
Contributor
|
@runeksvendsen That is expected behaviour currently, thank you for the report. |
|
Thanks for working on this @mpickering ❤️ Do you think this could be merged in its current state or is there something critical stopping you from doing it? User reports seem to be quite encouraging :) |
Contributor
|
@gvolpe I need to discuss with the maintainers about the implementation and also fix a regression to do with reloading when dependencies change. There's nothing stopping you using the branch. |
|
Thanks @mpickering
Well, there is 😄 ... I installed it using |
Collaborator
FYI this branch (rebased) is also exposed via https://github.com/alanz/haskell-language-server/tree/multi-experimental |
Contributor
|
TODO:
|
Collaborator
Author
|
Closing in favor of #522 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
14 participants
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.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This implements multi-component using separate HscEnv’s which is not
great but semes to work somehow.
The biggest issue atm is files it
autogendirectories which need tobe explicitly specified in hie.yaml which is rather annoying.
We need to find some solution for that and stop spinning up one HscEnv
per component before merging this.