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

Py27dict deepcopy performance #2331

Merged
merged 2 commits into from Feb 24, 2022

Conversation

marekaiv
Copy link
Contributor

Issue #, if available: n/a

Description of changes:
Implement __deepcopy__ for Py27Dict and related classes in order to improve performance. The generic deep copy in copy works, but is slow.

Description of how you validated changes:
Unit tests were added, and existing unit tests cover Py27Dict functionality. We also timed how long the transform took for a large template before and after the changes (for a very large template, these changes reduced processing time by ~80%). We confirmed that the transformed templates were the same before and after the changes.

Checklist:

  • Add/update unit tests using:
    • Correct values
    • Bad/wrong values (None, empty, wrong type, length, etc.) n/a
    • Intrinsic Functions n/a
  • Add/update integration tests n/a
  • make pr passes
  • Update documentation not necessary
  • Verify transformed template deploys and application functions as expected -- confirmed transformed template didn't change

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

@codecov-commenter
Copy link

codecov-commenter commented Feb 24, 2022

Codecov Report

Merging #2331 (858a2e5) into develop (e7a1496) will increase coverage by 0.73%.
The diff coverage is 98.20%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2331      +/-   ##
===========================================
+ Coverage    93.58%   94.31%   +0.73%     
===========================================
  Files           90       97       +7     
  Lines         6124     7089     +965     
  Branches      1260     1460     +200     
===========================================
+ Hits          5731     6686     +955     
- Misses         183      194      +11     
+ Partials       210      209       -1     
Impacted Files Coverage Δ
samtranslator/model/api/api_generator.py 94.38% <92.95%> (+0.02%) ⬆️
samtranslator/__init__.py 100.00% <100.00%> (ø)
samtranslator/feature_toggle/dialup.py 100.00% <100.00%> (ø)
samtranslator/feature_toggle/feature_toggle.py 100.00% <100.00%> (+12.16%) ⬆️
samtranslator/intrinsics/actions.py 98.78% <100.00%> (+0.06%) ⬆️
samtranslator/intrinsics/resource_refs.py 95.83% <100.00%> (-0.17%) ⬇️
samtranslator/metrics/method_decorator.py 100.00% <100.00%> (ø)
samtranslator/metrics/metrics.py 100.00% <100.00%> (ø)
samtranslator/model/__init__.py 97.63% <100.00%> (-0.02%) ⬇️
samtranslator/region_configuration.py 77.77% <0.00%> (-22.23%) ⬇️
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87d2a88...858a2e5. Read the comment docs.

@marekaiv marekaiv force-pushed the py27dict_deepcopy_performance branch 2 times, most recently from b024e8d to f50db8d Compare February 24, 2022 03:37
Copy link
Contributor

@CoshUS CoshUS left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the PR!

@marekaiv marekaiv force-pushed the py27dict_deepcopy_performance branch from f50db8d to c626e73 Compare February 24, 2022 16:16
Copy link
Contributor

@hawflau hawflau left a comment

Choose a reason for hiding this comment

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

LGTM!

@marekaiv marekaiv merged commit dfb1307 into aws:develop Feb 24, 2022
@marekaiv marekaiv deleted the py27dict_deepcopy_performance branch February 24, 2022 17:28
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

4 participants