Skip to content
This repository has been archived by the owner on Jan 2, 2021. It is now read-only.

Fix performance regression introduced by filepath normalisation #303

Merged
merged 1 commit into from
Jan 3, 2020
Merged

Fix performance regression introduced by filepath normalisation #303

merged 1 commit into from
Jan 3, 2020

Conversation

cocreature
Copy link
Collaborator

We already normalise filepaths in NormalizedFilePath. haskell-lsp
changed things such that the conversion from Uri to NormalizedUri
normalises the filepath again which caused a significant slowdown in
GetFileExists.

We already have a wrapper for converting from NormalizedFilePath to
NormalizedUri so this PR changes this wrapper to inline the definition
without the additional layer of normalisation.

fixes #298

The corresponding haskell-lsp PR that caused the regression was haskell/lsp#205

Before
Screenshot from 2020-01-03 15-35-32
After
Screenshot from 2020-01-03 15-35-56

@cocreature
Fix performance regression introduced by filepath normalisation
849da92 
We already normalise filepaths in NormalizedFilePath. haskell-lsp
changed things such that the conversion from Uri to NormalizedUri
normalises the filepath again which caused a significant slowdown in
GetFileExists.

We already have a wrapper for converting from NormalizedFilePath to
NormalizedUri so this PR changes this wrapper to inline the definition
without the additional layer of normalisation.

fixes #298
@cocreature cocreature requested review from a user and aherrmann-da January 3, 2020 14:38
aherrmann-da
aherrmann-da approved these changes Jan 3, 2020
View reviewed changes
Copy link
Contributor

@aherrmann-da aherrmann-da left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this.

Unfortunate, that you had to replicate part of the normalization from haskell-lsp.

@cocreature cocreature merged commit 7e80629 into master Jan 3, 2020
@cocreature cocreature deleted the fix-perf-regression branch January 3, 2020 15:01
@jneira
Copy link
Member

jneira commented Jan 3, 2020

Sorry for the inconveniences. I added the extra normalization step to ensure the file uris were correct even if they come from the aeson instance. I assumed naively that the performance would not be so worse.
Check if uris are correct had suffer a similar penalty, i think.
To avoid the duplication we could expose another function in haskell-lsp without the extra step, if you agree.

@cocreature
Copy link
Collaborator Author

No worries, these things happen and once #101 is addressed this shouldn’t be a bottleneck anymore. I wonder if it might make sense to move NormalizedFilePath to haskell-lsp. Then we could expose NormalizedFilePath -> NormalizedUri which can safely skip normalization.

pepeiborra pushed a commit to pepeiborra/ghcide that referenced this pull request Jan 3, 2020
@cocreature @pepeiborra
Fix performance regression introduced by filepath normalisation (hask…
8907d8a 
…ell#303)

We already normalise filepaths in NormalizedFilePath. haskell-lsp
changed things such that the conversion from Uri to NormalizedUri
normalises the filepath again which caused a significant slowdown in
GetFileExists.

We already have a wrapper for converting from NormalizedFilePath to
NormalizedUri so this PR changes this wrapper to inline the definition
without the additional layer of normalisation.

fixes #298
cocreature added a commit that referenced this pull request Jan 3, 2020
@cocreature
Fix check for empty file path
c0e145e 
I accidentally broke this on Windows in #303 by letting the two
conversirons get out of sync.
@cocreature cocreature mentioned this pull request Jan 3, 2020
Merged
cocreature added a commit that referenced this pull request Jan 4, 2020
@cocreature
Fix check for empty file path (#304)
64693ed 
I accidentally broke this on Windows in #303 by letting the two
conversirons get out of sync.
@jneira jneira mentioned this pull request Jan 6, 2020
Closed
pepeiborra pushed a commit to pepeiborra/ghcide that referenced this pull request Feb 1, 2020
@cocreature @pepeiborra
Fix performance regression introduced by filepath normalisation (hask…
2b37a02 
…ell#303)

We already normalise filepaths in NormalizedFilePath. haskell-lsp
changed things such that the conversion from Uri to NormalizedUri
normalises the filepath again which caused a significant slowdown in
GetFileExists.

We already have a wrapper for converting from NormalizedFilePath to
NormalizedUri so this PR changes this wrapper to inline the definition
without the additional layer of normalisation.

fixes #298
pepeiborra pushed a commit to pepeiborra/ghcide that referenced this pull request Feb 1, 2020
@cocreature @pepeiborra
Fix check for empty file path (haskell#304)
8bce668 
I accidentally broke this on Windows in haskell#303 by letting the two
conversirons get out of sync.
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
@cocreature
Fix performance regression introduced by filepath normalisation (hask…
45c60f6 
…ell/ghcide#303)

We already normalise filepaths in NormalizedFilePath. haskell-lsp
changed things such that the conversion from Uri to NormalizedUri
normalises the filepath again which caused a significant slowdown in
GetFileExists.

We already have a wrapper for converting from NormalizedFilePath to
NormalizedUri so this PR changes this wrapper to inline the definition
without the additional layer of normalisation.

fixes haskell/ghcide#298
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
@cocreature
Fix check for empty file path (haskell/ghcide#304)
0a3c27b 
I accidentally broke this on Windows in haskell/ghcide#303 by letting the two
conversirons get out of sync.
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
@cocreature
Fix performance regression introduced by filepath normalisation (hask…
22b54fe 
…ell/ghcide#303)

We already normalise filepaths in NormalizedFilePath. haskell-lsp
changed things such that the conversion from Uri to NormalizedUri
normalises the filepath again which caused a significant slowdown in
GetFileExists.

We already have a wrapper for converting from NormalizedFilePath to
NormalizedUri so this PR changes this wrapper to inline the definition
without the additional layer of normalisation.

fixes haskell/ghcide#298
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
@cocreature
Fix check for empty file path (haskell/ghcide#304)
85ca980 
I accidentally broke this on Windows in haskell/ghcide#303 by letting the two
conversirons get out of sync.
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
@cocreature
Fix performance regression introduced by filepath normalisation (hask…
7a573a5 
…ell/ghcide#303)

We already normalise filepaths in NormalizedFilePath. haskell-lsp
changed things such that the conversion from Uri to NormalizedUri
normalises the filepath again which caused a significant slowdown in
GetFileExists.

We already have a wrapper for converting from NormalizedFilePath to
NormalizedUri so this PR changes this wrapper to inline the definition
without the additional layer of normalisation.

fixes haskell/ghcide#298
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
@cocreature
Fix check for empty file path (haskell/ghcide#304)
faca5f2 
I accidentally broke this on Windows in haskell/ghcide#303 by letting the two
conversirons get out of sync.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@aherrmann-da aherrmann-da aherrmann-da approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

performance degradation after upgrade to haskell-lsp 0.19
3 participants
@cocreature @jneira @aherrmann-da