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

Performance improvements for GetSpanInfo #681

Merged
merged 8 commits into from
Jul 13, 2020

Conversation

pepeiborra
Copy link
Collaborator

Avoiding duplicate work and batching ghc-api lookups for both getDocs and lookupName are the main sources of improvement.

As a bonus I also removed usage of the GHCi code paths. Since there is no way for me to test if this fixes the ghc-lib issues, I'm happy to put the CPP pragmas back if that's preferred.

edit

Full benchmarks in: https://github.com/pepeiborra/ghcide/tree/perf-spaninfo-benchmark

@pepeiborra pepeiborra force-pushed the perf-spaninfo branch 8 times, most recently from e4d0f03 to 267fec4 Compare July 4, 2020 14:29
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.

Nice thank you!
We’re lagging behind a bit in DAML so it will be a while until I can test it. If it fails, we can always add the CPP back.

@cocreature
Copy link
Collaborator

Looks like this needs rebasing.

getSpanInfo was naively calling getDocumentations multiple times on the same
name. Fixed by deduplicating these calls.

getDocumentations is implemented on top of InteractiveEval.getDocs, which does
a lot of Ghc setup internally and is very inefficient. Fixed by introducing a
batch version of getDocs and batching all the calls in getSpanInfo

name          | success | samples | startup | setup | experiment | maxResidency
------------- | ------- | ------- | ------- | ----- | ---------- | ------------
edit (before) | True    | 10      | 6.94s   | 0.00s | 6.57s      | 177MB
edit (after)  | True    | 10      | 6.44s   | 0.00s | 4.38s      | 174MB
Played the deduplication trick on lookupName, which is slow for the same reasons
as getDocs. Batching made a smaller difference in my measurements, so did
not implement it
We don't use the interactive module, so there's no reason to go through the GHCi
code paths. Moreover, they apparently cause problems with ghc-lib.
@pepeiborra
Copy link
Collaborator Author

Rebased.

I'd rather put back the CPP exclusions, merge this now, and then send a separate PR to remove them which can be tested in due time. Might be able to do this later today

@cocreature cocreature merged commit cbafcf2 into haskell:master Jul 13, 2020
@pepeiborra
Copy link
Collaborator Author

Thanks! I'll submit a PR to restore the CPP sections later today or tomorrow.

pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Performance improvements

getSpanInfo was naively calling getDocumentations multiple times on the same
name. Fixed by deduplicating these calls.

getDocumentations is implemented on top of InteractiveEval.getDocs, which does
a lot of Ghc setup internally and is very inefficient. Fixed by introducing a
batch version of getDocs and batching all the calls in getSpanInfo

name          | success | samples | startup | setup | experiment | maxResidency
------------- | ------- | ------- | ------- | ----- | ---------- | ------------
edit (before) | True    | 10      | 6.94s   | 0.00s | 6.57s      | 177MB
edit (after)  | True    | 10      | 6.44s   | 0.00s | 4.38s      | 174MB

* More performance improvements

Played the deduplication trick on lookupName, which is slow for the same reasons
as getDocs. Batching made a smaller difference in my measurements, so did
not implement it

* Fix redundant constraints

* Skip the GHCi code paths for documentation

We don't use the interactive module, so there's no reason to go through the GHCi
code paths. Moreover, they apparently cause problems with ghc-lib.

* Skip the GHCi paths for lookupName

* Correctly load the module interface

* Compatibility with GHC 8.4 and 8.6

* Fix ghc-lib build
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Performance improvements

getSpanInfo was naively calling getDocumentations multiple times on the same
name. Fixed by deduplicating these calls.

getDocumentations is implemented on top of InteractiveEval.getDocs, which does
a lot of Ghc setup internally and is very inefficient. Fixed by introducing a
batch version of getDocs and batching all the calls in getSpanInfo

name          | success | samples | startup | setup | experiment | maxResidency
------------- | ------- | ------- | ------- | ----- | ---------- | ------------
edit (before) | True    | 10      | 6.94s   | 0.00s | 6.57s      | 177MB
edit (after)  | True    | 10      | 6.44s   | 0.00s | 4.38s      | 174MB

* More performance improvements

Played the deduplication trick on lookupName, which is slow for the same reasons
as getDocs. Batching made a smaller difference in my measurements, so did
not implement it

* Fix redundant constraints

* Skip the GHCi code paths for documentation

We don't use the interactive module, so there's no reason to go through the GHCi
code paths. Moreover, they apparently cause problems with ghc-lib.

* Skip the GHCi paths for lookupName

* Correctly load the module interface

* Compatibility with GHC 8.4 and 8.6

* Fix ghc-lib build
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Performance improvements

getSpanInfo was naively calling getDocumentations multiple times on the same
name. Fixed by deduplicating these calls.

getDocumentations is implemented on top of InteractiveEval.getDocs, which does
a lot of Ghc setup internally and is very inefficient. Fixed by introducing a
batch version of getDocs and batching all the calls in getSpanInfo

name          | success | samples | startup | setup | experiment | maxResidency
------------- | ------- | ------- | ------- | ----- | ---------- | ------------
edit (before) | True    | 10      | 6.94s   | 0.00s | 6.57s      | 177MB
edit (after)  | True    | 10      | 6.44s   | 0.00s | 4.38s      | 174MB

* More performance improvements

Played the deduplication trick on lookupName, which is slow for the same reasons
as getDocs. Batching made a smaller difference in my measurements, so did
not implement it

* Fix redundant constraints

* Skip the GHCi code paths for documentation

We don't use the interactive module, so there's no reason to go through the GHCi
code paths. Moreover, they apparently cause problems with ghc-lib.

* Skip the GHCi paths for lookupName

* Correctly load the module interface

* Compatibility with GHC 8.4 and 8.6

* Fix ghc-lib build
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

2 participants