Add LRU caching to tzoffset, tzstr and gettz#761
Merged
pganssle merged 4 commits intodateutil:masterfrom Oct 17, 2018
Merged
Conversation
Contributor
Author
|
Fixed the 2.7 16.1310862749815 μs (tz.gettz() calls without storing values)
19.910280592739582 μs (tz.gettz() calls with storing values)
14.881940744817257 μs (tz.tzoffset() calls without storing values)
17.241469584405422 μs (tz.tzoffset() calls with storing values)
15.357932634651661 μs (tz.tzstr() calls without storing values)
14.645632356405258 μs (tz.tzstr() calls with storing values) |
Contributor
Author
|
I am aware that CI fails and will fix We can add test for |
be4fbbb to
6719b6c
Compare
Contributor
Author
|
Sorry for delaying this. I cleaned up the commits a little bit and I think it is ready for review. |
Member
|
No problem on delay. If you notice the project board review is currently the bottleneck. |
686044f to
f57999c
Compare
pganssle
added a commit
to gokcennurlu/dateutil
that referenced
this pull request
Sep 24, 2018
30d2c51 to
d22cdc2
Compare
pganssle
added a commit
to gokcennurlu/dateutil
that referenced
this pull request
Sep 24, 2018
pganssle
approved these changes
Sep 24, 2018
Member
pganssle
left a comment
There was a problem hiding this comment.
@gokcennurlu I believe this is ready to merge. Can you review the changelog and AUTHORS.md changes that I added to make sure you are adequately credited before I merge?
d22cdc2 to
f17b3bf
Compare
pganssle
added a commit
to gokcennurlu/dateutil
that referenced
this pull request
Oct 16, 2018
Caching had been switched to use `weakrefs` in order to reuse instances if they are still alive, by dateutil#635. This introduces a LRU cache to the mentioned functions in order to prevent the instances created by them getting dealloc'd and alloc'd unnecessarily, in situations like this: ```python for i in range(100): tz.gettz('America/New_York') ``` `tz.tzoffset` and `tz.tzstr` get a LRU cache with size of 8. `tz.gettz`'s cache starts with 8 by default and can be modified by the introduced `tz.set_cache_size(int)` Closes dateutil#691
f17b3bf to
02ace35
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
I prepared a benchmark script and here are the results without the patch:
Benchmark:
https://gist.github.com/gokcennurlu/2b4e9d82e255c73e920c00ca5d26dcec
Results without patch:
Results without patch:
'with storing values' means that a strong reference is kept in the scope (by doing
a = func(..)) and lower values there explains the optimizing effect of currently usedWeakValueDictionarybased. caching.Closes #691
Pull Request Checklist