Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GC support #138

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ring/func/asyncio.py
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ def set_many_values(self, keys, values, expire):
def dict(
obj, key_prefix=None, expire=None, coder=None,
user_interface=CacheUserInterface, storage_class=None,
**kwargs):
maxsize=128, **kwargs):
""":class:`dict` interface for :mod:`asyncio`.

:see: :func:`ring.func.sync.dict` for common description.
Expand All @@ -463,6 +463,7 @@ def dict(
storage_class = fsync.PersistentDictStorage
else:
storage_class = fsync.ExpirableDictStorage
storage_class.maxsize = maxsize
Copy link
Owner

Choose a reason for hiding this comment

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

This code looks like it changes the maxsize value of the storage class. It will cause side effect for other calls with different value.

Copy link
Author

Choose a reason for hiding this comment

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

I assumed
if storage_class is None:
means that we're in a 'default' mode so we're in the somewhat private zone... what about _maxsize?

Copy link
Owner

Choose a reason for hiding this comment

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

The point is not about the naming rule. The next code will make problem.

# maxsize of f is 512 here because fsync.PersistentDictStorage.maxsize is 512
@ring.dict(maxsize=512)
def f():
   ...

# maxsize of both f and g are 128 now becasue fsync.PersistentDictStorage is 128
@ring.dict(maxsize=128)
def g():
   ...

We shouldn't set any kind of class variable for decorator parameters.

Copy link
Author

Choose a reason for hiding this comment

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

I totally missed that. What a shame. I'll figure out a better way.


return fbase.factory(
obj, key_prefix=key_prefix, on_manufactured=None,
Expand Down
6 changes: 4 additions & 2 deletions ring/func/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,7 @@ def factory(
# keyword-only arguments from here
# building blocks
coder, miss_value, user_interface, storage_class,
maxsize=128,
default_action=Ellipsis,
coder_registry=Ellipsis,
# callback
Expand Down Expand Up @@ -638,7 +639,7 @@ class RingRope(RopeCore):
def __init__(self, *args, **kwargs):
super(RingRope, self).__init__(*args, **kwargs)
self.user_interface = self.user_interface_class(self)
self.storage = self.storage_class(self, storage_backend)
self.storage = self.storage_class(self, storage_backend, maxsize)
Copy link
Owner

Choose a reason for hiding this comment

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

Does redis_hash failure related to this change?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, __init__ is overridden in RedisHashStorage. I'm looking at right now and seems there are some workarounds to get back to work but not sure which one will be the best one.
The problem is that BaseStorage has a init and ExpirableDictStorage & PersistentDictStorage adheres it but RedisHashStorage overrides it.

Can you tell me your opinion?

_ignorable_keys = suggest_ignorable_keys(
self.callable, ignorable_keys)
_key_prefix = suggest_key_prefix(self.callable, key_prefix)
Expand Down Expand Up @@ -747,9 +748,10 @@ class BaseStorage(object):
are mandatory; Otherwise not.
"""

def __init__(self, rope, backend):
def __init__(self, rope, backend, maxsize):
self.rope = rope
self.backend = backend
self.maxsize = maxsize

@abc.abstractmethod
def get(self, key): # pragma: no cover
Expand Down
69 changes: 65 additions & 4 deletions ring/func/sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,66 @@ def touch_value(self, key, expire):
except KeyError:
pass

class SizeMaintainer(object):
_DEBUG = False

def __init__(self, backend, target_size, expire_f=None):
from collections import abc
assert round(target_size) > 0, 'target_size has to be at least 1'
assert expire_f is None or callable(expire_f), 'expire_f has to be function or None'
assert isinstance(backend, abc.MutableMapping), 'backend has to be dict-like'
self._backend = backend
self._target_size = round(target_size)
self._expire_f = expire_f

def run(self):
if (len(self._backend) <= self._target_size):
return

import random, time
def strategy_with_expire():
MAX_EXPIRE_RETRY_COUNT = 4
now = time.time()
retry_count = 0

keys = list(self._backend.keys())
random.shuffle(keys)
key_index = 0
while (len(self._backend) > self._target_size) and (retry_count < MAX_EXPIRE_RETRY_COUNT):
key = keys[key_index]
val = self._backend.get(key, None)
expire = self._expire_f(val)
if expire < now:
self._backend.pop(key, None)
key_index += 1
if self._DEBUG:
print('{} removed from strategy_with_expire => size {}'.format(key, len(self._backend)))
else:
retry_count += 1

def strategy_with_force():
keys = list(self._backend.keys())
random.shuffle(keys)
key_index = 0
while (len(self._backend) > self._target_size):
key = keys[key_index]
self._backend.pop(key, None)
key_index += 1
if self._DEBUG:
print('{} removed from strategy_with_force => size {}'.format(key, len(self._backend)))

if self._DEBUG:
print('gc started in size:{}'.format(len(self._backend)))

if self._expire_f is not None:
strategy_with_expire()
strategy_with_force()

if self._DEBUG:
print('gc ended in size:{}'.format(len(self._backend)))

class ExpirableDictStorage(fbase.CommonMixinStorage, fbase.StorageMixin):

