bpo-35780: Fix errors in lru_cache() C code by rhettinger · Pull Request #11623

added 4 commits

January 18, 2019 17:55

…k->next references.

This saves an unnecessary duplicate lookup.

Clang assembly before:

    movq    16(%rax), %rcx      # link->prev
    movq    24(%rax), %rdx      # link->next
    movq    %rdx, 24(%rcx)      # link->prev->next = link->next;
    movq    24(%rax), %rdx      # duplicate fetch of link->next
    movq    %rcx, 16(%rdx)      # link->next->prev = link->prev;

Clang assembly after:

    movq    16(%rax), %rcx
    movq    24(%rax), %rdx
    movq    %rdx, 24(%rcx)
    movq    %rcx, 16(%rdx)

was incorrectly moved to the newest position as if the user
had made a recent call with this key.   The fix is to restore
it the oldest position, keeping the LRU invariant where keys
are tracked by recency of access.

Formerly, the code allowed cache to fall into an inconsistent
state.  Now, there are no code paths that have a full cache
but no links.

Also move decrefs to the end of end path to make it easier to
verify that there are no reentrant calls before the cache
invariants have been restored.

Save hit update for last (as the pure python version does).

Limit to just common scalar types to make the space saving
technique easier to reason about (easier to show correctness).

Since the final setitem is potentially reentrant, we have to reset
the full status prior to the setitem call (since we've already
removed a link and its associated cache dict entry).

After the setitem call, we cannot know whether some other thread
has reset the status, so we cannot just restore it without checking
to make sure the number of dict entries is at maxsize.

Negative maxsize was being treated as a cache size of 1
giving an almost 100% miss rate while still incurring
the overhead of cache checking and eviction.

The negative maxsize also showed-up in CacheInfo even though
it was non-sensical to have a negative maxsize.

The negative maxsize also made it into the struct for the C version.
This caused erroneous results for the calculation of the "full" flag.

It is cheaper and more reliable to make on-demand checks for
whether the cache is full than it is to recompute and occasionally
toggle a persistent state variable.

…ource of truth.

…ad of else clauses

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jan 26, 2019

(cherry picked from commit d8080c0)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>

rhettinger pushed a commit that referenced this pull request

Jan 26, 2019

Closed

Merged

bpo-35780: Fix errors in lru_cache() C code by rhettinger · Pull Request #11623 · python/cpython