This repository has been archived by the owner on Jan 2, 2021. It is now read-only.
Improve performance by caching conversion to NormalizedUri #384
Conversation
This file contains 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
This conversion is quite expensive to repeat multiple times. Therefore we cache it when creating a NormalizedFilePath so it's only computed once. Making this change causes a benchmark which calls hover 1000 times to go down from 259s to 44s.
NormalizedUri is the primary key type for the debouncer which will have a cached hash in the next haskell-lsp release.
|
Branch with the change which tracks the haskell-lsp is here - https://github.com/mpickering/ghcide/tree/perf-haskell-lsp |
pepeiborra
reviewed
Jan 28, 2020
| deriving (Show, Generic, Eq, Ord) | ||
|
|
||
| instance NFData NormalizedUriWrapper where | ||
| rnf = rwhnf |
There was a problem hiding this comment.
That NFData instance isn't correct either. Can we just lose it?
@ndmitchell does Shake care about these NFData instances?
|
Looks good to me! |
cocreature
approved these changes
Jan 28, 2020
| -- | ||
| -- This is one of the most performance critical parts of ghcide, do not | ||
| -- modify it without profiling. | ||
| data NormalizedFilePath = NormalizedFilePath NormalizedUriWrapper Int !FilePath |
There was a problem hiding this comment.
Similar question as for the haskell-lsp PR. Should we make the first two fields or at least the hash strict and unpack it?
There was a problem hiding this comment.
I can see the argument for making the hash strict but not the NormalizedUriWrapper as that is quite expensive to compute if you never use it.
There was a problem hiding this comment.
I made the hash strict.
pepeiborra
pushed a commit
to pepeiborra/ghcide
that referenced
this pull request
Feb 1, 2020
* Cache conversion to NormalizedUri from NormalizedFilePath This conversion is quite expensive to repeat multiple times. Therefore we cache it when creating a NormalizedFilePath so it's only computed once. Making this change causes a benchmark which calls hover 1000 times to go down from 259s to 44s. * Use a HashMap rather than a Map for debouncer NormalizedUri is the primary key type for the debouncer which will have a cached hash in the next haskell-lsp release. * NormalizedFilePath: Make the hash strict so it can be unpacked
pepeiborra
pushed a commit
to pepeiborra/ide
that referenced
this pull request
Dec 29, 2020
…hcide#384) * Cache conversion to NormalizedUri from NormalizedFilePath This conversion is quite expensive to repeat multiple times. Therefore we cache it when creating a NormalizedFilePath so it's only computed once. Making this change causes a benchmark which calls hover 1000 times to go down from 259s to 44s. * Use a HashMap rather than a Map for debouncer NormalizedUri is the primary key type for the debouncer which will have a cached hash in the next haskell-lsp release. * NormalizedFilePath: Make the hash strict so it can be unpacked
pepeiborra
pushed a commit
to pepeiborra/ide
that referenced
this pull request
Dec 29, 2020
…hcide#384) * Cache conversion to NormalizedUri from NormalizedFilePath This conversion is quite expensive to repeat multiple times. Therefore we cache it when creating a NormalizedFilePath so it's only computed once. Making this change causes a benchmark which calls hover 1000 times to go down from 259s to 44s. * Use a HashMap rather than a Map for debouncer NormalizedUri is the primary key type for the debouncer which will have a cached hash in the next haskell-lsp release. * NormalizedFilePath: Make the hash strict so it can be unpacked
pepeiborra
pushed a commit
to pepeiborra/ide
that referenced
this pull request
Dec 29, 2020
…hcide#384) * Cache conversion to NormalizedUri from NormalizedFilePath This conversion is quite expensive to repeat multiple times. Therefore we cache it when creating a NormalizedFilePath so it's only computed once. Making this change causes a benchmark which calls hover 1000 times to go down from 259s to 44s. * Use a HashMap rather than a Map for debouncer NormalizedUri is the primary key type for the debouncer which will have a cached hash in the next haskell-lsp release. * NormalizedFilePath: Make the hash strict so it can be unpacked
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Two changes in this patch make performance quite a lot better for all actions.
NormalizedUrifromNormalizedFilePathNormalizedUri.The cached
Hashableinstance is in this PR: haskell/lsp#214My benchmark which ran hover 1000 times went down from 259s to 37s including initialisation time. Hovering on large projects is not noticeably faster (1s vs < 0.1s).
Reviewer: @pepeiborra
Note: I removed the Eq/Ord instance from Pepe's original patch because they relied on the hash for comparing equality. There are some hotspots on the latest profile from