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

Fully asynchronous request handling #767

Merged
merged 7 commits into from
Sep 7, 2020

Conversation

pepeiborra
Copy link
Collaborator

#727 made the Shake queue able to concurrently handle more than one rule, but ghcide still handled LSP requests sequentially. This change makes the LSP handler multi-threaded, so now ghcide no longer blocks while e.g. computing an expensive code lens.

This change also adds a custom request protocol for testing, which can be generally useful. For instance, tests can now query the location of the interface files cache dir which will be useful for testing #760

Reviewers - things to watch for:

  • New space leaks
  • New race conditions

To do:

  • run benchmarks

@wz1000
Copy link
Collaborator

wz1000 commented Sep 5, 2020

Very nice, looks good to me. Now let's see how many tests break because of the ordering of messages...

exe/Main.hs Show resolved Hide resolved
@wz1000
Copy link
Collaborator

wz1000 commented Sep 5, 2020

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@pepeiborra
Copy link
Collaborator Author

pepeiborra commented Sep 5, 2020

The test suite is failing because the new CustomHandler responds to a message sent by testSessionWait in a way that lsp-test cannot parse.

The message that causes the issue:

void $ sendRequest (CustomClientMethod "non-existent-method") ()

I've raised this up in the lsp-test issue tracker lukel97/lsp-test#73

@wz1000
Copy link
Collaborator

wz1000 commented Sep 5, 2020

Maybe we can merge after marking the test as known broken.

@pepeiborra
Copy link
Collaborator Author

It's all the tests that use testSessionWait

@pepeiborra
Copy link
Collaborator Author

Early benchmarks not looking good - edit performance is much slower. There is increased memory usage but that seems to be a regression upstream

image

@wz1000
Copy link
Collaborator

wz1000 commented Sep 6, 2020

What is master, upstream, upstream1 and upstream2?

@pepeiborra
Copy link
Collaborator Author

pepeiborra commented Sep 6, 2020

What is master, upstream, upstream1 and upstream2?

I'll publish the whole benchmark artifacts once they are ready. In that screenshot:

  • Master points to v0.3.0 in my repo
  • Upstream is Haskell/ghcide:master
  • Upstream1 is upstream~1
  • etc

@pepeiborra
Copy link
Collaborator Author

After re-running the benchmarks in an isolated non-throttling CPU it looks like this branch does not introduce any time or space regression. Will upload full results shortly.

I have opened #771 to address the pre-existing space leak.

@pepeiborra
Copy link
Collaborator Author

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@pepeiborra
Copy link
Collaborator Author

The tests are now passing after upgrading to a fixed lsp-test version.

The artefacts from the ghcide_bench_linux_stack pipeline show no change in memory usage. There's also no significant change in time usage, but this not a reliable measurement since the benchmark runs in a shared environment.

Local benchmarks do show a 10% time decrease in the edit experiment.

image

@pepeiborra pepeiborra merged commit 684be68 into haskell:master Sep 7, 2020
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Cancellation of user actions

* Dispatch event handlers asynchronously

* add tests for asynchronous features

This adds a new Test plugin for custom requests
and a new blocking Command

* hlint

* Link the Testing plugin only when --testing

* Fix expectNoMoreDiagnostics

Needs also lukel97/lsp-test#74

* Upgrade lsp-test to a version that understands CustomClientMethod
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Cancellation of user actions

* Dispatch event handlers asynchronously

* add tests for asynchronous features

This adds a new Test plugin for custom requests
and a new blocking Command

* hlint

* Link the Testing plugin only when --testing

* Fix expectNoMoreDiagnostics

Needs also lukel97/lsp-test#74

* Upgrade lsp-test to a version that understands CustomClientMethod
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Cancellation of user actions

* Dispatch event handlers asynchronously

* add tests for asynchronous features

This adds a new Test plugin for custom requests
and a new blocking Command

* hlint

* Link the Testing plugin only when --testing

* Fix expectNoMoreDiagnostics

Needs also lukel97/lsp-test#74

* Upgrade lsp-test to a version that understands CustomClientMethod
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.

2 participants