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

Add custom cache layer for session loading #1197

Merged
merged 4 commits into from
Jan 29, 2021

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Jan 11, 2021

Achieves this by adding a HashMap from NormalizedFilePath
to its respective hie.yaml location.
This works nicely with the existing implementation, but
increases the memory usage (and I am not sure whether by a considerable amount).

This change is motivated by haskell/hie-bios#264
where we set the build-dir for cabal based projects to some config
directory in order to avoid recompilation issues whenever users invoked
cabal build.
However, this uncovered an issue in the existing code-base, as HLS will
ask for the compilation options for generated modules, such as
Paths_*, by looking for the responsible hie.yaml file and using an
internal cache for the options. This used to be fine, until the
aforementioned hie-bios change, since now Paths_* module will be in
some ~/.cache/hie-bios/... directory, which has no responsible
hie.yaml which is why we see error messages such: No hie.yaml found, use implicit hie-bios.

I think this error message was only shown when an explicit hie.yaml is used.

I hope the motivation and explanation was understandable :) (and sound, and correct)

The implementation is more a POC, to show that it really fixes the problem.

You can give it a try and see the changes by checking out https://github.com/fendor/haskell-language-server/tree/cabal-dist-dir-tests and using it on exe:hie-bios. Compare it to the output HLS gives when reverting the last commit to that branch.

cc @wz1000

@jneira
Copy link
Member

jneira commented Jan 12, 2021

Do we have some test around Path_'s module that were broken with that version of hie-bios?

@wz1000
Copy link
Collaborator

wz1000 commented Jan 12, 2021

Wouldn't it be simpler to make hie-bios detect and return the correct cradle location for files in ~/.cache/hie-bios/...?

@fendor
Copy link
Collaborator Author

fendor commented Jan 12, 2021

@wz1000 The invoked function is findCradle :: FilePath -> IO (Maybe FilePath), where the first parameter is ~/.cache/.../Paths_*, so we actually don't have the required information in that function, afaict.

Also, I think this is rather a HLS bug than hie-bios' bug, since hie-bios lists all modules and options (including Paths_* module), HLS should be able to work with it. Thus, the patch here.

Another possible implementation approach might be to utilise the Shake graph in some way.

@wz1000
Copy link
Collaborator

wz1000 commented Jan 12, 2021

We can write the path to the original hie.yaml file/root in something like ~/.cache/hie-bios/<builddir>/root.path

I'm also concerned about the behavior with other kinds of preprocessed haskell files, like happy parsers, hsc files etc. I think even they will end up being written in the builddir.

@fendor
Copy link
Collaborator Author

fendor commented Jan 12, 2021

Seems kind of hacky to me, when HLS should have all the required information already (e.g. -i, -I,...) and figure that out itself.

@jneira jneira marked this pull request as draft January 14, 2021 12:07
@jneira
Copy link
Member

jneira commented Jan 14, 2021

I marked it as draft in github following the description, to no merge it accidentally

@pepeiborra
Copy link
Collaborator

I'm not too worried by the increase in memory usage, did you check the benchmarks?

@pepeiborra
Copy link
Collaborator

I think that we should push ahead with this, unless there is a better solution available. Waiting for Cabal is just mad

@fendor
Copy link
Collaborator Author

fendor commented Jan 27, 2021

I will check the benchmarks and address the feedback, thank you!

@fendor fendor changed the title Draft: Add custom cache layer for session loading Add custom cache layer for session loading Jan 28, 2021
@fendor fendor marked this pull request as ready for review January 28, 2021 14:31
Achieves this by adding a HashMap from NormalizedFilePath
to its respective hie.yaml location.
This works nicely with the existing implementation, but
increases the memory usage by an considerable amount.

This change is motivated by haskell/hie-bios#264
where we set the build-dir for cabal based projects to some config
directory in order to avoid recompilation issues whenever users invoked
`cabal build`.
However, this uncovered an issue in the existing code-base, as HLS will
ask for the compilation options for generated modules, such as
`Paths_*`, by looking for the responsible `hie.yaml` file and using an
internal cache for the options. This used to be fine, until the
aforementioned hie-bios change, since now `Paths_*` module will be in
some `~/.cache/hie-bios/...` directory, which has no responsible
`hie.yaml` which is why we see error messages such: `No hie.yaml found,
use implicit hie-bios`.
@fendor
Copy link
Collaborator Author

fendor commented Jan 28, 2021

Having a bit of trouble trying to make sense of it.

Looks like the memory is not increased that much, but it seems like it is not freeing memory

hover

getDefinition_after_edit

code_actions

code_actions_after_edit

@wz1000
Copy link
Collaborator

wz1000 commented Jan 28, 2021

Those are just artifacts. The graphs are identical for all purposes.

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready to merge

@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Jan 28, 2021
@mergify mergify bot merged commit 1ebd374 into haskell:master Jan 29, 2021
@fendor
Copy link
Collaborator Author

fendor commented Jan 29, 2021

I will make a release for hie-bios with the mentioned change, and we will see if there is any fallout :)

@wz1000
Copy link
Collaborator

wz1000 commented Jan 29, 2021

@fendor I think haskell/hie-bios#265 is also important in order to actually make ghcide startup faster.

@fendor
Copy link
Collaborator Author

fendor commented Jan 29, 2021

@wz1000 True, but it already improves the developer flow by not forcing the user to re-configure whenever they use cabal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants