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.

Sign up for GitHub

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

Jump to bottom

Reduce unnecessary calls to PtyService #185389

Merged
merged 3 commits into from
Jun 22, 2023
Merged

Reduce unnecessary calls to PtyService #185389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e6881a3
memoize PtyService.getEnvironment
Tyriar Jun 16, 2023
31a5d12
Don't call PtyService.resize consecutively with the same values
Tyriar Jun 16, 2023
99e380d
Clean up RemotePty interface, use DI in remote backend
Tyriar Jun 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
65 changes: 32 additions & 33 deletions src/vs/workbench/contrib/terminal/browser/remotePty.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,8 @@ import { RemoteTerminalChannelClient } from 'vs/workbench/contrib/terminal/commo
import { IRemoteAgentService } from 'vs/workbench/services/remote/common/remoteAgentService';

export class RemotePty extends Disposable implements ITerminalChildProcess {
private readonly _onProcessData = this._register(new Emitter<string | IProcessDataEvent>());
readonly onProcessData = this._onProcessData.event;
private readonly _onProcessReady = this._register(new Emitter<IProcessReadyEvent>());
readonly onProcessReady = this._onProcessReady.event;
private readonly _onDidChangeProperty = this._register(new Emitter<IProcessProperty<any>>());
readonly onDidChangeProperty = this._onDidChangeProperty.event;
private readonly _onProcessExit = this._register(new Emitter<number | undefined>());
readonly onProcessExit = this._onProcessExit.event;
private readonly _onRestoreCommands = this._register(new Emitter<ISerializedCommandDetectionCapability>());
readonly onRestoreCommands = this._onRestoreCommands.event;

private _startBarrier: Barrier;

private _inReplay = false;

private _properties: IProcessPropertyMap = {
private readonly _startBarrier: Barrier;
private readonly _properties: IProcessPropertyMap = {
cwd: '',
initialCwd: '',
fixedDimensions: { cols: undefined, rows: undefined },
Expand All @@ -41,15 +27,27 @@ export class RemotePty extends Disposable implements ITerminalChildProcess {
failedShellIntegrationActivation: false,
usedShellIntegrationInjection: undefined
};
private readonly _lastDimensions: { cols: number; rows: number } = { cols: -1, rows: -1 };

get id(): number { return this._id; }
private _inReplay = false;

private readonly _onProcessData = this._register(new Emitter<string | IProcessDataEvent>());
readonly onProcessData = this._onProcessData.event;
private readonly _onProcessReady = this._register(new Emitter<IProcessReadyEvent>());
readonly onProcessReady = this._onProcessReady.event;
private readonly _onDidChangeProperty = this._register(new Emitter<IProcessProperty<any>>());
readonly onDidChangeProperty = this._onDidChangeProperty.event;
private readonly _onProcessExit = this._register(new Emitter<number | undefined>());
readonly onProcessExit = this._onProcessExit.event;
private readonly _onRestoreCommands = this._register(new Emitter<ISerializedCommandDetectionCapability>());
readonly onRestoreCommands = this._onRestoreCommands.event;

constructor(
private _id: number,
readonly id: number,
readonly shouldPersist: boolean,
private readonly _remoteTerminalChannel: RemoteTerminalChannelClient,
private readonly _remoteAgentService: IRemoteAgentService,
private readonly _logService: ILogService
@IRemoteAgentService private readonly _remoteAgentService: IRemoteAgentService,
@ILogService private readonly _logService: ILogService
) {
super();
this._startBarrier = new Barrier();
Expand All @@ -63,9 +61,9 @@ export class RemotePty extends Disposable implements ITerminalChildProcess {
throw new Error('Could not fetch remote environment');
}

this._logService.trace('Spawning remote agent process', { terminalId: this._id });
this._logService.trace('Spawning remote agent process', { terminalId: this.id });

const startResult = await this._remoteTerminalChannel.start(this._id);
const startResult = await this._remoteTerminalChannel.start(this.id);

if (startResult && 'message' in startResult) {
// An error occurred
Expand All @@ -83,7 +81,7 @@ export class RemotePty extends Disposable implements ITerminalChildProcess {

shutdown(immediate: boolean): void {
this._startBarrier.wait().then(_ => {
this._remoteTerminalChannel.shutdown(this._id, immediate);
this._remoteTerminalChannel.shutdown(this.id, immediate);
});
}

Expand All @@ -93,17 +91,18 @@ export class RemotePty extends Disposable implements ITerminalChildProcess {
}

this._startBarrier.wait().then(_ => {
this._remoteTerminalChannel.input(this._id, data);
this._remoteTerminalChannel.input(this.id, data);
});
}

resize(cols: number, rows: number): void {
if (this._inReplay) {
if (this._inReplay || this._lastDimensions.cols === cols && this._lastDimensions.rows === rows) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, will keep my eye out for issues, i wonder if this could go wrong

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there should only ever be a single window attached, it should stay in sync

return;
}
this._startBarrier.wait().then(_ => {

this._remoteTerminalChannel.resize(this._id, cols, rows);
this._lastDimensions.cols = cols;
this._lastDimensions.rows = rows;
this._remoteTerminalChannel.resize(this.id, cols, rows);
});
}

Expand All @@ -125,12 +124,12 @@ export class RemotePty extends Disposable implements ITerminalChildProcess {
}

this._startBarrier.wait().then(_ => {
this._remoteTerminalChannel.acknowledgeDataEvent(this._id, charCount);
this._remoteTerminalChannel.acknowledgeDataEvent(this.id, charCount);
});
}

async setUnicodeVersion(version: '6' | '11'): Promise<void> {
return this._remoteTerminalChannel.setUnicodeVersion(this._id, version);
return this._remoteTerminalChannel.setUnicodeVersion(this.id, version);
}

async getInitialCwd(): Promise<string> {
Expand All @@ -142,11 +141,11 @@ export class RemotePty extends Disposable implements ITerminalChildProcess {
}

async refreshProperty<T extends ProcessPropertyType>(type: T): Promise<IProcessPropertyMap[T]> {
return this._remoteTerminalChannel.refreshProperty(this._id, type);
return this._remoteTerminalChannel.refreshProperty(this.id, type);
}

async updateProperty<T extends ProcessPropertyType>(type: T, value: IProcessPropertyMap[T]): Promise<void> {
return this._remoteTerminalChannel.updateProperty(this._id, type, value);
return this._remoteTerminalChannel.updateProperty(this.id, type, value);
}

handleData(e: string | IProcessDataEvent) {
Expand All @@ -156,7 +155,7 @@ export class RemotePty extends Disposable implements ITerminalChildProcess {
this._onProcessExit.fire(e);
}
processBinary(e: string): Promise<void> {
return this._remoteTerminalChannel.processBinary(this._id, e);
return this._remoteTerminalChannel.processBinary(this.id, e);
}
handleReady(e: IProcessReadyEvent) {
this._onProcessReady.fire(e);
Expand Down Expand Up @@ -202,7 +201,7 @@ export class RemotePty extends Disposable implements ITerminalChildProcess {
}

handleOrphanQuestion() {
this._remoteTerminalChannel.orphanQuestionReply(this._id);
this._remoteTerminalChannel.orphanQuestionReply(this.id);
}

async getLatency(): Promise<number> {
Expand Down
5 changes: 3 additions & 2 deletions src/vs/workbench/contrib/terminal/browser/remoteTerminalBackend.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class RemoteTerminalBackend extends BaseTerminalBackend implements ITerminalBack
readonly remoteAuthority: string | undefined,
private readonly _remoteTerminalChannel: RemoteTerminalChannelClient,
@IRemoteAgentService private readonly _remoteAgentService: IRemoteAgentService,
@IInstantiationService private readonly _instantiationService: IInstantiationService,
@ILogService logService: ILogService,
@ICommandService private readonly _commandService: ICommandService,
@IStorageService private readonly _storageService: IStorageService,
Expand Down Expand Up @@ -215,7 +216,7 @@ class RemoteTerminalBackend extends BaseTerminalBackend implements ITerminalBack
rows,
unicodeVersion
);
const pty = new RemotePty(result.persistentTerminalId, shouldPersist, this._remoteTerminalChannel, this._remoteAgentService, this._logService);
const pty = this._instantiationService.createInstance(RemotePty, result.persistentTerminalId, shouldPersist, this._remoteTerminalChannel);
this._ptys.set(result.persistentTerminalId, pty);
return pty;
}
Expand All @@ -227,7 +228,7 @@ class RemoteTerminalBackend extends BaseTerminalBackend implements ITerminalBack

try {
await this._remoteTerminalChannel.attachToProcess(id);
const pty = new RemotePty(id, true, this._remoteTerminalChannel, this._remoteAgentService, this._logService);
const pty = this._instantiationService.createInstance(RemotePty, id, true, this._remoteTerminalChannel);
this._ptys.set(id, pty);
return pty;
} catch (e) {
Expand Down
11 changes: 8 additions & 3 deletions src/vs/workbench/contrib/terminal/electron-sandbox/localPty.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ import { IPtyHostProcessReplayEvent, ISerializedCommandDetectionCapability } fro
* created on the local pty host.
*/
export class LocalPty extends Disposable implements ITerminalChildProcess {
private _inReplay = false;
private _properties: IProcessPropertyMap = {
private readonly _properties: IProcessPropertyMap = {
cwd: '',
initialCwd: '',
fixedDimensions: { cols: undefined, rows: undefined },
Expand All @@ -27,6 +26,10 @@ export class LocalPty extends Disposable implements ITerminalChildProcess {
failedShellIntegrationActivation: false,
usedShellIntegrationInjection: undefined
};
private readonly _lastDimensions: { cols: number; rows: number } = { cols: -1, rows: -1 };

private _inReplay = false;

private readonly _onProcessData = this._register(new Emitter<IProcessDataEvent | string>());
readonly onProcessData = this._onProcessData.event;
private readonly _onProcessReplay = this._register(new Emitter<IPtyHostProcessReplayEvent>());
Expand Down Expand Up @@ -70,9 +73,11 @@ export class LocalPty extends Disposable implements ITerminalChildProcess {
this._localPtyService.input(this.id, data);
}
resize(cols: number, rows: number): void {
if (this._inReplay) {
if (this._inReplay || this._lastDimensions.cols === cols && this._lastDimensions.rows === rows) {
return;
}
this._lastDimensions.cols = cols;
this._lastDimensions.rows = rows;
this._localPtyService.resize(this.id, cols, rows);
}
async clearBuffer(): Promise<void> {
Expand Down
2 changes: 2 additions & 0 deletions src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { mark } from 'vs/base/common/performance';
import { ILifecycleService, LifecyclePhase } from 'vs/workbench/services/lifecycle/common/lifecycle';
import { DeferredPromise } from 'vs/base/common/async';
import { IStatusbarService } from 'vs/workbench/services/statusbar/browser/statusbar';
import { memoize } from 'vs/base/common/decorators';

export class LocalTerminalBackendContribution implements IWorkbenchContribution {
constructor(
Expand Down Expand Up @@ -229,6 +230,7 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke
return this._localPtyService.getProfiles?.(this._workspaceContextService.getWorkspace().id, profiles, defaultProfile, includeDetectedProfiles) || [];
}

@memoize
async getEnvironment(): Promise<IProcessEnvironment> {
return this._proxy.getEnvironment();
}
Expand Down