feat(aws-cdk-lib): reduce load time of JavaScript library #27217

rix0rrr · 2023-09-20T12:04:59Z

This change halves the load time of require('aws-cdk-lib'), from ~600ms to ~300ms on my machine. It also shaves ~8min off running all integ tests (again, on my machine).

It does this by making all loading of JavaScript modules lazy, so they are only loaded just before any symbols from them are actually accessed (instead of loading all modules at startup). This new automatic module replaces the previous manual effort to do something similar, in lazy-index.ts.

Note that this transformation (unfortunately) does not affect TypeScript compilation times: the TypeScript compiler is still going to read all .d.ts files. To improve interactive TypeScript performance, use swc or ts-node with type checking disabled. Fully type checking your application using tsc will take the amount of time it takes.

What's the big idea?

Turning this:

const x = require("x");
module.exports.banana = function() {
  return x.hello();
}

Into this:

var x = () => { var tmp = require("x"); x = () => tmp; return tmp; };
module.exports.banana = function () {
    return x().hello();
};

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

This change halves the load time of `require('aws-cdk-lib')`, from ~600ms to ~300ms on my machine. It also shaves ~8min off running all integ tests (again, on my machine). It does this by making all loading of JavaScript modules lazy, so they are only loaded just before any symbols from them are actually accessed (instead of loading all modules at startup).

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

…huijbers/lazy-requires

otaviomacedo · 2023-09-22T09:14:02Z

tools/@aws-cdk/lazify/lib/index.ts

+            || ts.isSetAccessor(node.parent) // class X { set ident() { ... } }
+            || ts.isSetAccessorDeclaration(node.parent) // interface X { set ident: string }
+          );
+          // We should also ignore this identifier if it is shadowed


So why are we not doing it?

otaviomacedo · 2023-09-22T09:23:34Z

tools/@aws-cdk/lazify/lib/index.ts

+                // tmp = require(...)
+                createVariable(factory, 'tmp', factory.createCallExpression(factory.createIdentifier('require'), [], [factory.createStringLiteral(moduleName)])),
+
+                // <this_fn> = () => tmp


Consider commenting on why we need this. My naive expectation would be to have something like:

var x = () => { return require("x"); };

without any need for a temporary variable.

packages/aws-cdk-lib/aws-lambda/test/code.test.ts

mergify · 2023-09-22T14:33:37Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-09-22T15:02:30Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 6d2237f
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-09-22T15:02:52Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

This is a follow-up to #27217 The check we needed to perform was inverted 😖 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…7217)" This reverts commit faffce0.

This change halves the load time of `require('aws-cdk-lib')`, from ~600ms to ~300ms on my machine. It also shaves ~8min off running all integ tests (again, on my machine). It does this by making all loading of JavaScript modules lazy, so they are only loaded just before any symbols from them are actually accessed (instead of loading all modules at startup). This new automatic module replaces the previous manual effort to do something similar, in `lazy-index.ts`. Note that this transformation (unfortunately) does not affect TypeScript compilation times: the TypeScript compiler is still going to read all `.d.ts` files. To improve interactive TypeScript performance, use `swc` or `ts-node` with type checking disabled. Fully type checking your application using `tsc` will take the amount of time it takes. ## What's the big idea? Turning this: ```js const x = require("x"); module.exports.banana = function() { return x.hello(); } ``` Into this: ```js var x = () => { var tmp = require("x"); x = () => tmp; return tmp; }; module.exports.banana = function () { return x().hello(); }; ``` ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

This is a follow-up to #27217 The check we needed to perform was inverted 😖 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

This change halves the load time of `require('aws-cdk-lib')`, from ~600ms to ~300ms on my machine. It also shaves ~8min off running all integ tests (again, on my machine). It does this by making all loading of JavaScript modules lazy, so they are only loaded just before any symbols from them are actually accessed (instead of loading all modules at startup). This new automatic module replaces the previous manual effort to do something similar, in `lazy-index.ts`. Note that this transformation (unfortunately) does not affect TypeScript compilation times: the TypeScript compiler is still going to read all `.d.ts` files. To improve interactive TypeScript performance, use `swc` or `ts-node` with type checking disabled. Fully type checking your application using `tsc` will take the amount of time it takes. Turning this: ```js const x = require("x"); module.exports.banana = function() { return x.hello(); } ``` Into this: ```js var x = () => { var tmp = require("x"); x = () => tmp; return tmp; }; module.exports.banana = function () { return x().hello(); }; ```

This is a re-roll of #27217 which had to be reverted (first attempt to fix forward in #27314, then reverted in #27353). The issue with the previous change were: - The mutation was accidentally switched off at the last second. - It did not work for ESM modules, because the code were were generating was not recognized as a CommonJS export by `cjs-module-lexer`. We now generate code that passes the `cjs-module-lexer`, and have tests in place to prove it. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added the p2 label Sep 20, 2023

aws-cdk-automation requested a review from a team September 20, 2023 12:05

mergify bot added the contribution/core This is a PR that came from AWS. label Sep 20, 2023

aws-cdk-automation previously requested changes Sep 20, 2023

View reviewed changes

Also replace lazy-index

a404b08

rix0rrr added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Sep 20, 2023

rix0rrr added 15 commits September 20, 2023 14:30

Add to workspaces

2a4c0a5

Remove accidentally checked-in files

7fe5bd2

Remove moved file

089d1be

Merge remote-tracking branch 'origin/main' into huijbers/lazy-requires

d5474d7

Compile bin as well

cedaae8

Merge branch 'main' into huijbers/lazy-requires

36ab138

Omg this config

c30313d

Merge branch 'huijbers/lazy-requires' of github.com:aws/aws-cdk into …

b9dd665

…huijbers/lazy-requires

Quiet by default

4a2da79

Add an ESM integ test

2cb1270

yarnlock

26f755c

Linterrr

bb0dfee

Exclude custom resources

807f2c3

Get rid of hash

6f4a2e7

Ignore .is_custom_resource everywhere

c3e8d83

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 21, 2023

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Sep 21, 2023

otaviomacedo approved these changes Sep 22, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 22, 2023

Clarify

e37f94e

github-advanced-security bot found potential problems Sep 22, 2023

View reviewed changes

packages/aws-cdk-lib/aws-lambda/test/code.test.ts Fixed Show fixed Hide fixed

Update code.test.ts

641a702

rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Sep 22, 2023

Merge branch 'main' into huijbers/lazy-requires

6d2237f

mergify bot merged commit faffce0 into main Sep 22, 2023
14 checks passed

mergify bot deleted the huijbers/lazy-requires branch September 22, 2023 15:02

rix0rrr mentioned this pull request Sep 27, 2023

fix: load time speed has regressed #27314

Merged

rix0rrr added a commit that referenced this pull request Sep 29, 2023

Revert "feat(aws-cdk-lib): reduce load time of JavaScript library (#2…

17dca57

…7217)" This reverts commit faffce0.

rix0rrr mentioned this pull request Sep 30, 2023

feat(aws-cdk-lib): reduce JavaScript load time, second attempt #27362

Merged

github-actions bot mentioned this pull request Oct 1, 2023

Monthly PRs metrics report - Sep 2023 #27368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-cdk-lib): reduce load time of JavaScript library #27217

feat(aws-cdk-lib): reduce load time of JavaScript library #27217

rix0rrr commented Sep 20, 2023 •

edited

aws-cdk-automation left a comment

otaviomacedo Sep 22, 2023

otaviomacedo Sep 22, 2023

mergify bot commented Sep 22, 2023

aws-cdk-automation commented Sep 22, 2023

mergify bot commented Sep 22, 2023

feat(aws-cdk-lib): reduce load time of JavaScript library #27217

feat(aws-cdk-lib): reduce load time of JavaScript library #27217

Conversation

rix0rrr commented Sep 20, 2023 • edited

What's the big idea?

aws-cdk-automation left a comment

Choose a reason for hiding this comment

otaviomacedo Sep 22, 2023

Choose a reason for hiding this comment

otaviomacedo Sep 22, 2023

Choose a reason for hiding this comment

mergify bot commented Sep 22, 2023

aws-cdk-automation commented Sep 22, 2023

AWS CodeBuild CI Report

mergify bot commented Sep 22, 2023

rix0rrr commented Sep 20, 2023 •

edited