Renovate PowerShellEditorServices

[rjmholt]

With the performance issues that are affecting EditorServices, some of the code getting fairly spidery and hard to maintain, and various test/build obsolescence issues, we should probably contemplate an ongoing effort to revitalise the codebase.

This issue is intended as a starting point for design discussions/suggestions to make EditorServices more manageable, more efficient and possibly more configurable. (With main emphasis on restoring performance and stability).

So just comment with any ideas or thoughts you've got.

Anything we generally agree on can be spun out into its own issue for work.

[rjmholt]

Testing

The tests for EditorServices have fallen behind a bit, and the existing ones are a bit flaky. The tests are all pretty monolithic. So here are my ideas there:

• Create unit tests to go alongside the existing integration tests
  • Each feature (ideally each class) should have a unit test set under a path like /tests/PowerShellEditorServices[.Host]/UnitTests/...
  • Parts of EditorServices should be mocked out as needed
• Move existing integration tests under an /Integration path. These would be any test involving HTTP requests (i.e. end-to-end deserialise -> process -> serialise tests).
• Break up existing testing into component/feature based classes
• We need to test on .NET Core (Windows and UNIX), so we probably need to build a netcoreapp2.0 and a net451 version of the tests.

[rjmholt]

Project Code Layout

EditorServices has gotten more complex as time has gotten on and is probably due for a restructuring to manage the complexity.

Naturally we can't change the whole project's architecture, but it might be possible to work out what we think we should have and then work out how we gradually migrate to that.

Base projects

• We should make sure it's well understood what belongs in PowerShellEditorServices vs PowerShellEditorServices.Host, PowerShellEditorServices.Protocol and PowerShellEditorServices.VSCode. Mainly the distinction between the 1st and 2nd.
• We should document that in a markdown document or similar.
• To prevent tangling things together, it might be worth having a PowerShellEditorServices.Common?

Features and Components

• We should work out what is meant by "Feature" and "Component" and document (or link to a VSCode document) that describes the concept.
• Based on newer additions like the CodeLens providers, we should ensure that all feature implementations live in their own directory and do all the component registration, etc. Then features/components should only be accessed through the component registry.

Consolidate components

• There seem to be a few components that have parts in both PowerShellEditorServices and PowerShellEditorServices.Host for no obvious reason. We should either consolidate them or document the reason.

[rjmholt]

Request Handling

Currently request handling is sometimes done by a component/feature but a lot of the time by LanguageServer.

Instead, requests should probably be handled individually by registered request handlers that follow an interface (like IRequestHandler<TRequest, TRequestParams>, with similar for responses).

This interface would probably have a single async TResult HandleRequest or async IResult HandleRequest method, so we can separate out the result return logic from the sendResult logic.

A request handler object would then be able to take a CancellationToken in either the method or the constructor, with the cancellation request handler being able to set the cancellation token using the same source.

This might not be an efficient thing to do (compared to handler methods), but we should in any case find a way to silo relevant request handling code. This would help with thread safety as well.

[rjmholt]

Benchmarking

Since performance is currently an issue for EditorServices, we should invest in some performance benchmarking infrastructure/tests so that we can identify performance regressions.

This would live under the testing code, and likely be extra instrumentation on existing tests, written in a low-overhead, general way, so that any test can be (or is automatically) instrumented with performance trackers.

We would also need specifically written standalone benchmarks of common usages, such as:

• Frequent intermittent edits
• Calling a formatter
• Calling PSScriptAnalyzer
• CodeLens results/queries
• Large script files
• Large numbers of small script files
• Projects using a complicated module structure

[rjmholt]

Caching and Laziness

We currently do a little bit of caching, plus some cleverness for incremental edits for script files, but there are still more opportunities for caching and lazy operations.

• With the current architecture, caches for things like Symbols, ASTs and CodeLenses don't have an obvious place to live. To prevent strange cache errors, it would probably be easier for all the caches that refer to a single file live with that file. So one possibility is to wrap a ScriptFile object with one that manages calls to script analysis, symbols, ASTs etc.
• We're currently generating (and allocating) an AST for every text edit, as well as symbols, CodeLenses, etc. Worse, PSScriptAnalyzer will then go and reparse the file and generate another AST! There might be a couple of things we can improve on here:
  • We can make the text edit and rather than immediately reparsing, just mark the file as dirty. Then the AST property can lazily regenerate the AST when it's actually referenced.
  • Script analysis currently sets a (pretty long) timeout for cancellations. To prevent bogging ourselves down when editing is constant, we could do something similar with AST parsing/intelliisense/symbols. We would need a way to make sure we never exceed a maximum time window with timeouts (i.e. the timeout is not per request, but instead each request checks if a timeout has been set and if not sets it with the decided time). Again the easiest way to do this might be AST refresh timeouts (i.e. we allow the AST to get stale within a certain timeframe)
  • Ideally we can get a (big) change into ScriptAnalyzer so it will take an AST, saving a second AST generation.
  • The big task for high-speed correctness would be to create a way to splice ASTs (so we only regenerate the edited part). With a composition object, or maybe some cached-reflection surgery this might be possible.

In any event, we should be caching the following:

• Symbols
• CodeLenses
• Completions/intellisense
• Script analysis

We should also try to have some logic to only perform operations in the windows that have focus.

[rjmholt]

Implementing a handler for CancelRequest

A possibly large win for us would be implementing a $/cancelRequest handler.

All our handlers are currently asynchronous, and many support a cancellation token. But we would need to make the following changes:

• Ensure that requests are all handled asynchronously, so that we can receive a cancellation request while still handling previous ones.
• Implement a cancellation request handler method/object that has a CancellationTokenSource which all other request handlers have. Have the cancellation request handler set the cancellation token when it receives a request.
• Put in cancellation handling logic to each request handler
  • We would need to decide for each request whether we can return a partial result or should just always bail out

[rjmholt]

/cc @joeyaiello, @tylerl0706, @rkeithhill, @SeeminglyScience

[rjmholt]

Here are some pre-existing architecture issues that are worth considering too:

• Split PowerShellEditorServices.Protocol library into its own standalone project #53
• Redesign ExtensionService to use granular feature interfaces #313
• Make fuller use of Serilog #680
• Build and keep a cache of installed modules for intellisense #263
• Request referenced files from the client instead of attempting to load from the language service #187
• Improve startup sequence of ProtocolServer implementations #106
• Improve shutdown sequence of DebugAdapter #110

[TylerLeonhardt]

Spoke to @rjmholt offline about this.

Totally a great collection of things that could be better.

The first one we think is worth tackling in the request handling. If we tear down the monolithic approach to handling messages by replacing it with classes that are made to handle single messages, we can more easily write unit tests for this.

This means that after this work is done, we focus on tests! We can add unit tests for each request, and refresh our current integration/functional tests.

While implementing the request handling, we can use that as an opportunity to implement better cancellation logic for when we (finally) get to handling cancelRequest.