Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HLS benchmarks #3117

Merged
merged 15 commits into from
Aug 25, 2022
Merged

HLS benchmarks #3117

merged 15 commits into from
Aug 25, 2022

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented Aug 20, 2022

Port the ghcide benchmark suite to HLS, adding support to benchmark plugin "configurations" independently.

This includes the following changes to the ghcide benchmark suite:

  • Support for "configurations" which are defined as sets of plugin ids.
    The benchmark will be run with only these plugins enabled and all others disabled
  • Support for configurable concurrency.

@@ -366,13 +360,12 @@ common brittany
build-depends: hls-brittany-plugin ^>= 1.0
cpp-options: -Dhls_brittany

executable haskell-language-server
library plugins
Copy link
Collaborator

@fendor fendor Aug 20, 2022

Choose a reason for hiding this comment

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

this might make it impossible to use stack for loading HLS into HLS. I.e. HLS needs to be configured to use cabal for development with this change due to commercialhaskell/stack#4564

Not a biggie, just fyi

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Finally time to drop the stack descriptors and the Circle CI

@pepeiborra pepeiborra force-pushed the hls-benchmarks branch 4 times, most recently from 3ca8f60 to bc907e9 Compare August 21, 2022 07:08
@pepeiborra pepeiborra marked this pull request as ready for review August 21, 2022 08:08
@michaelpj
Copy link
Collaborator

Great!

Is there a reason not to delete the ghcide benchmarking code? Would make the diff simpler too, since I assume the benchmark runner fails are copies of the existing ghcide ones, and then it would just be a move.

@pepeiborra
Copy link
Collaborator Author

Great!

Is there a reason not to delete the ghcide benchmarking code? Would make the diff simpler too, since I assume the benchmark runner fails are copies of the existing ghcide ones, and then it would just be a move.

I am actively deleting it. But maybe I missed some code.

@pepeiborra
Copy link
Collaborator Author

This is ready for review. CI is setup to run benchmarks for the plugin configurations defined in bench/config.yaml:

  • None: no plugins
  • All
  • Ghcide: just the ghcide plugins
  • Core: Ghcide plugins + call hierarchy + code range + eval + pragmas. This is an arbitrary selection
  • : configurations for each individual plugin, excluding formatting plugins since none of the benchmark experiments exercise STextDocumentFormatting.

configurations:
- None: []
- Core:
- callHierarchy
- codeRange
- eval
- ghcide-code-actions-bindings
- ghcide-code-actions-fill-holes
- ghcide-code-actions-imports-exports
- ghcide-code-actions-type-signatures
- ghcide-completions
- ghcide-type-lenses
- pragmas
- Ghcide:
- ghcide-code-actions-bindings
- ghcide-code-actions-fill-holes
- ghcide-code-actions-imports-exports
- ghcide-code-actions-type-signatures
- ghcide-completions
- ghcide-type-lenses
- All:
- alternateNumberFormat
- callHierarchy
- changeTypeSignature
- class
- codeRange
- eval
- explicitFixity
- floskell
- fourmolu
- gadt
- ghcide-code-actions-bindings
- ghcide-code-actions-fill-holes
- ghcide-code-actions-imports-exports
- ghcide-code-actions-type-signatures
- ghcide-completions
- ghcide-type-lenses
- hlint
- importLens
- moduleName
- ormolu
- pragmas
- qualifyImportedNames
- refineImports
- rename
- stylish-haskell
- alternateNumberFormat
- brittany
- callHierarchy
- changeTypeSignature
- class
- codeRange
- eval
- explicitFixity
- floskell
- fourmolu
- gadt
- ghcide-code-actions-bindings
- ghcide-code-actions-fill-holes
- ghcide-code-actions-imports-exports
- ghcide-code-actions-type-signatures
- ghcide-completions
# - ghcide-core # implicitly included in all configurations
# - ghcide-hover-and-symbols # implicitly included in all configurations
- ghcide-type-lenses
- haddockComments
- hlint
- importLens
- moduleName
- ormolu
- pragmas
- qualifyImportedNames
- refineImports
- rename
- retrie
- splice
- stan
- stylish-haskell
- tactics

The results show that hls-hlint-plugin is, unsurprisingly, the most expensive plugin, followed closely by Tactics.

There is certainly margin for improvement:

  1. Generated configurations: currently the list of configurations needs to be manually updated when new plugins are added (or removed).
  2. Conditional configurations: the 9.2 run is benchmarking a lot of plugins that are not included in the 9.2 build of HLS. These configurations are equivalent to the None configuration.
  3. Sharding: currently it takes CI ~4h to run all the configurations for the Cabal example, ideally we would split the work across multiple machines.

@michaelpj
Copy link
Collaborator

I am actively deleting it. But maybe I missed some code.

Excellent. There are still some files in ghcide/bench, though?

@pepeiborra
Copy link
Collaborator Author

pepeiborra commented Aug 21, 2022

I am actively deleting it. But maybe I missed some code.

Excellent. There are still some files in ghcide/bench, though?

The ghcide test suite references them, so a bit more work is required to move them.

