Skip to content

Add Thread.Sleep(100) to throttle REPL when it's non-interactive #1694

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

Merged

Conversation

colinblaise
Copy link
Contributor

We integrated PSES into our project and noticed a considerable increase in CPU usage while PSES is running.

I downloaded the PSES project and was able to step through the code and see it is executing a while loop with zero throttling.

Adding a Thread.Sleep(100) brought down the CPU of my application considerably when PSES was running. I did not notice any increase in latency when using PSES itself either.

Here are the parameters we are using:

powershell.AddCommand("Start-EditorServices")
        .AddParameter("HostName", "monaco")
        .AddParameter("HostProfileId", "0")
        .AddParameter("HostVersion", "1.0.0")
        .AddParameter("LogPath", Path.ChangeExtension(Path.GetTempFileName(), ".log"))
        .AddParameter("LogLevel", "Normal")
        .AddParameter("FeatureFlags", Array.Empty<string>())
        .AddParameter("BundledModulesPath", BundledModulesPath)
        .AddParameter("SessionDetailsPath", _sessionInfoFilePath)
        .AddParameter("LanguageServiceOnly");

Using Thread.Sleep(100) is heavy handed, so the solution can likely be expanded to either take in a variable that specifies the sleep amount, or better yet, remove the while loop entirely and refactor it into an even-driven approach and execute only when we have data in _taskQueue.

This solution works for us, but I am not at all familiar with the rest of this project. So please let me know if there are any obvious disadvantages with making this change. Thanks!

@andyleejordan
Copy link
Member

My guess is that you're seeeing this issue because DoOneRepl has become a no-op, since (I'm assuming) you're running non-interactively, and so don't have a a Console REPL:

private void DoOneRepl(CancellationToken cancellationToken)
{
if (!_hostInfo.ConsoleReplEnabled)
{
return;
}

Instead of a sleep, I'll see if we can use something from System.Threading.Channels.

In our use case it's only non-interactive during testing, as the extension provides an interactive console REPL and so DoOneRepl sits waiting in PSReadLine for user input.

@colinblaise
Copy link
Contributor Author

Ah gotcha! The code makes more sense with that in mind. Right, DoOneRepl is a no-op for us since we have it disabled.

@dkattan
Copy link
Contributor

dkattan commented Feb 4, 2022

@andschwa @colinblaise What if in the short term we move the Thread.Sleep onto line 603 so this only affects our oddball scenario where ConsoleReplEnabled is false

Ideally we'd get this done pretty quick as we're trying to launch Intellisense in our product next week and I don't want to use a custom PSES build.

@andyleejordan
Copy link
Member

@dkattan I'd be fine with that! With a comment to the effect of "Throttle the REPL loop with a sleep because we're not interactively reading input from the user."

@colinblaise
Copy link
Contributor Author

Done. Only sleeps now when ConsoleReplEnabled is false.

@dkattan
Copy link
Contributor

dkattan commented Feb 4, 2022

@andschwa LGTM!

@andyleejordan andyleejordan changed the title Added Thread.Sleep(100) to reduce CPU cycles Add Thread.Sleep(100) to throttle REPL when it's non-interactive Feb 8, 2022
@andyleejordan andyleejordan merged commit 5f54c43 into PowerShell:master Feb 8, 2022
@andyleejordan andyleejordan added Area-API Issue-Enhancement A feature request (enhancement). labels Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-API Issue-Enhancement A feature request (enhancement).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants