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

Migrate to OsPath #3046

Open
kokobd opened this issue Jul 18, 2022 · 16 comments
Open

Migrate to OsPath #3046

kokobd opened this issue Jul 18, 2022 · 16 comments
Labels
performance Issues about memory consumption, responsiveness, etc. type: enhancement New feature or request

Comments

@kokobd
Copy link
Collaborator

kokobd commented Jul 18, 2022

We have a better alternative to FilePath now. See: https://hasufell.github.io/posts/2022-06-29-fixing-haskell-filepaths.html

What we should do:

  1. Change NormalizedFilePath to NormalizedOsPath after the issue in lsp to support OsPath Migrate to OsPath lsp#445 is solved. Profiling it to make sure the performance improves.
  2. Change other usages of FilePath to OsPath

However, this can not be done until ghc-9.6 is released (ships the new filepath library), or all ghc shipped libraries are available on Hackage.

Many GHC bundled libraries do not have new versions released to Hackage (for example ghci and hpc, their versions on hackage is very old). Thus, cabal solver fails if I add filepath ^>= 1.4.100.0 to a project that depends on ghc, which then depends on ghci. The installed ghci links against filepath-1.4.2.2 in this case, and cabal can not rebuild a ghci with filepath ^>= 1.4.100.0, as the source code is not found on hackage.

@kokobd kokobd added type: enhancement New feature or request performance Issues about memory consumption, responsiveness, etc. status: blocked Not actionable, because blocked by upstream/GHC etc. labels Jul 18, 2022
@pepeiborra
Copy link
Collaborator

Thus, cabal solver fails if I add filepath ^>= 1.4.100.0 to a project that depends on ghc, which then depends on ghci.

Oh bummer. Well, if we really want to, it should be ok to embed OsPath and combinators in a filepath-compat package.

@kokobd kokobd removed the status: blocked Not actionable, because blocked by upstream/GHC etc. label Jul 18, 2022
@hasufell
Copy link
Member

https://gitlab.haskell.org/ghc/ghc/-/issues/21887#note_443922

It may be possible to provide updated GHC releases that include new filepath. But this is still in discussion.

@kokobd
Copy link
Collaborator Author

kokobd commented Jul 29, 2022

I don't know why, but the overall memory usage decreased from 1.5G to 700M
The data is collected by launching haskell-language-server from the command line in the HLS source repo with profiling RTS options enabled.
image
image

@hasufell
Copy link
Member

@kokobd you mean between String and OsPath?

@kokobd
Copy link
Collaborator Author

kokobd commented Jul 29, 2022

@kokobd you mean between String and OsPath?

@hasufell Yes. But when loading HLS executable into VSCode, I no longer witness such a big difference. I'm still investigating the reason.

@pepeiborra
Copy link
Collaborator

Can you run cabal bench ghcide and share the contents of the ghcide/bench-results folder minus the binaries ?

@kokobd
Copy link
Collaborator Author

kokobd commented Jul 30, 2022

cabal bench ghcide is failing with master & ghc 9.2.3. Verified by creating a fresh new Gitpod workspace from master.
image

@pepeiborra
Copy link
Collaborator

pepeiborra commented Jul 30, 2022

I'll take a look, but in the meantime CI shows that it works fine on 8.10.7. Would you be able to run it there?

EDIT: nevermind, I see that 8.10.7 won't do
EDIT2: #3069 should help

@pepeiborra
Copy link
Collaborator

Ran the edit benchmark on Cabal 3.6.3.0, results seem quite promising:
edit

@kokobd
Copy link
Collaborator Author

kokobd commented Aug 1, 2022

Next steps:

  1. Upgrade lsp and lsp-types in HLS to 1.5. It has some breaking changes.
  2. Either extend filepath-compat to support older versions of GHC (currently it only supports ghc 9.2.3), or release GHC minor versions with new filepath package. Support of some old GHC might need to be dropped. See: https://gitlab.haskell.org/ghc/ghc/-/issues/21887

@kokobd
Copy link
Collaborator Author

kokobd commented Aug 26, 2022

Real OsPath will only be available since GHC 9.6. Instead, I use ShortByteString within NormalizedFilePath. The result is still very promising.

Soon, we will benefit from the performance improvement with all versions of GHC supported by HLS.

image

image

Next steps:

  1. Merge use OsPath in NormalizedFilePath lsp#446 and Improve performance of NormalizedFilePath #3067. (almost done)
  2. Eliminate the usage of partial functions introduced by this PR gradually.

@michaelpj
Copy link
Collaborator

I think this is mostly done. Was there more to do?

@hasufell
Copy link
Member

I haven't looked at it, but I wonder how you do file operations etc. I'm guessing you convert back to filepath somewhere? In that case the conversion is not complete

@michaelpj
Copy link
Collaborator

It looks like the situation is:

  • lsp is set up for OsPath
  • HLS hasn't migrated at all, and in the interests of keeping it simple we probably won't until 9.6 is the oldest version we support, so we don't need conditional logic and can just use OsPath everywhere

@hasufell
Copy link
Member

So in ten years, I guess?

@michaelpj
Copy link
Collaborator

Two major GHC versions, so yeah, a while. Or I guess if someone is keen and wants to do it with conditional logic they could do!

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. type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants