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

Allow stale data back in GetSpanInfo rule #622

Merged
merged 1 commit into from
Jun 10, 2020

Conversation

pepeiborra
Copy link
Collaborator

GetSpanInfo allowed stale dependencies prior to #457 and was changed to disallow them for no good reason, which makes hovers unavailable when a dependency doesn't typecheck

While I was there, I also dropped early cutoff from GetHiFile as it seems unnecessary.

No impact in performance:

Before

name                       | samples | startup | setup | experiment | maxResidency
-------------------------- | ------- | ------- | ----- | ---------- | ------------
hover                      | 10      | 7.50s   | 0.00s | 9.71s      | 168MB       
hover after edit           | 10      | 7.25s   | 0.00s | 13.61s     | 151MB       
getDefinition              | 10      | 7.24s   | 0.00s | 5.92s      | 145MB       
documentSymbols            | 100     | 7.25s   | 0.00s | 1.43s      | 28MB        
documentSymbols after edit | 100     | 7.25s   | 0.00s | 2.77s      | 29MB        
completions after edit     | 10      | 7.26s   | 0.00s | 9.79s      | 153MB       
code actions               | 10      | 7.28s   | 5.06s | 0.39s      | 47MB        
code actions after edit    | 10      | 7.38s   | 0.00s | 9.11s      | 173MB       

After

name                       | samples | startup | setup | experiment | maxResidency
-------------------------- | ------- | ------- | ----- | ---------- | ------------
hover                      | 10      | 7.51s   | 0.00s | 9.85s      | 168MB       
hover after edit           | 10      | 7.28s   | 0.00s | 13.58s     | 152MB       
getDefinition              | 10      | 7.32s   | 0.00s | 5.99s      | 145MB       
documentSymbols            | 100     | 7.33s   | 0.00s | 1.42s      | 28MB        
documentSymbols after edit | 100     | 7.40s   | 0.00s | 2.79s      | 30MB        
completions after edit     | 10      | 7.31s   | 0.00s | 9.85s      | 152MB       
code actions               | 10      | 7.30s   | 5.12s | 0.39s      | 47MB        
code actions after edit    | 10      | 7.20s   | 0.00s | 9.19s      | 173MB   

@ndmitchell
Copy link
Collaborator

I think the problem is that useWithStale isn't really using stale results. It's using up to the minute results, but the last working up to the minute results, hence I always get tripped up by it. Definitely the right change to make.

But why is removing the early cutoff from the HiFile the right thing to do? Before if the interface hash was the same twice be cut off computation. That seems valuable in some cases, and I imagine a case your benchmark doesn't hit? Generally, having a cutoff hash or similar is very cheap, provided computing the hash is itself cheap.

@pepeiborra
Copy link
Collaborator Author

But why is removing the early cutoff from the HiFile the right thing to do? Before if the interface hash was the same twice be cut off computation. That seems valuable in some cases, and I imagine a case your benchmark doesn't hit? Generally, having a cutoff hash or similar is very cheap, provided computing the hash is itself cheap.

Just a desire to minimize things. We already get cut off if the GetModTime is the same for the interface and all its dependencies, right? But I guess that doesn't cover comments and whitespace edits, and given that the hash is likely cheap in this case, I'm happy to drop it. Thanks!

@pepeiborra pepeiborra force-pushed the hifile-early-cutoff branch from 25b1211 to 872606d Compare June 9, 2020 20:52
This rule used withstale dependencies prior to haskell#457 and was changed to plain use
for no good reason, which makes hovers unavailable when a dependency doesn't
typecheck
@pepeiborra pepeiborra force-pushed the hifile-early-cutoff branch from 872606d to cd434c5 Compare June 9, 2020 21:25
Copy link
Collaborator

@cocreature cocreature left a comment

Choose a reason for hiding this comment

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

Thanks!

@cocreature cocreature merged commit e380aad into haskell:master Jun 10, 2020
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
This rule used withstale dependencies prior to haskell/ghcide#457 and was changed to plain use
for no good reason, which makes hovers unavailable when a dependency doesn't
typecheck
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
This rule used withstale dependencies prior to haskell/ghcide#457 and was changed to plain use
for no good reason, which makes hovers unavailable when a dependency doesn't
typecheck
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
This rule used withstale dependencies prior to haskell/ghcide#457 and was changed to plain use
for no good reason, which makes hovers unavailable when a dependency doesn't
typecheck
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants