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

Add lru_cache to many mapper properties #1245

Merged
merged 1 commit into from Aug 8, 2022

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Jun 13, 2022

This PR is primarily for optimisation of the mapper by lru caching lots of properties on first run. This is still an unfortunate workaround for our weird class-level storage of these properties, but I think this is a reasonable performance fix that maintains backwards-compat.

Need to run some benchmarks before this gets merged.

@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #1245 (28e3899) into master (5dc5e4a) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 28e3899 differs from pull request most recent head de1eb51. Consider uploading reports for the commit de1eb51 to get more accurate results

@@            Coverage Diff             @@
##           master    #1245      +/-   ##
==========================================
+ Coverage   90.79%   90.81%   +0.02%     
==========================================
  Files          72       72              
  Lines        4346     4357      +11     
==========================================
+ Hits         3946     3957      +11     
  Misses        400      400              
Flag Coverage Δ
project 90.81% <100.00%> (+0.02%) ⬆️
validator 90.81% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
optimade/server/mappers/entries.py 97.39% <100.00%> (+0.27%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ml-evs ml-evs added server Issues pertaining to the example server implementation on-hold For PRs/issues that are on-hold for an unspecified time labels Jul 6, 2022
@ml-evs ml-evs force-pushed the ml-evs/add_more_mapper_caches branch 2 times, most recently from ec9f0ca to 28e3899 Compare August 8, 2022 11:22
@ml-evs ml-evs removed the on-hold For PRs/issues that are on-hold for an unspecified time label Aug 8, 2022
@ml-evs ml-evs marked this pull request as ready for review August 8, 2022 11:55
@ml-evs
Copy link
Member Author

ml-evs commented Aug 8, 2022

Just did some quick tests with the reference server (which has limited data and few provider fields), the validator runs about 10% faster with these changes, so I think we should include this.

Copy link
Contributor

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

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

I think it is good to add these caches. I suspect there are many more places in the code where this could be useful.

@ml-evs ml-evs force-pushed the ml-evs/add_more_mapper_caches branch from 28e3899 to de1eb51 Compare August 8, 2022 15:04
@ml-evs ml-evs enabled auto-merge (squash) August 8, 2022 15:07
@ml-evs ml-evs added the enhancement New feature or request label Aug 8, 2022
@ml-evs ml-evs merged commit d47eec1 into master Aug 8, 2022
@ml-evs ml-evs deleted the ml-evs/add_more_mapper_caches branch August 8, 2022 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request server Issues pertaining to the example server implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants