Skip to content

(towards 312) Remove imports and function declaration from hotpath (__new__ and match)#337

Merged
arporter merged 10 commits intomasterfrom
312_dynamic_import
Jun 14, 2022
Merged

(towards 312) Remove imports and function declaration from hotpath (__new__ and match)#337
arporter merged 10 commits intomasterfrom
312_dynamic_import

Conversation

@sergisiso
Copy link
Collaborator

@sergisiso sergisiso commented May 3, 2022

See performance gains in #312

@codecov
Copy link

codecov bot commented May 3, 2022 •
edited
Loading

Codecov Report

Merging #337 (7a9d95f) into master (4003e81) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
+ Coverage   91.33%   91.34%   +0.01%     
==========================================
  Files          36       36              
  Lines       13028    13047      +19     
==========================================
+ Hits        11899    11918      +19     
  Misses       1129     1129
Impacted Files Coverage Δ
src/fparser/two/Fortran2003.py 93.63% <100.00%> (+<0.01%) ⬆️
src/fparser/two/utils.py 95.57% <100.00%> (+0.10%) ⬆️

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 4003e81...7a9d95f. Read the comment docs.

@sergisiso sergisiso mentioned this pull request May 3, 2022
Open
@sergisiso sergisiso force-pushed the 312_dynamic_import branch from 324199b to 2e08bc7 Compare June 6, 2022 16:12
@sergisiso
Copy link
Collaborator Author

sergisiso commented Jun 6, 2022

@arporter This is also ready for review.

It removes the imports and a declaration of an inner function (which both have a non-negligible cost) outside the Base.__new__ method, which is the most executed method in all fparser (all instantiations that try to match anything in the parser end up calling it). I could not easily resolve the cyclic dependencies without a big code restructuring, so I opted for a more unconventional dynamic import class.

This speedups up 20% the processing of NEMO files.

@sergisiso sergisiso requested a review from arporter June 6, 2022 16:21
@sergisiso sergisiso self-assigned this Jun 6, 2022
@sergisiso sergisiso force-pushed the 312_dynamic_import branch from 2e08bc7 to 163a1cc Compare June 6, 2022 16:41
arporter
arporter requested changes Jun 9, 2022
View reviewed changes
Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Impressive.
Changes are all pycodestyle/pylint clean.
Performance with the benchmark is now down from 49s to 41s (ish).
Just a few minor typos to fix and a request for a comment.

@arporter arporter added reviewed with actions PR has been reviewed and is back with developer and removed under review labels Jun 9, 2022
@sergisiso sergisiso added ready for review and removed reviewed with actions PR has been reviewed and is back with developer labels Jun 13, 2022
@sergisiso
Copy link
Collaborator Author

sergisiso commented Jun 13, 2022

@arporter Ready for next review

arporter
arporter approved these changes Jun 14, 2022
View reviewed changes
Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

All requested changes have been made.
Coverage of the diff is still 100%.
Proceeding to merge.

@arporter arporter added the ready for merge PR is waiting on final CI checks before being merged. label Jun 14, 2022
@arporter arporter merged commit ffee889 into master Jun 14, 2022
@arporter arporter deleted the 312_dynamic_import branch June 14, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arporter arporter arporter approved these changes

Assignees

@sergisiso sergisiso

Labels

ready for merge PR is waiting on final CI checks before being merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sergisiso @arporter