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(cdk): performance improvements by utilizing Promise.all #25945

Closed
wants to merge 4 commits into from

Conversation

sanjaiyan-dev
Copy link

@sanjaiyan-dev sanjaiyan-dev commented Jun 12, 2023

This pull request aims to optimize performance by leveraging the Promise.all method to execute functions concurrently. By utilizing this approach, we can achieve improved efficiency and potentially reduce the overall execution time of the codebase.

Changes Made

  • Introduced the use of Promise.all to concurrently execute multiple functions.
  • Replaced sequential function calls with parallel execution using Promise.all.
  • Utilized await to handle the resolved values of the promises returned by Promise.all.
  • Ensured proper error handling and error propagation within the promises.

Benefits

  • Enhances performance by executing functions concurrently, leveraging the available resources efficiently.
  • Reduces the overall execution time by avoiding unnecessary sequential execution.
  • Improves code readability by eliminating complex asynchronous control flow patterns.
  • Maintains error handling capabilities and propagates errors appropriately within the promises.

Please review these changes and let me know if any further adjustments or enhancements are required.

@gitpod-io
Copy link

gitpod-io bot commented Jun 12, 2023

@aws-cdk-automation aws-cdk-automation requested a review from a team June 12, 2023 19:58
@github-actions github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jun 12, 2023
@sanjaiyan-dev sanjaiyan-dev marked this pull request as ready for review June 12, 2023 19:59
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.

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

@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Jun 12, 2023
@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@sanjaiyan-dev
Copy link
Author

🤔

@mrgrain
Copy link
Contributor

mrgrain commented Jul 6, 2023

@aws-cdk/cdk-build-tools: lib/compile.ts(14,48): error TS2345: Argument of type '(javascriptFile: string) => Promise<void>' is not assignable to parameter of type '(value: unknown, index: number, array: unknown[]) => Promise<void>'.
@aws-cdk/cdk-build-tools:   Types of parameters 'javascriptFile' and 'value' are incompatible.
@aws-cdk/cdk-build-tools:     Type 'unknown' is not assignable to type 'string'.
@aws-cdk/cdk-build-tools: �[2K�[1G�[31merror�[39m Command failed with exit code 1.
@aws-cdk/cdk-build-tools: �[2K�[1G�[34minfo�[39m Visit �[1mhttps://yarnpkg.com/en/docs/cli/run�[22m for documentation about this command.

@mrgrain mrgrain changed the title Improving Performance by Utilizing Promise.all for Function Execution ✈ feat(cdk-lib): performance improvements by utilizing Promise.all Jul 6, 2023
@mrgrain
Copy link
Contributor

mrgrain commented Jul 6, 2023

@sanjaiyan-dev Very cool thanks for that! Unfortunately the change seems to make the build fail.

Are you invested enough to look into this?
Also, did you run any benchmarks on how much this improves the performance?

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mrgrain mrgrain added pr-linter/exempt-readme The PR linter will not require README changes 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 Jul 7, 2023
@mrgrain
Copy link
Contributor

mrgrain commented Jul 7, 2023

@sanjaiyan-dev I need a passing build and I can run the cli integ tests for you. I've exempted the other things.

@mrgrain mrgrain changed the title feat(cdk-lib): performance improvements by utilizing Promise.all fix(cdk-lib): performance improvements by utilizing Promise.all Jul 7, 2023
@mrgrain mrgrain changed the title fix(cdk-lib): performance improvements by utilizing Promise.all fix(cdk): performance improvements by utilizing Promise.all Jul 7, 2023
@sanjaiyan-dev
Copy link
Author

@sanjaiyan-dev Very cool thanks for that! Unfortunately the change seems to make the build fail.

Are you invested enough to look into this?
Also, did you run any benchmarks on how much this improves the performance?

I'm sorry to hear that the recent change caused the build to fail. I apologize for any inconvenience it may have caused. I'm invested in resolving the issue and will look into it immediately to identify the cause and find a solution. 🙌

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jul 11, 2023
@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/25945/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

@mrgrain
Copy link
Contributor

mrgrain commented Jul 12, 2023

I'm sorry to hear that the recent change caused the build to fail. I apologize for any inconvenience it may have caused. I'm invested in resolving the issue and will look into it immediately to identify the cause and find a solution. 🙌

No worries. Just open a new PR when you are ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. p2 pr/needs-cli-test-run This PR needs CLI tests run against it. pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-readme The PR linter will not require README 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

3 participants