Implement heapq for cookie expire times #9203
Conversation
aiohttp/cookiejar.py
Outdated
| # expired heap, rebuild the expired heap without the deleted items | ||
| # and clear the deleted items. | ||
| deleted_len = len(self._deleted) | ||
| if deleted_len > 100 and deleted_len > len(self._expire_heap) * 2: |
There was a problem hiding this comment.
probably should make 100 a constant MIN_DELETED_TO_CLEANUP
bdraco
added
backport-3.10
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #9203 +/- ##
==========================================
- Coverage 98.31% 98.30% -0.02%
==========================================
Files 107 107
Lines 34508 34520 +12
Branches 4100 4104 +4
==========================================
+ Hits 33928 33935 +7
- Misses 410 412 +2
- Partials 170 173 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
noticed while working on #9203 TODO: show profiles
noticed while working on #9203 TODO: show profiles
| break | ||
| heapq.heappop(self._expire_heap) | ||
| cookie_key = entry[1:] | ||
| if self._expirations.get(cookie_key) == when: |
There was a problem hiding this comment.
needs some tests to changing expire times
Going backwards, going forwards, and the same time seen twice
| if not expire_heap_len: | ||
| return | ||
|
|
||
| if expire_heap_len > 100 and expire_heap_len > len(self._expirations) * 2: |
There was a problem hiding this comment.
Need a test for the cleanup
| ) | ||
| or predicate(morsel) | ||
| ] | ||
| if to_del: |
There was a problem hiding this comment.
Looks like we don't have coverage for clear matching nothing
| @@ -167,11 +159,40 @@ def __len__(self) -> int: | |||
| return sum(len(cookie.values()) for cookie in self._cookies.values()) | |||
|
|
|||
| def _do_expiration(self) -> None: | |||
There was a problem hiding this comment.
I'd put some comments in this function, it's a little dense to make sense of.
What do these changes do?
Implement heapq for cookie expire times
Are there changes in behavior for the user?
no
Is it a substantial burden for the maintainers to support this?
heapq is a bit of a complex structure to maintain
Related issue number
fixes #8575
fixes #7790
note that profile is not exactly the same number of iterations since its using a real world use case of 60s and its hard to get it exactly the same (1644 before) / (1413 after) but its such a significant difference, its more than enough to show the performance improvement of ~96-97% less run time (adjusted ratio for the iterations):
before
after
