Skip to content

Commit 1ca31fd

Browse files
authored
perf(cloud9): "Upload Lambda" is slow since Toolkit 2.x #4443
Problem: In Cloud9, using the "Upload Lambda" action on even a small directory, takes 5+ seconds. Example trace: 2024-02-16 15:45:44 [DEBUG]: createRegionPrompter: selected { id: 'us-west-2', name: 'US West (Oregon)' } 2024-02-16 15:45:44 [DEBUG]: findApplicationJsonFile (cloud9: true) 2024-02-16 15:45:44 [DEBUG]: cloud9Findfile: /home/ec2-user/environment/ 2024-02-16 15:45:46 [DEBUG]: cloud9Findfile: /home/ec2-user/environment/.c9 2024-02-16 15:45:49 [DEBUG]: cloud9Findfile: /home/ec2-user/environment/.c9/amazonwebservices.aws-toolkit-vscode 2024-02-16 15:45:50 [DEBUG]: cloud9Findfile: /home/ec2-user/environment/.c9/metadata 2024-02-16 15:45:52 [DEBUG]: cloud9Findfile: /home/ec2-user/environment/.c9/test 4 2024-02-16 15:45:52 [DEBUG]: uploadLambda: .application.json not found in: "/home/ec2-user/environment/" Solution: The vscode `stat()` API is very slow, especially on the Cloud9 VFS. Avoid it by special-casing the implementation of `fsCommon.exists()` on Cloud9. 2024-02-16 16:12:14 [DEBUG]: createRegionPrompter: selected { id: 'us-west-2', name: 'US West (Oregon)' } 2024-02-16 16:12:14 [DEBUG]: findApplicationJsonFile (cloud9: true) 2024-02-16 16:12:14 [DEBUG]: cloud9Findfile: /home/ec2-user/environment/ 2024-02-16 16:12:14 [DEBUG]: cloud9Findfile: /home/ec2-user/environment/.c9 2024-02-16 16:12:14 [DEBUG]: cloud9Findfile: /home/ec2-user/environment/.c9/amazonwebservices.aws-toolkit-vscode 2024-02-16 16:12:14 [DEBUG]: cloud9Findfile: /home/ec2-user/environment/.c9/metadata 2024-02-16 16:12:14 [DEBUG]: cloud9Findfile: /home/ec2-user/environment/.c9/test 4 2024-02-16 16:12:14 [DEBUG]: uploadLambda: .application.json not found in: "/home/ec2-user/environment/" TODO: the following functions which loop through `readdir` results are also potentially slow: getDirSize cleanLogFiles
1 parent 5faec4a commit 1ca31fd

24 files changed

+173
-102
lines changed

docs/ARCHITECTURE.md

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,14 @@ Abstractly, a 'wizard' is a collection of discrete, linear steps (subroutines),
337337

338338
### Creating a Wizard (Quick Picks)
339339

340-
A new wizard can be created by extending off the base `Wizard` class, using the template type to specify the shape of the wizard state. All wizards have an internal 'form' property that is used to assign steps. We can assign UI elements (namely, quick picks) using the `bindPrompter` method on form elements. This method accepts a callback that should return a `Prompter` given the current state. For this example, we will use `createQuickPick` and `createInputBox` for our prompters:
340+
Create a new wizard by extending the base `Wizard` class, using the template type to specify the
341+
shape of the wizard state. All wizards have an internal `form` property that is used to assign
342+
steps. You can assign UI elements (namely, quickpicks) using the `bindPrompter` method on form
343+
elements. This method accepts a callback that should return a `Prompter` given the current state.
344+
For this example, we will use `createQuickPick` and `createInputBox` for our prompters:
345+
346+
If you need to call async functions to construct your `Wizard` subclass, define your init logic in
347+
the `init()` method instead of the constructor.
341348

342349
```ts
343350
interface ExampleState {
@@ -382,7 +389,7 @@ Note that all wizards can potentially return `undefined` if the workflow was can
382389
Use `createWizardTester` on an instance of a wizard. Tests can then be constructed by asserting both the user-defined and internal state. Using the above `ExampleWizard`:
383390

384391
```ts
385-
const tester = createWizardTester(new ExampleWizard())
392+
const tester = await createWizardTester(new ExampleWizard())
386393
tester.foo.assertShowFirst() // Fails if `foo` is not shown (or not shown first)
387394
tester.bar.assertDoesNotShow() // True since `foo` is not assigned an explicit value
388395
tester.foo.applyInput('Hello, world!') // Manipulate 'user' state

packages/toolkit/src/lambda/commands/uploadLambda.ts

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,10 @@ import * as nls from 'vscode-nls'
99
const localize = nls.loadMessageBundle()
1010

1111
import AdmZip from 'adm-zip'
12-
import * as fs from 'fs-extra'
1312
import * as path from 'path'
13+
import { fsCommon } from '../../srcShared/fs'
1414
import { showConfirmationMessage, showViewLogsMessage } from '../../shared/utilities/messages'
15-
import {
16-
fileExists,
17-
cloud9Findfile,
18-
makeTemporaryToolkitFolder,
19-
tryRemoveFolder,
20-
} from '../../shared/filesystemUtilities'
15+
import { cloud9Findfile, makeTemporaryToolkitFolder, tryRemoveFolder } from '../../shared/filesystemUtilities'
2116
import * as localizedText from '../../shared/localizedText'
2217
import { getLogger } from '../../shared/logger'
2318
import { SamCliBuildInvocation } from '../../shared/sam/cli/samCliBuild'
@@ -209,16 +204,15 @@ export interface UploadLambdaWizardState {
209204
}
210205

211206
export class UploadLambdaWizard extends Wizard<UploadLambdaWizardState> {
212-
constructor(lambda?: LambdaFunction, invokePath?: vscode.Uri) {
213-
super({ initState: { lambda } })
207+
public override async init(): Promise<this> {
214208
this.form.lambda.region.bindPrompter(() => createRegionPrompter().transform(region => region.id))
215209

216-
if (invokePath) {
210+
if (this.invokePath) {
217211
this.form.uploadType.setDefault('directory')
218-
if (fs.statSync(invokePath.fsPath).isFile()) {
219-
this.form.targetUri.setDefault(vscode.Uri.file(path.dirname(invokePath.fsPath)))
212+
if (await fsCommon.existsFile(this.invokePath.fsPath)) {
213+
this.form.targetUri.setDefault(vscode.Uri.file(path.dirname(this.invokePath.fsPath)))
220214
} else {
221-
this.form.targetUri.setDefault(invokePath)
215+
this.form.targetUri.setDefault(this.invokePath)
222216
}
223217
} else {
224218
this.form.uploadType.bindPrompter(() => createUploadTypePrompter())
@@ -242,7 +236,7 @@ export class UploadLambdaWizard extends Wizard<UploadLambdaWizardState> {
242236

243237
this.form.lambda.name.bindPrompter(state => {
244238
// invoking from the command palette passes no arguments
245-
if (!invokePath) {
239+
if (!this.invokePath) {
246240
if (state.uploadType === 'directory') {
247241
return createFunctionNamePrompter(state.lambda!.region!, state.targetUri)
248242
} else {
@@ -253,10 +247,10 @@ export class UploadLambdaWizard extends Wizard<UploadLambdaWizardState> {
253247
)
254248
}
255249
} else {
256-
return createFunctionNamePrompter(state.lambda!.region!, invokePath)
250+
return createFunctionNamePrompter(state.lambda!.region!, this.invokePath)
257251
}
258252
})
259-
if (lambda) {
253+
if (this.lambda) {
260254
this.form.directoryBuildType.bindPrompter(() => createBuildPrompter(), {
261255
showWhen: ({ uploadType }) => uploadType === 'directory',
262256
})
@@ -265,6 +259,12 @@ export class UploadLambdaWizard extends Wizard<UploadLambdaWizardState> {
265259
}
266260

267261
this.form.confirmedDeploy.bindPrompter(state => createConfirmDeploymentPrompter(state.lambda!))
262+
263+
return this
264+
}
265+
266+
constructor(readonly lambda?: LambdaFunction, readonly invokePath?: vscode.Uri) {
267+
super({ initState: { lambda } })
268268
}
269269
}
270270

@@ -304,7 +304,7 @@ async function runUploadLambdaWithSamBuild(lambda: Required<LambdaFunction>, par
304304
// Detect if handler is present and provide strong guidance against proceeding if not.
305305
try {
306306
const handlerFile = path.join(parentDir.fsPath, getLambdaDetails(lambda.configuration).fileName)
307-
if (!(await fileExists(handlerFile))) {
307+
if (!(await fsCommon.exists(handlerFile))) {
308308
const isConfirmed = await showConfirmationMessage({
309309
prompt: localize(
310310
'AWS.lambda.upload.handlerNotFound',
@@ -416,7 +416,7 @@ async function runUploadLambdaZipFile(lambda: LambdaFunction, zipFileUri: vscode
416416
cancellable: false,
417417
},
418418
async progress => {
419-
const zipFile = await fs.readFile(zipFileUri.fsPath).catch(err => {
419+
const zipFile = await fsCommon.readFile(zipFileUri.fsPath).catch(err => {
420420
throw new ToolkitError('Failed to read zip', { cause: err })
421421
})
422422
return await uploadZipBuffer(lambda, zipFile, progress)
@@ -459,7 +459,7 @@ async function zipAndUploadDirectory(
459459
*/
460460
async function uploadZipBuffer(
461461
lambda: LambdaFunction,
462-
zip: Buffer,
462+
zip: Uint8Array,
463463
progress: vscode.Progress<{
464464
message?: string | undefined
465465
increment?: number | undefined
@@ -482,14 +482,14 @@ export async function findApplicationJsonFile(
482482
startPath: vscode.Uri,
483483
cloud9 = isCloud9()
484484
): Promise<vscode.Uri | undefined> {
485-
if (!(await fileExists(startPath.fsPath))) {
485+
if (!(await fsCommon.exists(startPath.fsPath))) {
486486
getLogger().error(
487487
'findApplicationJsonFile() invalid path (not accessible or does not exist): "%s"',
488488
startPath.fsPath
489489
)
490490
return undefined
491491
}
492-
const isdir = fs.statSync(startPath.fsPath).isDirectory()
492+
const isdir = await fsCommon.existsDir(startPath.fsPath)
493493
const parentDir = isdir ? startPath.fsPath : path.dirname(startPath.fsPath)
494494
const found = cloud9
495495
? await cloud9Findfile(parentDir, '.application.json')
@@ -508,10 +508,10 @@ export async function findApplicationJsonFile(
508508
return found[0]
509509
}
510510

511-
export function getFunctionNames(file: vscode.Uri, region: string): string[] | undefined {
511+
export async function getFunctionNames(file: vscode.Uri, region: string): Promise<string[] | undefined> {
512512
try {
513513
const names: string[] = []
514-
const appData = JSON.parse(fs.readFileSync(file.fsPath, { encoding: 'utf-8' }).toString())
514+
const appData = JSON.parse(await fsCommon.readFileAsString(file.fsPath))
515515
if (appData['Functions']) {
516516
const functions = Object.keys(appData['Functions'])
517517
if (functions) {
@@ -536,7 +536,7 @@ async function listAllLambdaNames(region: string, path?: vscode.Uri) {
536536
// Get Lambda functions from .application.json #2588
537537
if (path) {
538538
const appFile = await findApplicationJsonFile(path)
539-
const namesFromAppFile = appFile ? getFunctionNames(appFile, region) : undefined
539+
const namesFromAppFile = appFile ? await getFunctionNames(appFile, region) : undefined
540540
if (!appFile) {
541541
getLogger().debug('lambda: .application.json not found')
542542
} else if (!namesFromAppFile) {

packages/toolkit/src/shared/clients/lambdaClient.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ export class DefaultLambdaClient {
9898
}
9999
}
100100

101-
public async updateFunctionCode(name: string, zipFile: Buffer): Promise<Lambda.FunctionConfiguration> {
101+
public async updateFunctionCode(name: string, zipFile: Uint8Array): Promise<Lambda.FunctionConfiguration> {
102102
getLogger().debug(`updateFunctionCode called for function: ${name}`)
103103
const client = await this.createSdkClient()
104104

packages/toolkit/src/shared/filesystemUtilities.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ export async function getDirSize(
4242
return getDirSize(filePath, startTime, duration, fileExt)
4343
}
4444
if (type === vscode.FileType.File && fileName.endsWith(fileExt)) {
45+
// TODO: This is SLOW on Cloud9.
4546
const stat = await fsCommon.stat(filePath)
4647
return stat.size
4748
}
@@ -271,6 +272,7 @@ export async function hasFileWithSuffix(dir: string, suffix: string, exclude?: v
271272
* @returns List of one or zero Uris (for compat with vscode.workspace.findFiles())
272273
*/
273274
export async function cloud9Findfile(dir: string, fileName: string): Promise<vscode.Uri[]> {
275+
getLogger().debug('cloud9Findfile: %s', dir)
274276
const files = await fsCommon.readdir(dir)
275277
const subDirs: vscode.Uri[] = []
276278
for (const file of files) {

packages/toolkit/src/shared/wizards/wizard.ts

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ type StepCache = { picked?: any; stepOffset?: [number, number] } & { [key: strin
6464
export type StateWithCache<TState, TProp> = TState & { stepCache: StepCache; estimator: StepEstimator<TProp> }
6565

6666
export interface WizardOptions<TState> {
67+
/** Optional {@link WizardForm} provided by the caller instead of the default. */
6768
readonly initForm?: WizardForm<TState>
6869
readonly initState?: Partial<TState>
6970
/** Provides a way to apply inputs to Prompters as if the user has already responded */
@@ -75,13 +76,26 @@ export interface WizardOptions<TState> {
7576
* A generic wizard that consumes data from a series of {@link Prompter prompters}. The 'form' public property
7677
* exposes functionality to add prompters to the wizard with optional context, utilizing the {@link WizardForm}
7778
* class. Wizards will modify a single property of their internal state with each prompt.
79+
*
80+
* Subclasses can override {@link init} if an "async constructor" is needed.
7881
*/
7982
export class Wizard<TState extends Partial<Record<keyof TState, unknown>>> {
80-
protected readonly _form: WizardForm<TState>
83+
protected readonly __form: WizardForm<TState>
8184
protected readonly boundSteps: Map<string, StepFunction<TState>> = new Map()
8285
protected stateController: StateMachineController<TState>
8386
private _stepOffset: [number, number] = [0, 0]
8487
private _exitStep?: StepFunction<TState>
88+
/** Guards against accidental use of the Wizard before `init()`. */
89+
private _ready: boolean
90+
91+
/** Checks that `init()` was performed (if it was defined). */
92+
private assertReady() {
93+
// Check for `false` explicity so that the base-class constructor can access `this._form`.
94+
// We want to guard against confusion when implementing a subclass, not this base-class.
95+
if (this._ready === false && this.init) {
96+
throw Error('run() (or init()) must be called immediately after creating the Wizard')
97+
}
98+
}
8599

86100
/**
87101
* The offset is applied to both the current step and total number of steps. Useful if the wizard is
@@ -97,6 +111,11 @@ export class Wizard<TState extends Partial<Record<keyof TState, unknown>>> {
97111
return this._stepOffset[1] + this.stateController.totalSteps
98112
}
99113

114+
public get _form() {
115+
this.assertReady()
116+
return this.__form
117+
}
118+
100119
public get form() {
101120
return this._form.body
102121
}
@@ -117,11 +136,21 @@ export class Wizard<TState extends Partial<Record<keyof TState, unknown>>> {
117136

118137
public constructor(private readonly options: WizardOptions<TState> = {}) {
119138
this.stateController = new StateMachineController(options.initState as TState)
120-
this._form = options.initForm ?? new WizardForm()
139+
this.__form = options.initForm ?? new WizardForm()
121140
this._exitStep =
122141
options.exitPrompterProvider !== undefined ? this.createExitStep(options.exitPrompterProvider) : undefined
142+
143+
// Subclass constructor logic should live in `init()`, if it exists.
144+
this._ready = !this.init
123145
}
124146

147+
/**
148+
* Optional, one-time, asynchronous setup for subclasses that need an "async constructor". The
149+
* subclass must put all of its constructor logic here, instead of its normal constructor.
150+
* Called by `run()` exactly once.
151+
*/
152+
public async init?(): Promise<this>
153+
125154
private assignSteps(): void {
126155
this._form.properties.forEach(prop => {
127156
const provider = this._form.getPrompterProvider(prop)
@@ -132,6 +161,12 @@ export class Wizard<TState extends Partial<Record<keyof TState, unknown>>> {
132161
}
133162

134163
public async run(): Promise<TState | undefined> {
164+
if (!this._ready && this.init) {
165+
this._ready = true // Let init() use `this._form`.
166+
await this.init()
167+
delete this.init
168+
}
169+
135170
this.assignSteps()
136171
this.resolveNextSteps((this.options.initState ?? {}) as TState).forEach(step =>
137172
this.stateController.addStep(step)

packages/toolkit/src/srcShared/fs.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,25 @@ export class FileSystemCommon {
8282

8383
async exists(path: Uri | string, fileType?: vscode.FileType): Promise<boolean> {
8484
path = FileSystemCommon.getUri(path)
85+
86+
if (isCloud9()) {
87+
// vscode.workspace.fs.stat() is SLOW. Avoid it on Cloud9.
88+
try {
89+
const stat = await fsPromises.stat(path.fsPath)
90+
// Note: comparison is bitwise (&) because `FileType` enum is bitwise.
91+
// See vscode.FileType docstring.
92+
if (fileType === undefined || fileType & vscode.FileType.Unknown) {
93+
return true
94+
} else if (fileType & vscode.FileType.Directory) {
95+
return stat.isDirectory()
96+
} else if (fileType & vscode.FileType.File) {
97+
return stat.isFile()
98+
}
99+
} catch {
100+
return false
101+
}
102+
}
103+
85104
try {
86105
const stat = await this.stat(path)
87106
// check filetype if it was given

packages/toolkit/src/test/apprunner/wizards/codeRepositoryWizard.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,10 @@ describe('AppRunnerCodeRepositoryWizard', function () {
2222
let tester: WizardTester<AppRunner.SourceConfiguration>
2323
let repoTester: WizardTester<AppRunner.CodeRepository>
2424

25-
beforeEach(function () {
25+
beforeEach(async function () {
2626
// apprunner client and git api will never be called
2727
const wizard = new AppRunnerCodeRepositoryWizard({} as any, {} as any)
28-
tester = createWizardTester(wizard)
28+
tester = await createWizardTester(wizard)
2929
repoTester = tester.CodeRepository
3030
})
3131

packages/toolkit/src/test/apprunner/wizards/createServiceWizard.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import { DefaultAppRunnerClient } from '../../../shared/clients/apprunnerClient'
1414
describe('CreateServiceWizard', function () {
1515
let tester: WizardTester<AppRunner.CreateServiceRequest>
1616

17-
beforeEach(function () {
17+
beforeEach(async function () {
1818
const regionCode = 'us-east-1'
1919
const wizard = new CreateAppRunnerServiceWizard(
2020
regionCode,
@@ -27,7 +27,7 @@ describe('CreateServiceWizard', function () {
2727
}
2828
)
2929

30-
tester = createWizardTester(wizard)
30+
tester = await createWizardTester(wizard)
3131
})
3232

3333
describe('CreateAppRunnerServiceWizard', function () {

packages/toolkit/src/test/apprunner/wizards/imageRepositoryWizard.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,9 @@ describe('AppRunnerImageRepositoryWizard', function () {
1515
let tester: WizardTester<AppRunner.SourceConfiguration>
1616
let repoTester: WizardTester<AppRunner.ImageRepository>
1717

18-
beforeEach(function () {
18+
beforeEach(async function () {
1919
const wizard = new AppRunnerImageRepositoryWizard({} as any, {} as any) // the clients will never be called
20-
tester = createWizardTester(wizard)
20+
tester = await createWizardTester(wizard)
2121
repoTester = tester.ImageRepository
2222
})
2323

@@ -52,9 +52,9 @@ describe('AppRunnerImageRepositoryWizard', function () {
5252
describe('ImageIdentifierForm', function () {
5353
let tester: WizardTester<{ repo: TaggedEcrRepository }>
5454

55-
beforeEach(function () {
55+
beforeEach(async function () {
5656
const form = new ImageIdentifierForm({} as any) // ecr will never be called
57-
tester = createWizardTester(form)
57+
tester = await createWizardTester(form)
5858
})
5959

6060
it('asks for tag if not provided', function () {

packages/toolkit/src/test/cloudWatchLogs/commands/searchLogGroup.test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,15 @@ import { createWizardTester } from '../../shared/wizards/wizardTestUtils'
1313

1414
describe('searchLogGroup', async function () {
1515
describe('Wizard', async function () {
16-
it('shows steps in correct order', function () {
17-
const testWizard = createWizardTester(new SearchLogGroupWizard())
16+
it('shows steps in correct order', async function () {
17+
const testWizard = await createWizardTester(new SearchLogGroupWizard())
1818
testWizard.submenuResponse.assertShowFirst()
1919
testWizard.timeRange.assertShowSecond()
2020
testWizard.filterPattern.assertShowThird()
2121
})
2222

2323
it('skips steps if parameters are given', async function () {
24-
const nodeTestWizard = createWizardTester(
24+
const nodeTestWizard = await createWizardTester(
2525
new SearchLogGroupWizard({
2626
groupName: 'group-test',
2727
regionName: 'region-test',

0 commit comments

Comments
 (0)