Skip to content

0.15.x #139

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

Closed
wants to merge 3 commits into from
Closed

0.15.x #139

wants to merge 3 commits into from

Conversation

masklinn
Copy link
Contributor

No description provided.

Amongst other changes, #113 switched the cache to a FIFO inspired by
the standard library's re module, however it didn't really take
concurrency in account, so didn't really consider: that double-pops
are possible (probably why the stdlib ignores a bunch of errors),
which can cause KeyError during lookup (as two workers try to clear
the first key, one succeeds, and the other doesn't find the key and
fails).

It also has a few other less major issues:

- double-inserts are possible, which can cause the cache to exceed set
  capacity permanently by the number of concurrent workers
- the stdlib's method only works properly with Python 3.6's naturally
  ordered `dict`, but I'd rather not drop 2.7 compatibility from 0.x
  unless there are very good causes to as, despite 2.7 having been
  EOL'd in 2020, it still accounts for more downloads than 3.10
  (according to pypistats)

Using an ordered dict would solve (3), and allow using an LRU rather
than a FIFO, but it would not actually prevent double-pops or
double-inserts, that would require a proper lock on lookup. Which
might not be that expensive but given the lack of a good dataset to
bench with, it seems a lot of additional complexity for something
we've got no visibility on. But that can be considered if someone
reports a serious performance regression from this.

So for now just revert to a "reset" cache replacement policy. If /
when we drop older versions we can switch to `functools.lru_cache` and
let the stdlib take care of this (and possibly have cache
stats). Alternatively if we get a good testing dataset one day we can
bench cache replacement policies or even provide pluggable policies.

Anyway fixes #132, closes #133
Fix for #132 on 0.15
@masklinn masklinn closed this Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant