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

Resolve shell env API performance issue for single env approach #238488

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ export interface IShellEnvDetectionCapability {
startEnvironmentSingleVar(isTrusted: boolean): void;
setEnvironmentSingleVar(key: string, value: string | undefined, isTrusted: boolean): void;
endEnvironmentSingleVar(isTrusted: boolean): void;
applyEnvironmentDiff(env: Map<string, string>, isTrusted: boolean): void;
}

export const enum CommandInvalidationReason {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ export class ShellEnvDetectionCapability extends Disposable implements IShellEnv
private _pendingEnv: Map<string, string> | undefined;
private _env: Map<string, string> = new Map();
get env(): Map<string, string> { return this._env; }

private readonly _onDidChangeEnv = this._register(new Emitter<Map<string, string>>());
readonly onDidChangeEnv = this._onDidChangeEnv.event;

Expand Down Expand Up @@ -59,8 +58,33 @@ export class ShellEnvDetectionCapability extends Disposable implements IShellEnv
if (!this._pendingEnv) {
return;
}
this._env = this._pendingEnv;
this.applyEnvironmentDiff(this._pendingEnv, isTrusted);
this._pendingEnv = undefined;
this._onDidChangeEnv.fire(this._env);
}
// Check for environment differs, and was updated.
// This way we only fire an event if the environment actually changed.

// TODO: Handle deleted variable on TS side. It is already one in bash side
// unsetEnvironmentSingleVar()
applyEnvironmentDiff(env: Map<string, string>, isTrusted: boolean): void {
if (!isTrusted) {
return;
}

let envDiffers: boolean = false;

for (const [key, value] of env.entries()) {
if (this._env.has(key) && this._env.get(key) === value) {
// Do nothing
} else if (value !== undefined) {
this._env.set(key, value);
envDiffers = true;
}
}

if (envDiffers) {
this._onDidChangeEnv.fire(this._env);
return;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ export class ShellIntegrationAddon extends Disposable implements IShellIntegrati
if (arg0 !== undefined) {
try {
const env = JSON.parse(deserializeMessage(arg0));
this._createOrGetShellEnvDetection().setEnvironment(env, arg1 === this._nonce);
this._createOrGetShellEnvDetection().applyEnvironmentDiff(env, arg1 === this._nonce);
} catch (e) {
this._logService.warn('Failed to parse environment from shell integration sequence', arg0);
}
Expand Down
48 changes: 24 additions & 24 deletions src/vs/workbench/api/common/extHostTerminalShellIntegration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,30 +55,30 @@ export class ExtHostTerminalShellIntegration extends Disposable implements IExtH
}));

// Convenient test code:
// this.onDidChangeTerminalShellIntegration(e => {
// console.log('*** onDidChangeTerminalShellIntegration', e);
// });
// this.onDidStartTerminalShellExecution(async e => {
// console.log('*** onDidStartTerminalShellExecution', e);
// // new Promise<void>(r => {
// // (async () => {
// // for await (const d of e.execution.read()) {
// // console.log('data2', d);
// // }
// // })();
// // });
// for await (const d of e.execution.read()) {
// console.log('data', d);
// }
// });
// this.onDidEndTerminalShellExecution(e => {
// console.log('*** onDidEndTerminalShellExecution', e);
// });
// setTimeout(() => {
// console.log('before executeCommand(\"echo hello\")');
// Array.from(this._activeShellIntegrations.values())[0].value.executeCommand('echo hello');
// console.log('after executeCommand(\"echo hello\")');
// }, 4000);
this.onDidChangeTerminalShellIntegration(e => {
console.log('*** onDidChangeTerminalShellIntegration', e.shellIntegration.env);
});
this.onDidStartTerminalShellExecution(async e => {
console.log('*** onDidStartTerminalShellExecution', e);
// new Promise<void>(r => {
// (async () => {
// for await (const d of e.execution.read()) {
// console.log('data2', d);
// }
// })();
// });
for await (const d of e.execution.read()) {
console.log('data', d);
}
});
this.onDidEndTerminalShellExecution(e => {
console.log('*** onDidEndTerminalShellExecution', e);
});
setTimeout(() => {
console.log('before executeCommand(\"echo hello\")');
Array.from(this._activeShellIntegrations.values())[0].value.executeCommand('echo hello');
console.log('after executeCommand(\"echo hello\")');
}, 4000);
}

public $shellIntegrationChange(instanceId: number): void {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ fi

VSCODE_SHELL_INTEGRATION=1

vsc_env_keys=()
vsc_env_values=()

# Run relevant rc/profile only if shell integration has been injected, not when run manually
if [ "$VSCODE_INJECTION" == "1" ]; then
if [ -z "$VSCODE_SHELL_LOGIN" ]; then
Expand Down Expand Up @@ -214,16 +217,77 @@ __vsc_update_cwd() {
builtin printf '\e]633;P;Cwd=%s\a' "$(__vsc_escape_value "$__vsc_cwd")"
}

# __vsc_update_env() {
# builtin printf '\e]633;EnvSingleStart;%s;\a' $__vsc_nonce
# for var in $(compgen -v); do
# if printenv "$var" >/dev/null 2>&1; then
# value=$(builtin printf '%s' "${!var}")
# builtin printf '\e]633;EnvSingleEntry;%s;%s;%s\a' "$var" "$(__vsc_escape_value "$value")" $__vsc_nonce
# fi
# done
# builtin printf '\e]633;EnvSingleEnd;%s;\a' $__vsc_nonce
# }

updateEnvCache() {
local key="$1"
local value="$2"

for i in "${!vsc_env_keys[@]}"; do
if [[ "${vsc_env_keys[$i]}" == "$key" ]]; then
if [[ "${vsc_env_values[$i]}" != "$value" ]]; then
anthonykim1 marked this conversation as resolved.
Show resolved Hide resolved
vsc_env_values[$i]="$value"
# printf "Updated value for %s\n" "$key"
builtin printf '\e]633;EnvSingleEntry;%s;%s;%s\a' "$key" "$(__vsc_escape_value "$value")" "$__vsc_nonce"
fi
return
fi
done

vsc_env_keys+=("$key")
vsc_env_values+=("$value")
# printf "Added new variable %s\n" "$key"
builtin printf '\e]633;EnvSingleEntry;%s;%s;%s\a' "$key" "$(__vsc_escape_value "$value")" "$__vsc_nonce"
}

trackMissingEnvVars() {
anthonykim1 marked this conversation as resolved.
Show resolved Hide resolved
# Capture current environment variables in an array
local current_env_keys=()
while IFS='=' read -r key value; do
current_env_keys+=("$key")
done < <(env)

# Compare vsc_env_keys with current_env_keys
for key in "${vsc_env_keys[@]}"; do
local found=false
for env_key in "${current_env_keys[@]}"; do
if [[ "$key" == "$env_key" ]]; then
found=true
break
fi
done
if [[ "$found" == false ]]; then
printf "Missing environment variable: %s\n" "$key"
unset 'vsc_env_keys[i]'
unset 'vsc_env_values[i]'
fi
done

# Remove gaps from unset
vsc_env_keys=("${vsc_env_keys[@]}")
vsc_env_values=("${vsc_env_values[@]}")
}

__vsc_update_env() {

start_time=$(date +%s)
builtin printf "Start time: %s\n" "$start_time"

builtin printf '\e]633;EnvSingleStart;%s;\a' $__vsc_nonce

while IFS='=' read -r key value; do
updateEnvCache "$key" "$value"
done < <(env)

builtin printf '\e]633;EnvSingleEnd;%s;\a' $__vsc_nonce

end_time=$(date +%s)
elapsed_time=$(($end_time - $start_time))

# builtin printf "Time taken: $elapsed_time seconds\n"

trackMissingEnvVars
builtin printf "Time taken: $elapsed_time seconds\n"
anthonykim1 marked this conversation as resolved.
Show resolved Hide resolved
}

__vsc_command_output_start() {
if [[ -z "${__vsc_first_prompt-}" ]]; then
Expand Down Expand Up @@ -252,9 +316,9 @@ __vsc_command_complete() {
fi
__vsc_update_cwd

# if [ "$__vsc_stable" = "0" ]; then
# __vsc_update_env
# fi
if [ "$__vsc_stable" = "0" ]; then
__vsc_update_env
fi
}
__vsc_update_prompt() {
# in command execution
Expand Down Expand Up @@ -285,9 +349,9 @@ __vsc_precmd() {
__vsc_first_prompt=1
__vsc_update_prompt

# if [ "$__vsc_stable" = "0" ]; then
# __vsc_update_env
# fi
if [ "$__vsc_stable" = "0" ]; then
__vsc_update_env
fi
}

__vsc_preexec() {
Expand Down
Loading