Consider caching Alias & Command information

[TylerLeonhardt]

On every textDocument/references request and every textDocument/codeLens request, we rely on the pipeline to get the current available.

This is time consuming and bottlenecked by the pipeline thread. As such, we should consider caching this result based on the state of the Integrated Console.

Coming off the the back of #980, one of the patterns I liked was the ability to tell if something was run in the PSIC using PSRL’s ENTER handler and F8’s handler.

Bringing this concept in, we can use it as a “Dirty” check where we cache aliases and command info results, and only refresh the cache when the PSIC is Dirty.

That way, for example, our textDocument/references request and textDocument/codeLens request only rely on the pipeline once the cache is Dirty and an otherwise run unblocked giving us a perf improvement.

[SeeminglyScience]

Ah, looks like we already do.

PowerShellEditorServices/src/PowerShellEditorServices/Language/LanguageService.cs

Lines 730 to 735 in 9358709

	private async Task GetAliasesAsync()
	{
	if (_areAliasesLoaded)
	{
	return;
	}

Maybe we should change the name of GetAliasesAsync to something like EnsureAliasCacheAsync to make that more clear.

[TylerLeonhardt]

@SeeminglyScience Yeah nice find… still doesn’t look like the cache ever gets invalidated? And it’d be nice to have something similar for Commands for the Command Explorer and other operations.

[rkeithhill]

I think an easy "potential" win is simply breaking up the single async thread that reads and dispatches messages into two separate threads. One that reads messages from the client and queues them. The other thread would be essentially what we have to today but rather than read directly from the client, it reads from the PSES queue. The primary advantage is that the thread reading from the client could maintain a separate cancellation queue such that the dispatch thread would first check the message it has dequeued to see if it has been canceled. If so, it would send the appropriate ack back to the client and grab the next message to dispatch. This would prevent PSES from getting so backed up processing requests that the client doesn't need anymore.

So yeah, I've done a prototype implementation of such a beast. And no, I'm not confident it is good to go as-is. It needs someone with more async-fu than me to check it over. I would have been more comfortable doing this not using async but hey, that's my limitation with server-oriented async. :-(

The PR in question also adds some logging instrumentation to keep track of queue wait times (as can be seen by output from the PsesLogAnalyzer). You will notice that when using that PR (#832), there still can be quite long queue wait times but at least we are seeing reality here. With all the logging we've put in for timing requests e.g. completion request times, we have no idea how long that LSP message was sitting in the pipe before we read it and started processing it. Sorry to sound like a broken record but we really need to do something to process cancel requests IMO. I don't care at all if it's not the approach I've taken in PR #832. It just needs to happen ... somehow. :-)

[SeeminglyScience]

@TylerLeonhardt

And it’d be nice to have something similar for Commands for the Command Explorer and other operations.

On second thought, that gets real complicated. At first glance it seems like that would help us avoid tying up the pipeline thread, but quite a few properties/methods on CommandInfo will try to force their way back to the pipeline thread anyway. Most notably CommandInfo.Parameters will more than likely end up with dead lock if we try to access it from another thread.

We'd have to create a flat version of CommandInfo that only contains the info we use specifically, and then cache that. That's totally worth doing imo, just more work.

@rkeithhill

I think an easy "potential" win is simply breaking up the single async thread that reads and dispatches messages into two separate threads. One that reads messages from the client and queues them. The other thread would be essentially what we have to today but rather than read directly from the client, it reads from the PSES queue. The primary advantage is that the thread reading from the client could maintain a separate cancellation queue such that the dispatch thread would first check the message it has dequeued to see if it has been canceled. If so, it would send the appropriate ack back to the client and grab the next message to dispatch. This would prevent PSES from getting so backed up processing requests that the client doesn't need anymore.

👍 * 1000

So yeah, I've done a prototype implementation of such a beast. And no, I'm not confident it is good to go as-is. It needs someone with more async-fu than me to check it over. I would have been more comfortable doing this not using async but hey, that's my limitation with server-oriented async. :-(

Actually I'm inclined to say that the message reader should be entirely sync. Especially with the whole custom SynchronizationContext thing. Having two separate workflows that absolutely can't use the same thread makes me nervous about both using tpm. I feel like that's going to end up with both the message reader and the dispatcher both trying to get back on the sync context thread at the same time. I don't know enough about how the sync context is set up to say with any confidence that that'll happen though.

My idea for threading in general is:

1. A sync thread for message reading. This thread populates a BlockingCollection with requests
2. A sync thread for message dispatching. This thread would read from the blocking collection above, and dispatch handlers to the thread pool.
3. Dispatch handlers thread pool. Any handlers would be dispatched to various threads, if they need to access the pipeline thread they would write a custom job object to a BlockingCollection.
4. A pipeline controller thread. This thread would read the custom job objects created by the dispatch handlers.

Though tbh, I'm tempted to say none of the project should be async. TPM is fantastic when you can use it all the way though. The second you need to dip between async and sync, everything gets really complicated. I think it would be harder to read, but I think it would also be harder to introduce new race conditions. Just something to think about, not something I feel particularly strongly about.