Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix LruCache. Make TreeCache track its own size. #538

Merged
merged 6 commits into from
Jan 29, 2016

Conversation

erikjohnston
Copy link
Member

The LruCache failed to reset self.size when clear was called. This meant that self.size could be much large than the actual size, thereby reducing the actual effective size of the cache.

This PR fixes the issue by reverting the LruCache to not track size and instead use len(..) on the underlying cache. TreeCache now keeps track of its own size.

@erikjohnston erikjohnston changed the title Reset size on clear Fix LruCache. Make TreeCache track its own size. Jan 29, 2016
@NegativeMjark
Copy link
Contributor

Hmm, there are a couple of odd things actually

@@ -142,10 +140,11 @@ def cache_clear():
list_root[NEXT] = list_root
list_root[PREV] = list_root
cache.clear()
self.size = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this still here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also why didn't this fix the problem OOI?

Copy link
Member Author

Choose a reason for hiding this comment

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

I dislike the LruCache trying to keep track of the size of the underlying cache. I much prefer that caches/collections keep track of their own size.

@NegativeMjark
Copy link
Contributor

Also could you add a test to check you've fixed the bug?

@erikjohnston
Copy link
Member Author

@NegativeMjark PTAL

@NegativeMjark
Copy link
Contributor

LGTM

erikjohnston added a commit that referenced this pull request Jan 29, 2016
Fix LruCache. Make TreeCache track its own size.
@erikjohnston erikjohnston merged commit 02a9c3b into develop Jan 29, 2016
@erikjohnston erikjohnston deleted the erikj/fix_lru_cache branch February 23, 2016 16:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants