| msg340429 - (view) |
Author: Jason R. Coombs (jaraco) *  |
Date: 2019-04-17 18:45 |
| In [this ticket](https://github.com/jaraco/jaraco.functools/issues/12), I learned that [jaraco.functools.method_cache](https://github.com/jaraco/jaraco.functools/blob/6b32ee0dfd3e7c88f99e88cd87c35fa9b76f261f/jaraco/functools.py#L109-L180) no longer works on Python 3.7.3. A distilled version of what's not working is this example: ``` >>> import jaraco.functools >>> class MyClass: ... calls = 0 ... @jaraco.functools.method_cache ... def call_me_maybe(self, val): ... self.calls += 1 ... return val ... >>> a = MyClass() >>> a.call_me_maybe(0) 0 >>> a.call_me_maybe(0) 0 >>> a.calls 2 ``` The second call to the cached function is missing the cache even though the parameters to the function are the same. ``` >>> a.call_me_maybe <functools._lru_cache_wrapper object at 0x107eb2df0> >>> a.call_me_maybe.cache_info() CacheInfo(hits=0, misses=2, maxsize=128, currsize=2) ``` Here's a further distilled example not relying on any code from jaraco.functools: ``` >>> def method_cache(method): ... def wrapper(self, *args, **kwargs): ... # it's the first call, replace the method with a cached, bound method ... bound_method = functools.partial(method, self) ... cached_method = functools.lru_cache()(bound_method) ... setattr(self, method.__name__, cached_method) ... return cached_method(*args, **kwargs) ... return wrapper ... >>> import functools >>> class MyClass: ... calls = 0 ... @method_cache ... def call_me_maybe(self, val): ... self.calls += 1 ... return val ... >>> a = MyClass() >>> a.call_me_maybe(0) 0 >>> a.call_me_maybe(0) 0 >>> a.calls 2 ``` I was not able to replicate the issue with a simple lru_cache on a partial object: ``` >>> def func(a, b): ... global calls ... calls += 1 ... >>> import functools >>> cached = functools.lru_cache()(functools.partial(func, 'a')) >>> calls = 0 >>> cached(0) >>> cached(0) >>> calls 1 ``` Suggesting that there's some interaction with the instance attribute and the caching functionality. I suspect the issue arose as a result of changes in . |
|
|
| msg340433 - (view) |
Author: Jason R. Coombs (jaraco) * |
Date: 2019-04-17 18:52 |
| I've put together this full reproducer script: ``` import functools def method_cache(method): def wrapper(self, *args, **kwargs): # it's the first call, replace the method with a cached, bound method bound_method = functools.partial(method, self) cached_method = functools.lru_cache()(bound_method) setattr(self, method.__name__, cached_method) return cached_method(*args, **kwargs) return wrapper class MyClass: calls = 0 @method_cache def call_me_maybe(self, number): self.calls += 1 return number ob = MyClass() ob.call_me_maybe(0) ob.call_me_maybe(0) assert ob.calls == 1 ``` That code fails on Python 3.7.3. If I bypass the `from _functools import _lru_cache_wrapper` in functools, the test no longer fails, so the issue seems to be only with the C implementation. |
|
|
| msg340438 - (view) |
Author: Josh Rosenberg (josh.r) *  |
Date: 2019-04-17 19:28 |
| It seems highly likely this is related to #26110 which optimizes method calls by avoiding the creation of bound methods in certain circumstances, and/or the follow-up #29263. Side-note: bound_method = functools.partial(method, self) is not how you create real bound methods. The correct approach (that makes a bound method identical to what the interpreter makes when necessary) for your use case is: bound_method = types.MethodType(method, self) |
|
|
| msg340439 - (view) |
Author: Josh Rosenberg (josh.r) * |
Date: 2019-04-17 19:34 |
| Hmm... Although I can't seem to reproduce on 3.7.2 (ob.calls is 1), so assuming it is really happening in 3.7.3, it wouldn't be either of those issues (which were fully in place long before 3.7.2 I believe). |
|
|
| msg340440 - (view) |
Author: Jason R. Coombs (jaraco) * |
Date: 2019-04-17 19:37 |
| Hi Josh. Thanks for the tip on types.MethodType. I've updated the code to use that and the behavior seems to be the same, MethodType does seem a more appropriate way to create a bound method. Regarding the referenced tickets, I suspect they're not pertinent, as the behavior did work in Python 3.7.1 (confirmed) and 3.7.2 (highly likely), so the regression appears to be in the changes for 3.7.3 (https://docs.python.org/3.7/whatsnew/changelog.html#python-3-7-3-final). |
|
|
| msg340554 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2019-04-19 18:22 |
| Thanks for the reproducer code. I've bisected this back to b2b023c657ba8c3f4a24d0c847d10fe8e2a73d44 which fixes other known bugs in the lru_cache in issue 35780. The problem is due to the lines that use a scalar instead of an args tuple for exact ints and strs. I'll work-up a PR to fix it soon (I'm on vacation and have limited connectivity so it may take a few days). |
|
|
| msg340555 - (view) |
Author: Jason R. Coombs (jaraco) * |
Date: 2019-04-19 18:34 |
| Nice work. Thanks Raymond. |
|
|
| msg340560 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2019-04-20 00:55 |
| For the record, here's what is going on. The method_cache() code uses a slightly different invocation for the first call than for subsequent calls. In particular, the wrapper() uses **kwargs with an empty dict whereas the first call didn't use keyword arguments at all. The C version of the lru_cache() is treating that first call as distinct from the second call, resulting in a cache miss for the both the first and second invocation but not in subsequent invocations. The pure python lru_cache() has a memory saving fast path taken when kwds is an empty dict. The C version is out-of-sync with because it runs the that path only when kwds==NULL and it doesn't check for the case where kwds is an empty dict. Here's a minimal reproducer: @lru_cache() def f(x): pass f(0) f(0, **{}) assert f.cache_info().hits == 1 Here's a possible fix: diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index 3f1c01651d..f118119479 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -751,7 +751,7 @@ lru_cache_make_key(PyObject *args, PyObject *kwds, int typed) Py_ssize_t key_size, pos, key_pos, kwds_size; /* short path, key will match args anyway, which is a tuple */ - if (!typed && !kwds) { + if (!typed && (!kwds | |
PyDict_GET_SIZE(kwds) == 0)) { if (PyTuple_GET_SIZE(args) == 1) { key = PyTuple_GET_ITEM(args, 0); if (PyUnicode_CheckExact(key) |
|
| msg340586 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2019-04-20 17:20 |
| New changeset 14adbd45980f705cb6554ca17b8a66b56e105296 by Raymond Hettinger in branch 'master': bpo-36650: Fix handling of empty keyword args in C version of lru_cache. (GH-12881) https://github.com/python/cpython/commit/14adbd45980f705cb6554ca17b8a66b56e105296 |
|
|
| msg340587 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2019-04-20 17:50 |
| New changeset 8b30ee843528d0f0e2bfc3307d86658915579c21 by Raymond Hettinger (Miss Islington (bot)) in branch '3.7': bpo-36650: Fix handling of empty keyword args in C version of lru_cache. (GH-12881) (GH-12888) https://github.com/python/cpython/commit/8b30ee843528d0f0e2bfc3307d86658915579c21 |
|
|