From c3cc9277c41a71ca033bad3cbfe26233f599cbe7 Mon Sep 17 00:00:00 2001 From: Raymond Hettinger Date: Fri, 1 Feb 2019 17:06:26 -0800 Subject: [PATCH 01/10] Start adding link guard macros --- Modules/_functoolsmodule.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index d72aaff2b13c4a..75ae05a7cf941e 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -844,6 +844,9 @@ infinite_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwd return result; } +#define MARK_UNUSED(link) {link->prev = NULL; link->next = NULL;} +#define IS_USED(link) (link->prev != NULL && link->next != NULL) + static void lru_cache_extract_link(lru_list_elem *link) { @@ -851,6 +854,7 @@ lru_cache_extract_link(lru_list_elem *link) lru_list_elem *link_next = link->next; link_prev->next = link->next; link_next->prev = link->prev; + MARK_UNUSED(link); } static void @@ -920,7 +924,7 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds return NULL; } link = (lru_list_elem *)_PyDict_GetItem_KnownHash(self->cache, key, hash); - if (link != NULL) { + if (link != NULL && IS_USED(link)) { lru_cache_extract_link(link); lru_cache_append_link(self, link); result = link->result; From 423fbd8c93cf8ccd01588f1d3f5dd6b33bca421b Mon Sep 17 00:00:00 2001 From: Raymond Hettinger Date: Fri, 1 Feb 2019 17:30:29 -0800 Subject: [PATCH 02/10] Mark unused when links are created or extracted. Mark used when appended. --- Modules/_functoolsmodule.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index 75ae05a7cf941e..bbd64df34d333e 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -850,8 +850,10 @@ infinite_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwd static void lru_cache_extract_link(lru_list_elem *link) { - lru_list_elem *link_prev = link->prev; - lru_list_elem *link_next = link->next; + lru_list_elem *link_prev, *link_next; + assert(IS_USED(link)); + link_prev = link->prev; + link_next = link->next; link_prev->next = link->next; link_next->prev = link->prev; MARK_UNUSED(link); @@ -860,21 +862,27 @@ lru_cache_extract_link(lru_list_elem *link) static void lru_cache_append_link(lru_cache_object *self, lru_list_elem *link) { - lru_list_elem *root = &self->root; - lru_list_elem *last = root->prev; + lru_list_elem *root, *last; + assert(!IS_USED(link)); + root = &self->root; + last = root->prev; last->next = root->prev = link; link->prev = last; link->next = root; + assert(IS_USED(link)); } static void lru_cache_prepend_link(lru_cache_object *self, lru_list_elem *link) { - lru_list_elem *root = &self->root; - lru_list_elem *first = root->next; + lru_list_elem *root, *first; + assert(!IS_USED(link)); + root = &self->root; + first = root->next; first->prev = root->next = link; link->prev = root; link->next = first; + assert(IS_USED(link)); } /* General note on reentrancy: @@ -976,6 +984,7 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds Py_DECREF(result); return NULL; } + MARK_UNUSED(link); link->hash = hash; link->key = key; @@ -990,7 +999,9 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds Py_DECREF(link); return NULL; } - lru_cache_append_link(self, link); + if (!IS_USED(link)) { + lru_cache_append_link(self, link); + } Py_INCREF(result); /* for return */ return result; } From 5c7aa812ffdae4631c3d5ee6db413800749f64db Mon Sep 17 00:00:00 2001 From: Raymond Hettinger Date: Fri, 1 Feb 2019 17:40:22 -0800 Subject: [PATCH 03/10] Cover remaining code paths --- Modules/_functoolsmodule.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index bbd64df34d333e..1e488fc6acb18a 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -845,7 +845,7 @@ infinite_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwd } #define MARK_UNUSED(link) {link->prev = NULL; link->next = NULL;} -#define IS_USED(link) (link->prev != NULL && link->next != NULL) +#define IS_USED(link) (link != NULL && link->prev != NULL && link->next != NULL) static void lru_cache_extract_link(lru_list_elem *link) @@ -1019,9 +1019,17 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds */ PyObject *oldkey, *oldresult, *popresult; - /* Extract the oldest item. */ + /* Get the oldest link. */ assert(self->root.next != &self->root); link = self->root.next; + if (!IS_USED(link)) { + /* Getting here means another thread is in the process of + removing this link, so let that proceed normally and + let's just return the calculated result.*/ + Py_DECREF(key); + return result; + } + /* Extract the oldest item. */ lru_cache_extract_link(link); /* Remove it from the cache. The cache dict holds one reference to the link, @@ -1075,7 +1083,9 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds Py_DECREF(oldresult); return NULL; } - lru_cache_append_link(self, link); + if (!IS_USED(link)) { + lru_cache_append_link(self, link); + } Py_INCREF(result); /* for return */ Py_DECREF(popresult); Py_DECREF(oldkey); From 684f58d75931be07d92e4ac8f8abab579ac8fc98 Mon Sep 17 00:00:00 2001 From: Raymond Hettinger Date: Fri, 1 Feb 2019 17:46:41 -0800 Subject: [PATCH 04/10] Add usage note for the macros --- Modules/_functoolsmodule.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index 1e488fc6acb18a..9d41284fa26941 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -844,6 +844,9 @@ infinite_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwd return result; } +/* Mark a link as unused when it is created or when it is extracted + from the doubly-linked list. Use this marker to avoid being + updated by two threads at the same time. */ #define MARK_UNUSED(link) {link->prev = NULL; link->next = NULL;} #define IS_USED(link) (link != NULL && link->prev != NULL && link->next != NULL) From ebbec89a2f72f31915dcd8f387d81cb20d1a7c1b Mon Sep 17 00:00:00 2001 From: Raymond Hettinger Date: Fri, 1 Feb 2019 20:01:10 -0800 Subject: [PATCH 05/10] Fix nits --- Modules/_functoolsmodule.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index 9d41284fa26941..e81a6bad3cdfa7 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -846,7 +846,11 @@ infinite_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwd /* Mark a link as unused when it is created or when it is extracted from the doubly-linked list. Use this marker to avoid being - updated by two threads at the same time. */ + updated by two threads at the same time. We don't want to + append a link unless we know it is not already in used + and we don't want to extract a link that is NULL or already + in the process of being extracted. + */ #define MARK_UNUSED(link) {link->prev = NULL; link->next = NULL;} #define IS_USED(link) (link != NULL && link->prev != NULL && link->next != NULL) @@ -1074,7 +1078,7 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds prev and next fields set to valid values. We have to wait for successful insertion in the cache dict before adding the link to the linked list. Otherwise, the potentially reentrant - __eq__ call could cause the then ophan link to be visited. */ + __eq__ call could cause the then orphan link to be visited. */ if (_PyDict_SetItem_KnownHash(self->cache, key, (PyObject *)link, hash) < 0) { /* Somehow the cache dict update failed. We no longer can @@ -1086,10 +1090,13 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds Py_DECREF(oldresult); return NULL; } + Py_INCREF(result); /* for return */ if (!IS_USED(link)) { lru_cache_append_link(self, link); } - Py_INCREF(result); /* for return */ + else { + Py_DECREF(link); + } Py_DECREF(popresult); Py_DECREF(oldkey); Py_DECREF(oldresult); From 8bd21d2fa5a9c5967137bdac013c355c462a6718 Mon Sep 17 00:00:00 2001 From: Raymond Hettinger Date: Sat, 2 Feb 2019 09:40:46 -0800 Subject: [PATCH 06/10] Add guard to prepend_link --- Modules/_functoolsmodule.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index e81a6bad3cdfa7..772e5ba21bddc5 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -1059,7 +1059,12 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds original position as the oldest link. Then we allow the error propagate upward; treating it the same as an error arising in the user function. */ - lru_cache_prepend_link(self, link); + if (!IS_USED(link)) { + lru_cache_prepend_link(self, link); + } + else { + Py_DECREF(link); + } Py_DECREF(key); Py_DECREF(result); return NULL; From df4ae2eec44f03255237dac78d532fdfa9e91603 Mon Sep 17 00:00:00 2001 From: Raymond Hettinger Date: Sat, 2 Feb 2019 10:16:50 -0800 Subject: [PATCH 07/10] Fix refcnt --- Modules/_functoolsmodule.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index 772e5ba21bddc5..47569e7823aa21 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -1006,10 +1006,13 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds Py_DECREF(link); return NULL; } + Py_INCREF(result); /* for return */ if (!IS_USED(link)) { lru_cache_append_link(self, link); } - Py_INCREF(result); /* for return */ + else { + Py_DECREF(link); + } return result; } /* Since the cache is full, we need to evict an old key and add From 34dbfa0b93a779d264cc2444cbee29a4a21c8486 Mon Sep 17 00:00:00 2001 From: Raymond Hettinger Date: Sat, 2 Feb 2019 11:28:39 -0800 Subject: [PATCH 08/10] Add more extensive comments --- Modules/_functoolsmodule.c | 63 ++++++++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 20 deletions(-) diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index 47569e7823aa21..3c55d2ebae6fe6 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -940,6 +940,11 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds } link = (lru_list_elem *)_PyDict_GetItem_KnownHash(self->cache, key, hash); if (link != NULL && IS_USED(link)) { + /* Either the __hash__ or __eq__ call can let another thread + also get to this same link, so we need to move the link + atomically (extract and append with in one-step guarded by + the GIL). The only works if some other thread has not + already detached the link. */ lru_cache_extract_link(link); lru_cache_append_link(self, link); result = link->result; @@ -958,6 +963,10 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds Py_DECREF(key); return NULL; } + /* We now hold references to a key and a result. The hash value is known. + After the callback to the user function, we no longer know anything + about the contents of the cache, so we have to check to see whether + an equivalent key has already been added. */ testresult = _PyDict_GetItem_KnownHash(self->cache, key, hash); if (testresult != NULL) { /* Getting here means that this same key was added to the cache @@ -967,18 +976,24 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds return result; } if (PyErr_Occurred()) { - /* This is an unusual case since this same lookup - did not previously trigger an error during lookup. - Treat it the same as an error in user function - and return with the error set. */ + /* This is an unusual case since this same lookup did not + previously trigger an error during lookup. Treat it the same + as an error in user function and return with the error + set. */ Py_DECREF(key); Py_DECREF(result); return NULL; } - /* This is the normal case. The new key wasn't found before - user function call and it is still not there. So we - proceed normally and update the cache with the new result. */ - + /* This is the normal case. The new key wasn't found before the user + function call and it is still not there. So we proceed normally + and update the cache with the new result. + + The dict lookup can trigger arbitrary code if there is an __eq__ + call (this only occurs when the full hash value exactly matches + that for another key). That means that we don't really know + the key is still missing. XXX Could state flag for the cache + detect this exotic event? + */ assert(self->maxsize > 0); if (PyDict_GET_SIZE(self->cache) < self->maxsize || self->root.next == &self->root) @@ -991,28 +1006,36 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds Py_DECREF(result); return NULL; } - MARK_UNUSED(link); - - link->hash = hash; - link->key = key; - link->result = result; /* What is really needed here is a SetItem variant with a "no clobber" option. If the __eq__ call triggers a reentrant call that adds - this same key, then this setitem call will update the cache dict + this same key, then this SetItem call will update the cache dict with this new link, leaving the old link as an orphan (i.e. not - having a cache dict entry that refers to it). */ + having a cache dict entry that refers to it). + + The link is marked as unused because the don't want to add it + to the doubly-linked list until we know that updating the + cache dict was successful. Marking it as unused prevents + other threads from using this link before all of its fields + are valid. + */ + MARK_UNUSED(link); if (_PyDict_SetItem_KnownHash(self->cache, key, (PyObject *)link, hash) < 0) { Py_DECREF(link); return NULL; } - Py_INCREF(result); /* for return */ - if (!IS_USED(link)) { - lru_cache_append_link(self, link); - } - else { + if (IS_USED(link)) { + /* XXX If we can prove this case can't happen, turn this test + into an assertion. */ + Py_DECREF(key); Py_DECREF(link); + return result; } + Py_INCREF(result); /* for return */ + link->hash = hash; + link->key = key; + link->result = result; + lru_cache_append_link(self, link); return result; } /* Since the cache is full, we need to evict an old key and add From b75e4ccd90c81958b024420b4226afb80a061d1d Mon Sep 17 00:00:00 2001 From: Raymond Hettinger Date: Sat, 2 Feb 2019 12:23:07 -0800 Subject: [PATCH 09/10] Note differences from the pure Python version --- Modules/_functoolsmodule.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index 3c55d2ebae6fe6..61f652ea2f8336 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -661,6 +661,25 @@ sequence is empty."); /* lru_cache object **********************************************************/ +/* There are four principal algorithmic differences from the pure python version: + + 1). The C version relies on the GIL instead of having its own reentrant lock. + + 2). The prev/next link fields use borrowed references. + + 3). For a full cache, the pure python version rotates the location of the + root entry so that it never has to move individual links and it can + limit updates to just the key and result fields. However, in the C + version, links are temporarily removed while the cache dict updates are + occurring. Afterwards, they are appended or prepended back into the + doubly-linked lists. + + 4) In the Python version, a _HashSeq class is used to prevent __hash__ + from being called more than once. In the C version, the "known hash" + variants of dictionary calls as used to the same effect. + +*/ + /* this object is used delimit args and keywords in the cache keys */ static PyObject *kwd_mark = NULL; From 997d521883cd9f74b3d85a65644e87ef9313a282 Mon Sep 17 00:00:00 2001 From: Raymond Hettinger Date: Sat, 2 Feb 2019 12:28:01 -0800 Subject: [PATCH 10/10] Note doubts about a comment's veracity --- Modules/_functoolsmodule.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index 61f652ea2f8336..01cf8fa7832994 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -1085,7 +1085,11 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds lru_cache_extract_link(link); /* Remove it from the cache. The cache dict holds one reference to the link, - and the linked list holds yet one reference to it. */ + and the linked list holds yet one reference to it. + + XXX Double check this statement. The linked list + is documented above to only have borrowed references. + */ popresult = _PyDict_Pop_KnownHash(self->cache, link->key, link->hash, Py_None); if (popresult == Py_None) {