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

Reload environment variables only on WM_SETTINGCHANGE #15102

Open
lhecker opened this issue Apr 4, 2023 · 9 comments
Open

Reload environment variables only on WM_SETTINGCHANGE #15102

lhecker opened this issue Apr 4, 2023 · 9 comments
Labels
Area-Performance Performance-related issue Area-Quality Stability, Performance, Etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Milestone

Comments

@lhecker
Copy link
Member

lhecker commented Apr 4, 2023

til::env is rather complex, reads a lot of registry keys and requires lots of string operations. Since #14999 we recreate the til::env cache every time we create a new tab, but this isn't necessary, considering that changes to environment variables are exceedingly rare. Most users will run a Windows Terminal instance without it ever seeing a single environment variable change.

👉 Listen to WM_SETTINGCHANGE and only then recreate the globally shared cache. If multi-threading is a concern, just encode the variables back into a \0 separated string, the same way we get it from the OS anyways already. This would reflect how ConEmu does it: Maximus5/ConEmu#468

@lhecker lhecker added Area-Performance Performance-related issue Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. Area-Quality Stability, Performance, Etc. labels Apr 4, 2023
@lhecker lhecker added this to the Terminal v1.19 milestone Apr 4, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Apr 4, 2023
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Apr 4, 2023
@voronoipotato
Copy link

microsoft/winget-cli#549

@lhecker some people in the winget comments thread seem to think that microsoft/terminal should be responsible for refreshing the environment variables after a program is installed through winget. I'm highly skeptical of this claim, and it sounds backwards to me, but could we emit a WM_SETTINGCHANGE from winget to refresh the env variables after this is merged?

Thanks!

@lhecker
Copy link
Member Author

lhecker commented May 9, 2023

Since #14999 we already get the latest environment variables whenever you create a new tab or pane. But
Windows Terminal cannot be responsible for refreshing them the way some people ask for it in that issue.

In short, we cannot refresh the variables of another process, like the shell for instance. While overwriting the environment stored in RTL_USER_PROCESS_PARAMETERS in a foreign process would already be quite difficult in on itself, it wouldn't solve the problem for PowerShell in particular, because it parses the environment on startup and puts it into what's effectively a Map<string, string>. Us updating RTL_USER_PROCESS_PARAMETERS wouldn't update that map - only PowerShell itself can do that.

This issue is just about how #14999 took a large shortcut and burns dozens of millions of cycles during startup (and every time you open a tab), because it reconstructs the environment variables every time without caching. I want it to only do that when we receive a WM_SETTINGCHANGE so that it's faster.

@voronoipotato
Copy link

Thank you for all the information, that was very helpful!

@citelao
Copy link

citelao commented May 16, 2023

@lhecker, if I understand correctly:

  • As of Reload environment variables by default; add setting to disable #14999 (which is not built into any released bits as of 16 May 2023), we fetch the latest environment variables when users create a new tab or pane.
    • So if I winget install vim, a new tab/pane of Terminal will have vim.
  • However, that change reloads environment variables every time users open a new tab or pane. This bug tracks a resolution to that:
    • Listen to WM_SETTINGCHANGE in Terminal; only reload environment variables for new tabs/panes after that change is fired.
    • This proposed change will not affect observed behavior: users must still open new tabs/panes to refresh their environment variables variables.

Am I understanding correctly?

@lhecker
Copy link
Member Author

lhecker commented May 16, 2023

Yep, that's correct.

@sba923
Copy link

sba923 commented May 22, 2023

What's the order of magnitude of the time added by #14999 when opening a new tab?

@lhecker
Copy link
Member Author

lhecker commented May 22, 2023

That heavily depends on whether you have animations enabled in Windows or not (and which ones). WinUI's animations are an extreme resource hog. From what I can tell it's about 1.5-3% if they're enabled and about 5-10% if disabled, edit: relative to the total CPU cost inside WindowsTerminal.exe.

@lhecker lhecker modified the milestones: Terminal v1.19, Backlog May 22, 2023
@zadjii-msft
Copy link
Member

For folks who maybe got confused like I did:

Reloading env vars takes the same number of cycles regardless of whether animations are enabled or not.

Opening tabs with animations enabled takes more CPU cycles than without, so the impact of env vars is relatively less.

@sba923
Copy link

sba923 commented May 22, 2023

For folks who maybe got confused like I did:

Reloading env vars takes the same number of cycles regardless of whether animations are enabled or not.

Opening tabs with animations enabled takes more CPU cycles than without, so the impact of env vars is relatively less.

Thanks for the clarification!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Performance Performance-related issue Area-Quality Stability, Performance, Etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests

5 participants