repoze / repoze.lru

Tiny LRU cache
http://pypi.python.org/pypi/repoze.lru/
Other
103 stars 24 forks source link

Fixes for issues #2-#8 #9

Closed snordhausen closed 12 years ago

snordhausen commented 12 years ago

Issue #2 (LRUCache will evict entries even though not full) get() does not set self.hand any more, so no entries will be evicted before the cache is really full.

Issue #3 (Repeated pushing of same entry removes other cache entries) LRUCache now checks if the key is already in the cache. If so, no other entries are evicted.

Issue #4 (LRUCache.clock wastes RAM) self.clock was split up into two lists, thereby saving a dict for each cache entry

Issue #6 (LRUCache is not thread safe) self.clear() now atomically resets self.clock_xxx, avoiding the index out of bounds errors when get() or put() were accessing self.clock. Additionally, reading self.hand in put() is now protected by a lock, preventing problems when many threads try to put() simultaneously and need to wait on another that holds the lock

Issue #7 (LRUCache.put() can take multiple seconds on large caches) put() will now check at most 107 entries. This avoids applications from stalling and in the worst case, it will only evict <1% innocent victims. After that, it evicts those items that would have been evicted, anyway.

issue #8 (LRUCache should provide a way to remove items from the cache) A new "invalidate()" method was added.

mcdonc commented 12 years ago

This looks like great work.

Is there any way I could tweak you into writing enough tests to maintain the 100% statement coverage that the repoze.lru module had before these changes?

$ cd /path/to/checkout/of/repoze.lru
$ /somevirtualenv/bin/python setup.py develop
$ /somevirtualenv/bin/easy_install nose coverage
$ /somevirtualenv/bin/python setup.py nosetests --with-coverage

This will produce a coverage report indicating the untested lines.

Also the tests no longer run under anything but Python 2.7, and the code no longer works under Python 3.2, and this will have to be fixed before a merge. A reasonably easy way to run the full test suite on all platforms is via "tox"

$ cd /path/to/checkout/of/repoze.lru
$ /somevirtualenv/bin/easy_install tox
$ /somevirtualenv/bin/tox

This will attempt to run the tests under Python 2.5, 2.6, 2.7, 3.2, PyPy and Jython, each of which will need to be present on your system (see http://tox.testrun.org/latest/). Note that if it passes on Python 2.5 it will probably pass under Jython and if it passes on 2.7 it will probably pass under PyPy, so if those are too much trouble to install, I wouldn't bother too much. But 2.5, 2.6, 2.7, and 3.2 compatibility are definitely a requisite.

snordhausen commented 12 years ago

I really did manage to break everything but Python 2.7... The issues that popped up during local re-testing are now fixed in my fork. Tests now pass on Python 2.4-2.7, Python 3.1, Python 3.2 (issues a DeprecationWarning) and Jython 2.5.1+. No Debian package for PyPy in the repos, so haven't tested it.

I will also have a look at reaching full statement later on.

snordhausen commented 12 years ago

I just checked the statement coverage thing, and it seems some part of the test framework does not like files that were chmoded to 755. When tests.py has permissions 755 (rwxr-xr-x) I get:

Name         Stmts   Miss  Cover   Missing
------------------------------------------
repoze.lru     175    154    12%   23-33, 37-51, 55-60, 65-115, 120-124, 135-146, 150-165, 169-181, 190-241, 246-250, 260-265, 268-279
----------------------------------------------------------------------
Ran 0 tests in 0.002s

Once I chmod the test script to 644 (rw-r--r--) I get:

Name               Stmts   Miss  Cover   Missing
------------------------------------------------
repoze.lru           175      0   100%   
repoze.lru.tests     370      1    99%   532
------------------------------------------------
TOTAL                545      1    99%   
----------------------------------------------------------------------
Ran 25 tests in 3.851s

(The only statement that is not run is the if name == 'main' stuff in the test cases)

Can this be fixed somewhere in the test configuration or do I need to chmod the script back to 644?

tseaver commented 12 years ago

I merged the request with changelog entries.

I also removed the 'if name == 'main' bit from 'tests.py': we prefer to use 'setup.py test' or nosetests to run the package tests, rather than treating test modules as scripts.

mcdonc commented 12 years ago

Thanks to Tres this stuff got merged, sorry for the flake-out.

I'm wondering if you could add your name to CONTRIBUTORS.txt in a pull request, snordhausen?