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

feat: improve performance in storing current cloud backend #13821

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

ErlendHer
Copy link
Contributor

@ErlendHer ErlendHer commented Jun 13, 2024

Description of changes

This PR addresses the issue where for large repositories with JavaScript resources, the "saving deployment state" step can be incredibly slow. For large repositories, more than 20 minutes. The cause of this performance issue is unoptimised code copying over all node_modules to the #current_cloud_backend directory, then using a glob and deleting any node_modules that were copied. This is extremely inefficient, and the proposed changes simply ignores and do not copy over node_modules in the first place.

Benchmark
benchmark of the time it takes to process 1 randomly selected TypeScript lambda function from our repository
Before: 15.410s
After: 0.009s (9ms)

This is a reduction from 15 seconds down to 9 milliseconds. For our repository, this would reduce the copying step of "saving deployment state" from 20 minutes down to roughly a second.

Issue #13814

13814

Description of how you validated changes

Tried running a deploy with amplify-dev before and after the changes and verifying that the output of #current_cloud_backend are identical

Checklist

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

@ErlendHer ErlendHer requested a review from a team as a code owner June 13, 2024 09:50
@ErlendHer ErlendHer force-pushed the platform-push/move-resources-performance-improvement branch 2 times, most recently from d3319df to 73818f5 Compare June 13, 2024 09:56
edwardfoyle
edwardfoyle previously approved these changes Jul 2, 2024
@0618
Copy link
Contributor

0618 commented Jul 2, 2024

Thank you for contributing @ErlendHer ! Could you please remove the const removeNodeModulesDir... so that the PR testing check can pass on this file?

warning 'removeNodeModulesDir' is assigned a value but never used @typescript-eslint/no-unused-vars

@ErlendHer ErlendHer force-pushed the platform-push/move-resources-performance-improvement branch from 73818f5 to be4a10d Compare July 9, 2024 17:55
@ErlendHer
Copy link
Contributor Author

Thank you for contributing @ErlendHer ! Could you please remove the const removeNodeModulesDir... so that the PR testing check can pass on this file?

warning 'removeNodeModulesDir' is assigned a value but never used @typescript-eslint/no-unused-vars

Sorry for late response, have been out on holiday. Yes of course. It's been removed now

@awsluja
Copy link
Contributor

awsluja commented Jul 11, 2024

This is great, the PR checks are failing because the commit history is showing 'feat:', which involves making other version bumps to packages. Since this is purely a performance improvement, we can do this as a patch.

To do so, you can commit this as a 'fix' commit. That way, it results in a patch bump.

Ie, re-open this PR from a new branch, where you've committed this change as a 'fix' commit instead of a 'feat' commit.

@ErlendHer ErlendHer force-pushed the platform-push/move-resources-performance-improvement branch from be4a10d to 24c8f85 Compare July 12, 2024 09:34
@ErlendHer
Copy link
Contributor Author

This is great, the PR checks are failing because the commit history is showing 'feat:', which involves making other version bumps to packages. Since this is purely a performance improvement, we can do this as a patch.

To do so, you can commit this as a 'fix' commit. That way, it results in a patch bump.

Ie, re-open this PR from a new branch, where you've committed this change as a 'fix' commit instead of a 'feat' commit.

I rewrote history and changed the commit message instead, does that now work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants