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

Fix spaninfo Haddocks for local modules #678

Merged
merged 2 commits into from Jul 1, 2020

Conversation

pepeiborra
Copy link
Collaborator

The regression was introduced in #630.

I added GhcSessionDeps with the idea of reusing the typecheck GHC session
for computing the SpanInfo, instead of rebuilding it from scratch.

But I forgot to actually reuse it, or maybe the change got lost during the merge.

The regression was introduced in haskell#630.

I added `GhcSessionDeps` with the idea of reusing the typecheck GHC session
for computing the SpanInfo, instead of rebuilding it from scratch.

But I forgot to actually reuse it, or maybe the change got lost during the merge.
@ndmitchell
Copy link
Collaborator

Is there any test that will stop us breaking it again in the future?

@pepeiborra
Copy link
Collaborator Author

pepeiborra commented Jun 30, 2020

Benchmarks here - https://github.com/pepeiborra/ghcide/tree/fix-spaninfo-benchmarks/bench-hist

Sadly the edit experiment regresses quite a bit - it takes twice as long.
Keep in mind that edits now ask for GetSpanInfo too.
This happens because now the GHC session does contain matches for the doc lookups.
I suppose asking GHC for benchmarks Haddocks is really slow!
Or more likely, our code to convert Haddock markups to markdown.
Will need more investigation (but perhaps in another PR)

https://github.com/pepeiborra/ghcide/blob/fix-spaninfo-benchmarks/bench-hist/results.csv

@pepeiborra
Copy link
Collaborator Author

Is there any test that will stop us breaking it again in the future?

Good question. There are tests for this, but they fall short of checking for comments!
Should be an easy fix, to be added in this PR

@pepeiborra
Copy link
Collaborator Author

I have extended the tests that were intended to verify for this condition, and checked that they would fail without this patch

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!

@pepeiborra
Copy link
Collaborator Author

The profiler points towards GHC, although there are no details below ioMsgMaybe
profile of edit experiment (1000 samples).zip

@pepeiborra pepeiborra force-pushed the fix-spaninfo branch 2 times, most recently from 5c5634a to 6074a45 Compare June 30, 2020 17:57
@pepeiborra
Copy link
Collaborator Author

pepeiborra commented Jun 30, 2020

I have looked at the performance regression after switching to GhcSessionDeps which has the effect of correctly loading all the dependencies in the Ghc session, and it's attributed to:

  1. The getDocumentationTryGhc call, which wasn't doing much previously for home package names.
  2. The lookupName call, idem, and
  3. The desugarExpr call, which becomes much more expensive too for some reason.

I suspect that all three cases are simply providing more answers and doing more work now that the Ghc session is correctly loaded with the dependencies.

There are some low hanging fruits in cases 2 and 3 which I'll address in a separate PR.

@cocreature
Copy link
Collaborator

Thanks for the analysis. Happy to leave this for a separate PR.

@cocreature cocreature merged commit 035019d into haskell:master Jul 1, 2020
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Fix regression in SpanInfo haddocks for local modules

The regression was introduced in haskell/ghcide#630.

I added `GhcSessionDeps` with the idea of reusing the typecheck GHC session
for computing the SpanInfo, instead of rebuilding it from scratch.

But I forgot to actually reuse it, or maybe the change got lost during the merge.

* Add test
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Fix regression in SpanInfo haddocks for local modules

The regression was introduced in haskell/ghcide#630.

I added `GhcSessionDeps` with the idea of reusing the typecheck GHC session
for computing the SpanInfo, instead of rebuilding it from scratch.

But I forgot to actually reuse it, or maybe the change got lost during the merge.

* Add test
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Fix regression in SpanInfo haddocks for local modules

The regression was introduced in haskell/ghcide#630.

I added `GhcSessionDeps` with the idea of reusing the typecheck GHC session
for computing the SpanInfo, instead of rebuilding it from scratch.

But I forgot to actually reuse it, or maybe the change got lost during the merge.

* Add test
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.

None yet

3 participants