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

bpo-35780: Fix errors in lru_cache() C code #11623

Merged
merged 30 commits into from Jan 26, 2019

Conversation

rhettinger
Copy link
Contributor

@rhettinger rhettinger commented Jan 20, 2019

rhettinger added 4 commits Jan 19, 2019
…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)
rhettinger added 4 commits Jan 20, 2019
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.
@rhettinger rhettinger removed the performance Performance or resource usage label Jan 20, 2019
rhettinger added 2 commits Jan 20, 2019
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).
@tirkarthi
Copy link
Member

tirkarthi commented Jan 20, 2019

Backporting to 3.6 would require approval from @ned-deily

rhettinger added 2 commits Jan 20, 2019
Limit to just common scalar types to make the space saving
technique easier to reason about (easier to show correctness).
rhettinger added 9 commits Jan 22, 2019
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.
@rhettinger rhettinger merged commit d8080c0 into python:master Jan 26, 2019
@miss-islington
Copy link
Contributor

miss-islington commented Jan 26, 2019

Thanks @rhettinger for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒🤖

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>
@bedevere-bot
Copy link

bedevere-bot commented Jan 26, 2019

GH-11682 is a backport of this pull request to the 3.7 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants