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

fix: importing aws-cdk-lib is slow #23813

Merged
merged 5 commits into from
Jan 25, 2023

Conversation

mrgrain
Copy link
Contributor

@mrgrain mrgrain commented Jan 24, 2023

Fixes slow import of top-level aws-cdk-lib.

At the very end of the build, the compiled index.js file is replaced with a version optimized for import performance.
Instead of loading submodules directly at the top level, exports are defined as getter functions.
This way they will only be required when actually imported from aws-cdk-lib.
Improves AWS CDK app performance by ~400ms.

Background reading: https://medium.com/trabe/exporting-getters-in-commonjs-modules-dd8f98b7d85e


$ hyperfine -i "node -e \"const { App } = require('aws-cdk-lib');\"" "node -e \"const { App } = require('aws-cdk-lib-candidate');\"" --warmup 5
Benchmark 1: node -e "const { App } = require('aws-cdk-lib');"
  Time (mean ± σ):      1.227 s ±  0.125 s    [User: 1.102 s, System: 0.262 s]
  Range (min … max):    1.110 s …  1.508 s    10 runs
 
Benchmark 2: node -e "const { App } = require('aws-cdk-lib-candidate');"
  Time (mean ± σ):     251.9 ms ±  19.7 ms    [User: 226.6 ms, System: 51.0 ms]
  Range (min … max):   224.6 ms … 278.3 ms    10 runs
 
Summary
  'node -e "const { App } = require('aws-cdk-lib-candidate');"' ran
    4.87 ± 0.62 times faster than 'node -e "const { App } = require('aws-cdk-lib');"'

All Submissions:

Adding new Construct Runtime Dependencies:

  • This PR adds new construct runtime dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
mrgrain Momo Kornher
@gitpod-io
Copy link

gitpod-io bot commented Jan 24, 2023

@aws-cdk-automation aws-cdk-automation requested a review from a team January 24, 2023 12:49
@github-actions github-actions bot added the p2 label Jan 24, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jan 24, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

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.

@mrgrain mrgrain changed the title fix(aws-cdk-lib): importing of aws-cdk-lib is slow fix: importing of aws-cdk-lib is slow Jan 24, 2023
@mrgrain mrgrain changed the title fix: importing of aws-cdk-lib is slow fix: importing aws-cdk-lib is slow Jan 24, 2023
@mrgrain mrgrain added pr-linter/exempt-test The PR linter will not require test changes pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Jan 24, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review January 24, 2023 12:56

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

@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 24, 2023

Exempted tests and integ-tests as no change and passing tests is a sign of this working as expected.

@mrgrain mrgrain added the pr/do-not-merge This PR should not be merged at this time. label Jan 24, 2023
@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 24, 2023

Running a full build as f6257a3f-2a84-43c9-bdc5-5388adaa12b6

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Lovely hack-fix! 😍

mrgrain and others added 2 commits January 24, 2023 13:18

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was signed with the committer’s verified signature.
mrgrain Momo Kornher
@mrgrain mrgrain marked this pull request as ready for review January 24, 2023 18:41
@mrgrain mrgrain removed the pr/do-not-merge This PR should not be merged at this time. label Jan 24, 2023
@mergify
Copy link
Contributor

mergify bot commented Jan 24, 2023

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).

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@mrgrain mrgrain added the pr/do-not-merge This PR should not be merged at this time. label Jan 24, 2023
@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 24, 2023

Leaving the do not merge label on until he have decided if this is a good idea.

@rix0rrr rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Jan 25, 2023
@mergify
Copy link
Contributor

mergify bot commented Jan 25, 2023

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).

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 8aaeffb into aws:main Jan 25, 2023
@mergify
Copy link
Contributor

mergify bot commented Jan 25, 2023

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).

@mrgrain mrgrain deleted the mrgrain/fix/top-level-import-speed branch January 25, 2023 11:31
@mikolaj-w-provectusalgae
Copy link

mikolaj-w-provectusalgae commented Jan 26, 2023

Is there a chance this PR has broken an es6 style import?

import { aws_cognito as cognito, aws_ssm as ssm } from 'aws-cdk-lib';
         ^^^^^^^^^^^
SyntaxError: Named export 'aws_cognito' not found. The requested module 'aws-cdk-lib' is a CommonJS module, which may not support all module.exports as named exports.

rix0rrr added a commit that referenced this pull request Jan 26, 2023
PR #23813 made imports lazy, but in the resulting code, Nodejs no longer
recognizes the exports when importing `aws-cdk-lib` from an ESM module.

Apply a transformation on the source that makes Nodejs detect the
exports again.
@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 26, 2023

Thanks @mikolaj-w-provectusalgae We are working on a fix.

mergify bot pushed a commit that referenced this pull request Jan 26, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
PR #23813 made imports lazy, but in the resulting code, Nodejs no longer recognizes the exports when importing `aws-cdk-lib` from an ESM module.

To solve this, vend two different index files: one for use by CJS imports, one for use by ESM imports.

ESM modules will still try to load the entire library, so they don't benefit from the speed boost. This is unavoidable: we tried a more complex method that forced ESM to recognize the lazy module references anyway (by tricking the backwards compatibility lexer), but ESM did not experience a speed boost, indicating that it was crawling the entire module irrespective of the submodule accessor's laziness. So, we are opting for the simpler solution of vending two index files instead.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mrgrain pushed a commit that referenced this pull request Jan 26, 2023

Verified

This commit was signed with the committer’s verified signature.
mrgrain Momo Kornher
PR #23813 made imports lazy, but in the resulting code, Nodejs no longer recognizes the exports when importing `aws-cdk-lib` from an ESM module.

To solve this, vend two different index files: one for use by CJS imports, one for use by ESM imports.

ESM modules will still try to load the entire library, so they don't benefit from the speed boost. This is unavoidable: we tried a more complex method that forced ESM to recognize the lazy module references anyway (by tricking the backwards compatibility lexer), but ESM did not experience a speed boost, indicating that it was crawling the entire module irrespective of the submodule accessor's laziness. So, we are opting for the simpler solution of vending two index files instead.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 26, 2023

@mikolaj-w-provectusalgae Can you confirm if v2.62.1 fixes this for you?

@mikolaj-w-provectusalgae
Copy link

mikolaj-w-provectusalgae commented Jan 26, 2023

It is partially fixed. The project can be synthesized and deployed, but the tests fail. I'll do some digging in Jest's configuration and see what's wrong.

Test suite failed to run

    lib/index.ts:3:5 - error TS2305: Module '"../node_modules/aws-cdk-lib/lazy-index.js"' has no exported member 'aws_ec2'.

    3     aws_ec2 as ec2,
          ~~~~~~~
    lib/index.ts:4:5 - error TS2305: Module '"../node_modules/aws-cdk-lib/lazy-index.js"' has no exported member 'aws_iam'.

    4     aws_iam as iam,
          ~~~~~~~
    lib/index.ts:5:5 - error TS2305: Module '"../node_modules/aws-cdk-lib/lazy-index.js"' has no exported member 'aws_lambda'.

    5     aws_lambda as lambda,
    
    ...

Jest config

{
    preset: 'ts-jest/presets/default-esm', // or other ESM presets
    testEnvironment: 'node',
    roots: ['<rootDir>/__tests__'],
    testMatch: ['**/*.test.ts'],
    moduleNameMapper: {
        '^(\\.{1,2}/.*)\\.js$': '$1',
    },
    transform: {
        '^.+\\.tsx?$': [
            'ts-jest',
            {
                useESM: true,
            },
        ],
    },
};

@mikolaj-w-provectusalgae

I guess lazy-index.d.ts is the problem. index.d.ts with all exports should be used.

@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 26, 2023

Makes sense. Could you try adding a types entry to node_modules/aws-cdk-lib/package.json in your project?

Relevant section would then look like this:

{
  // ...
  "exports": {
    ".": {
      "types": "./index.d.ts",   // <---- this is new, needs to be first entry
      "import": "./index.js",
      "require": "./lazy-index.js"
    },
    // ...
}

Reference: https://www.typescriptlang.org/docs/handbook/esm-node.html#packagejson-exports-imports-and-self-referencing

@mikolaj-w-provectusalgae

Yep. That's it! test, synth and deployment pass.

@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 26, 2023

Sweet! Thanks for helping out. We'll get the fix out asap.

@mikolaj-w-provectusalgae

Sweet! Thanks for helping out. We'll get the fix out asap.

Thanks for your quick response!

@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 27, 2023

@mikolaj-w-provectusalgae v2.62.2 is released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-test The PR linter will not require test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants