Skip to content
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

Space Leak #386

Closed
ldub opened this issue Sep 9, 2020 · 13 comments
Closed

Space Leak #386

ldub opened this issue Sep 9, 2020 · 13 comments
Labels
performance Issues about memory consumption, responsiveness, etc. status: needs info Not actionable, because there's missing information type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..

Comments

@ldub
Copy link

ldub commented Sep 9, 2020

Subject of the issue

Massive space leak when hacking on persistent.

Your environment

  • Output of haskell-language-server --probe-tools or haskell-language-server-wrapper --probe-tools
    • How would I run this? I just have the VS Code plugin enabled
  • Which lsp-client do you use
    • VS Code
  • Describe your project (alternative: link to the project)
  • Contents of hie.yaml
    • none

Steps to reproduce

Start hacking on persistent with the vs code hls package

Expected behaviour

I'm not sure if it should work out of the box with no hie.yaml or anything, but I certainly didnt expect a space leak.

Actual behaviour

image

Include debug information

  • macOS Catalina
@Ailrun
Copy link
Member

Ailrun commented Sep 9, 2020

Does your case have anything to do with #352, namely, have a self referencing type?

@Ailrun Ailrun added status: needs repro type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. labels Sep 9, 2020
@ldub
Copy link
Author

ldub commented Sep 9, 2020

Seems likely, but I'm not 100% sure. I think its fine to close this until #352 is resolved and then I can re-test.

@Ailrun Ailrun added the performance Issues about memory consumption, responsiveness, etc. label Sep 9, 2020
@pepeiborra
Copy link
Collaborator

The problem here and in #352 is the lack of hie.yaml. We need to either fix the implicit cradle scenario or disallow it completely

@codygman
Copy link

codygman commented Sep 9, 2020

My vote is disable it completely now, release, then figure out how to move forward. Present users with an error that they need to create a cradle and link them to instructions.

A lot of people got burned off of using HIE because of "performance issues" and went back to just ghcid. It doesn't matter if those performance issues were the fault of hie or not though.

I think the publicity of hls is at stake here, though it's created a lot of performance goodwill.

@Ailrun
Copy link
Member

Ailrun commented Sep 9, 2020

I also think it would be better to disallow it for now, as it looks like #186 won't be completed any time soon. There are a lot of issues which will be resolved with valid hie.yaml, but abstruse errors confuse users so that they post those issues. What's worse, users may decide to just leave HLS without posting an issue.

@jneira
Copy link
Member

jneira commented Sep 9, 2020

Agree but we dont know neither how many are using the implicit cradle happily. Afaik simple projects, typical from newcomers, work with the actual implicit hie. Generate a hie.yaml is not rocket science but those projects will not work anymore and you can hit another errors trying to add it. Newcomers know barely what it is a component! We dont know how many will simple move away.

Maybe if the error message is good enough to motivate them make that additional step... but ideally we should generate it automatically with implicit hie.

@ndmitchell
Copy link
Collaborator

FWIW, I used the implicit cradle with my website generator project this weekend. The experience was very smooth. Had it been a case of "well, if you want to edit those 2 lines and have error checking you must ..." then I wouldn't have used HLS, but would have gone for Ghcid. And I'm a developer of HLS/Ghcide!

I think we should definitely not require a cradle. But if there isn't a cradle, writing out one with implicit hie seems quite reasonable.

@pepeiborra
Copy link
Collaborator

One potential problem with disallowing the implicit cradle scenario is that the ghcide test suite is full of cradle-less unit tests - these are one-module projects with no Cabal or Stack descriptor. So whatever solution we adopt needs to work for that use case or refactor the entire test suite. Or perhaps be specific to haskell-language-server, leaving ghcide on its own

@codygman
Copy link

FWIW, I used the implicit cradle with my website generator project this weekend. The experience was very smooth. Had it been a case of "well, if you want to edit those 2 lines and have error checking you must ..." then I wouldn't have used HLS, but would have gone for Ghcid. And I'm a developer of HLS/Ghcide!

I think we should definitely not require a cradle. But if there isn't a cradle, writing out one with implicit hie seems quite reasonable.

To be clear, I think the implicit cradle is valuable.

But if there are edge cases likely to occur with a feature that cause this much memory usage, I'd argue stability should be favored.

It sounds like @ndmitchell avoided this memory leak in the course of normal development though. I wonder why.

@ndmitchell
Copy link
Collaborator

It sounds like @ndmitchell avoided this memory leak in the course of normal development though. I wonder why.

Or it leaked, but so slowly I didn't notice. It's 2 Haskell files.

But if there are edge cases likely to occur with a feature that cause this much memory usage, I'd argue stability should be favored.

We should fix the feature, rather than harming newcomers experiences. It doesn't seem too hard to generate a hie.yaml in one step, and then use it in a second.

@ldub
Copy link
Author

ldub commented Sep 16, 2020

image
also getting pretty high CPU usage, locking up my system

pepeiborra pushed a commit that referenced this issue Dec 27, 2020
@Ailrun
Copy link
Member

Ailrun commented Jan 28, 2021

I believe now we are using a better implicit hie.yaml. Are you still able to reproduce the same memory leak?

@jneira jneira added the status: needs info Not actionable, because there's missing information label Jun 17, 2021
@jneira
Copy link
Member

jneira commented Jan 31, 2022

@ldub newer hls versions has lot of improvements around memory usage so i am gonna clode this optimistically, feel free to reopen if you continue experiencing it with a new hls version

@jneira jneira closed this as completed Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issues about memory consumption, responsiveness, etc. status: needs info Not actionable, because there's missing information type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..
Projects
None yet
Development

No branches or pull requests

6 participants