pkgset.FileCache: implement __contains__ for efficiency #267
Conversation
AdamWill
force-pushed
the
filecache-implement-contains
branch
from
6583c8b
to
ad941e6
Compare
| @@ -229,6 +229,9 @@ def __getitem__(self, name): | |||
| def __setitem__(self, name, value): | |||
| self.file_cache[os.path.abspath(name)] = value | |||
|
|
|||
| def __contains__(self, item): | |||
There was a problem hiding this comment.
I must say, the mismatch between getitem/setitem and contains is concerning.
Keys are converted into absolute paths in the getter/setter but they are NOT converted during a contains check.
This means the basic contract of "inserting an object into a container means the container contains it" is violated:
from kobo.pkgset import FileCache
fc = FileCache()
fc['/y'] = object()
# Is the thing we just inserted in there?
# Of course it is...
assert '/y' in fc
# How about now?
fc['x'] = object()
# Nope - CRASH! I asked for 'x' but it stored '$PWD/x'
# and did not do the same transformation during 'contains'.
assert 'x' in fc I initially thought that your change introduced this problem, however the problem is already present on the current foo in self.file_cache path making use of __iter__, the code just wasn't visible.
I guess we should not attempt to fix that in this change, since it might be backwards-incompatible.
But if you are looking at code using this class, maybe you want to take a look at it with this bug in mind.
Any code using this class and not providing absolute paths as input has the potential to behave very strangely.
See https://pagure.io/pungi/pull-request/1796 . We figured out there that `foo in somefilecache` works, but is much less performant than `foo in somefilecache.file_cache`, because the latter hits the efficient `__contains__` method that Python dicts have, but the former winds up iterating over the dict's keys via `FileCache.__iter__`. In an extreme case, this was making something take 20 minutes instead of 3 seconds. Users can avoid this by just hitting the `file_cache` directly as in the PR, but to fix this for any other users without them having to change, let's implement `__contains__` in `FileCache` by just passing it through to the dict. Signed-off-by: Adam Williamson <[email protected]>
AdamWill
force-pushed
the
filecache-implement-contains
branch
from
ad941e6
to
d66f05e
Compare
See https://pagure.io/pungi/pull-request/1796 . We figured out there that
foo in somefilecacheworks, but is much less performant thanfoo in somefilecache.file_cache, because the latter hits the efficient__contains__method that Python dicts have, but the former winds up iterating over the dict's keys viaFileCache.__iter__. In an extreme case, this was making something take 20 minutes instead of 3 seconds.Users can avoid this by just hitting the
file_cachedirectly as in the PR, but to fix this for any other users without them having to change, let's implement__contains__inFileCacheby just passing it through to the dict.