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

Improve performance of NormalizedFilePath #3067

Merged
merged 17 commits into from
Sep 8, 2022
Merged

Improve performance of NormalizedFilePath #3067

merged 17 commits into from
Sep 8, 2022

Conversation

kokobd
Copy link
Collaborator

@kokobd kokobd commented Jul 30, 2022

Upgrade lsp to benefit from the recent optimization in NormalizedFilePath.
Upstream PRs:

This PR doesn't take care of Nix, as Nix is already failing due to hiedb and hie-bios. Maybe someone familiar with Nix could solve them all in one PR.

@kokobd kokobd force-pushed the kokobd/os-path branch 2 times, most recently from 156f1f0 to 17c81ec Compare August 26, 2022 00:35
@kokobd kokobd mentioned this pull request Aug 26, 2022
@kokobd kokobd changed the title use OsPath in NormalizedFilePath Improve performance of NormalizedFilePath Sep 2, 2022
@kokobd kokobd force-pushed the kokobd/os-path branch 2 times, most recently from e34e0db to 3fa1858 Compare September 4, 2022 13:50
@kokobd kokobd marked this pull request as ready for review September 6, 2022 11:01
@@ -76,7 +76,7 @@ versions:


# - 1.8.0.0
# - upstream: origin/master
- upstream: origin/master
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should compare with origin/master by default, so that we can see if a PR makes the performance better or worse.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I disabled it temporarily when merging the benchmark suite PR because upstream/master lacked some fixes that were required to run it. Thanks for enabling it!

# - ghcide-code-actions-bindings
# - ghcide-code-actions-fill-holes
# - ghcide-code-actions-imports-exports
# - ghcide-code-actions-type-signatures
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Enabling all of these is very slow to run, and the generated chart contains too many lines and is too hard to read. A good default is only to enable the "All" group.

We can see the difference.
Before:
image
After:
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems reasonable

@kokobd
Copy link
Collaborator Author

kokobd commented Sep 8, 2022

Current result:

image
image

Note that the result is not ALWAYS better. Locally, most benchmarks perform better, as you see above. But on the CI, some benchmark results are not as good as the local ones. See:

image

@kokobd kokobd added the merge me Label to trigger pull request merge label Sep 8, 2022
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.

Awesome stuff!

@mergify mergify bot merged commit 3fb4082 into master Sep 8, 2022
sloorush pushed a commit to sloorush/haskell-language-server that referenced this pull request Sep 12, 2022
* upgrade lsp

* modify default benchmark config

* upgrade lsp

* use lsp master

* temp: compare benchmark with previous commit

* use text in NormalizedFilePath

* upgrade to lsp master

* fix stack config

* remove obsolete dir form ghcide.cabal

* run pre-commit without args

* Revert "run pre-commit without args"

This reverts commit 1c2a11d.

* remove unnecessary tests
@kokobd kokobd deleted the kokobd/os-path branch September 13, 2022 06:09
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.

2 participants