class ExpirableDictStorage(fbase.CommonMixinStorage, fbase.StorageMixin):
in_memory_storage = True
now = time.time

Expand All @@ -229,6 +286,9 @@ def set_value(self, key, value, expire):
expired_time = _now + expire
self.backend[key] = expired_time, value

if self.maxsize < len(self.backend):
SizeMaintainer(self.backend, self.maxsize * 0.75, lambda x: x[0]).run()

def delete_value(self, key):
try:
del self.backend[key]
Expand All @@ -252,7 +312,6 @@ def touch_value(self, key, expire):


class PersistentDictStorage(fbase.CommonMixinStorage, fbase.StorageMixin):

in_memory_storage = True

def get_value(self, key):
Expand All @@ -264,6 +323,8 @@ def get_value(self, key):

def set_value(self, key, value, expire):
self.backend[key] = value
if self.maxsize < len(self.backend):
SizeMaintainer(self.backend, self.maxsize * 0.75).run()

def delete_value(self, key):
try:
Expand Down Expand Up @@ -443,7 +504,7 @@ def lru(
def dict(
obj, key_prefix=None, expire=None, coder=None,
user_interface=CacheUserInterface, storage_class=None,
**kwargs):
maxsize=128, **kwargs):
"""Basic Python :class:`dict` based cache.

This backend is not designed for real products. Please carefully read the
Expand All @@ -470,7 +531,7 @@ def dict(

return fbase.factory(
obj, key_prefix=key_prefix, on_manufactured=None,
user_interface=user_interface, storage_class=storage_class,
user_interface=user_interface, storage_class=storage_class, maxsize=maxsize,
miss_value=None, expire_default=expire, coder=coder,
**kwargs)

Expand Down
140 changes: 140 additions & 0 deletions tests/test_dict_size.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@

import ring


def test_dict_size_persistence_1():
cache = {}
MAX_SIZE = 1

@ring.dict(cache, maxsize=MAX_SIZE)
def f_persistent_1(i):
return i

for i in range(MAX_SIZE * 100):
f_persistent_1(i)
assert len(cache) <= 1
assert len(cache) <= 1

def test_dict_size_persistence_default():
cache = {}

@ring.dict(cache)
def f_persistent_default(i):
return i

for i in range(1000):
f_persistent_default(i)
assert len(cache) <= 128
assert len(cache) <= 128

def test_dict_size_persistence_1000():
cache = {}
MAX_SIZE = 1000

@ring.dict(cache, maxsize=MAX_SIZE)
def f_persistent_default(i):
return i

for i in range(MAX_SIZE * 100):
f_persistent_default(i)
assert len(cache) <= MAX_SIZE
assert len(cache) <= MAX_SIZE

def test_dict_size_persistent_with_delete():
cache = {}
MAX_SIZE = 10

@ring.dict(cache, maxsize=MAX_SIZE)
def f_persistent_with_delete(i):
return i

for i in range(MAX_SIZE * 100):
f_persistent_with_delete(i)
assert len(cache) <= MAX_SIZE
if i % 17 == 0:
for pop_count in range(8):
if len(cache) > 0:
cache.popitem()


assert len(cache) <= MAX_SIZE

def test_dict_size_expire_1():
cache = {}
MAX_SIZE = 1

@ring.dict(cache, maxsize=MAX_SIZE, expire=1)
def f_expire_1(i):
return i

for i in range(MAX_SIZE * 100):
f_expire_1(i)
assert len(cache) <= MAX_SIZE
assert len(cache) <= MAX_SIZE

def test_dict_size_expire_default():
cache = {}

@ring.dict(cache, expire=1)
def f_expire_default(i):
return i

for i in range(1000):
f_expire_default(i)
assert len(cache) <= 128
assert len(cache) <= 128

def test_dict_size_expire_1000():
cache = {}
MAX_SIZE = 1000

@ring.dict(cache, maxsize=MAX_SIZE, expire=1)
def f_expire_1(i):
return i

for i in range(MAX_SIZE * 100):
f_expire_1(i)
assert len(cache) <= MAX_SIZE

assert len(cache) <= MAX_SIZE

def test_dict_size_expire_some():
import time
cache = {}
MAX_SIZE = 150

@ring.dict(cache, maxsize=MAX_SIZE, expire=1)
def f_expire_some_expire(i):
return i

for _ in range(5):
for i in range(100):
f_expire_some_expire(i)
assert len(cache) <= MAX_SIZE
time.sleep(1)
for i in range(100, 200):
f_expire_some_expire(i)
assert len(cache) <= MAX_SIZE
assert len(cache) <= MAX_SIZE


def test_dict_size_expire_with_delete():
import time
cache = {}

@ring.dict(cache, expire=1)
def f_expire_with_delete(i):
return i

for i in range(1000):
f_expire_with_delete(i)
time.sleep(1)
for i in range(1000, 2000):
f_expire_with_delete(i)
assert len(cache) <= 128
if i % 17 == 0:
for pop_count in range(8):
if len(cache) > 0:
cache.popitem()

assert len(cache) <= 128