To be more precise, we have:

  1. The benchmark experiment definitions, which are in a library for reuse in the ghcide test suite.
  2. The ghcide-bench executable, which could be renamed to lsp-bench since it's not that ghcide specific anymore.

Both things could be moved to a standalone package lsp-bench with executable and test suite stanzas the haskell-language-server package for clarity.

@pepeiborra
Copy link
Collaborator Author

I took a look and ghcide-bench uses code from the ghcide test suite (D.I.Test.Diagnostic) that is quite tricky to extract. The path would be:

  1. move all the ghcide support test code to hls-test-utils,
  2. move ghcide-bench to its own package.

Part 1 is a bit scary, I don't know how much work it will be.

@michaelpj
Copy link
Collaborator

The path would be:

Yeah, step 1 looks scary, especially because then the ghcide tests would depend on hls-test-utils which depends on ghcide? Not sure if that would work.

Would we need to do step 2? Wouldn't we just fold it into the HLS benchmark suite?

Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Haven't looked at the actual benchmarking code, I trust you on that, just superficial comments.

.gitignore Show resolved Hide resolved
bench/Main.hs Show resolved Hide resolved
bench/README.md Show resolved Hide resolved
bench/README.md Outdated Show resolved Hide resolved
bench/config.yaml Show resolved Hide resolved
exe/Main.hs Outdated Show resolved Hide resolved
haskell-language-server.cabal Outdated Show resolved Hide resolved
src/HlsPlugins.hs Show resolved Hide resolved

basicTests :: TestTree
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems marginally useful, are we just losing this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are losing the example plugins. There's hardly any point when we have a nice tutorial and 25? existing plugins that can serve as example

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 Might as well delete their source too then?

@michaelpj
Copy link
Collaborator

Looks like you're going ahead with a ghcide-bench package.

@pepeiborra
Copy link
Collaborator Author

Looks like you're going ahead with a ghcide-bench package.

Yeah, but I avoided the scary step 1 by duplicating some things from the ghcide test suite. Can be deleted when/if they are extracted to a helper package

The main goal here is to move the Plugins module into an internal
library so that it can be reused from the benchmark suite.

In order to
make that easier, and since they hardly serve a purpose in a repository
with 25 plugins, I delete the Example and Example2 plugin descriptors
and their dependencies.
Port the ghcide benchmark suite to HLS and benchmark plugin
"configurations" independently.

This includes the following changes to the ghcide benchmark suite and
HLS:
- Support for "configurations" which are defined as sets of plugin ids.
  The benchmark will be run with only these plugins enabled and all
  others disabled
- Support for configurable concurrency. This relies on RTS -ol and -po
  flags to place the RTS traces in the target location rather than in
  the cwd

This change requires two commits, the next one places
ghcide/bench/hist/Main.hs into its final location to help 'git'
recognize the change as a file move
Goal is to display the formatted CSV (via column) one row per line
We parse maxResidency and allocatedBytes from the RTS -S output, but runSessionWithHandles kills the server without waiting for it to exit and these stats don't get logged.

The solution is to use runSessionWithHandles', but unfortunately it is internal and not exposed. I have raised a PR to expose it and in the meantime we need a source repo package.
@pepeiborra pepeiborra merged commit d0e3e0f into master Aug 25, 2022
sloorush pushed a commit to sloorush/haskell-language-server that referenced this pull request Sep 12, 2022
* extract ghcide:experiments-types

* extract haskell-language-server:plugins and let go of examples

The main goal here is to move the Plugins module into an internal
library so that it can be reused from the benchmark suite.

In order to
make that easier, and since they hardly serve a purpose in a repository
with 25 plugins, I delete the Example and Example2 plugin descriptors
and their dependencies.

* HLS benchmark suite

Port the ghcide benchmark suite to HLS and benchmark plugin
"configurations" independently.

This includes the following changes to the ghcide benchmark suite and
HLS:
- Support for "configurations" which are defined as sets of plugin ids.
  The benchmark will be run with only these plugins enabled and all
  others disabled
- Support for configurable concurrency. This relies on RTS -ol and -po
  flags to place the RTS traces in the target location rather than in
  the cwd

This change requires two commits, the next one places
ghcide/bench/hist/Main.hs into its final location to help 'git'
recognize the change as a file move

* ghcide/bench/hist/Main.hs -> bench/Main.hs

* CI - fix artifact names for uniqueness

* disable shorten HLS step

* Do not store eventlogs to avoid out of disk space

* render durations up to milliseconds

* shorten titles

Goal is to display the formatted CSV (via column) one row per line

* exclude formatting plugin configurations

* Extract ghcide-bench to a standalone package

* ghcide-bench: fix stderr capturing

* Fix mem stats

We parse maxResidency and allocatedBytes from the RTS -S output, but runSessionWithHandles kills the server without waiting for it to exit and these stats don't get logged.

The solution is to use runSessionWithHandles', but unfortunately it is internal and not exposed. I have raised a PR to expose it and in the meantime we need a source repo package.

* feedbacks

* delete Example plugins
@fendor
Copy link
Collaborator

fendor commented Oct 6, 2022

PR seems to have removed the example plugins, an accident?

edit: i see, not an accident

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants