[Django] #34681: Optimize memcache_key_warnings()
Django
------------------------------------------------+--------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+--------------------------
The cache functino memcache_key_warnings() iterates the key character-by-
character, a slow operation in Python because it has to create and destroy
many individual `str` objects. Instead, we can search the string with a
regular expression for a 6x speedup.
IPython session comparing old and new approaches:
{{{
In [1]: import re
...:
...: MEMCACHE_MAX_KEY_LENGTH = 250
...:
...:
...: def old(key):
...: if len(key) > MEMCACHE_MAX_KEY_LENGTH:
...: yield (
...: "Cache key will cause errors if used with memcached:
%r "
...: "(longer than %s)" % (key, MEMCACHE_MAX_KEY_LENGTH)
...: )
...: for char in key:
...: if ord(char) < 33 or ord(char) == 127:
...: yield (
...: "Cache key contains characters that will cause
errors if "
...: "used with memcached: %r" % key
...: )
...: break
...:
...: memcached_error_chars_re = re.compile(r"[\x00-\x32\x127]")
...:
...:
...: def new(key):
...: if len(key) > MEMCACHE_MAX_KEY_LENGTH:
...: yield (
...: "Cache key will cause errors if used with memcached:
%r "
...: "(longer than %s)" % (key, MEMCACHE_MAX_KEY_LENGTH)
...: )
...: if memcached_error_chars_re.match(key):
...: yield (
...: "Cache key contains characters that will cause errors
if "
...: "used with memcached: %r" % key
...: )
...:
In [2]: %timeit list(old('acme-bookstore-user-1234567-book-78910391'))
1.35 µs ± 1.09 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops
each)
In [3]: %timeit list(new('acme-bookstore-user-1234567-book-78910391'))
212 ns ± 1.42 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops
each)
In [4]: %timeit list(old('homepage\n'))
545 ns ± 1.17 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops
each)
In [5]: %timeit list(new('homepage\n'))
209 ns ± 1.03 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops
each)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34681>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Adam
Type: | Johnson
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Adam Johnson):
* owner: nobody => Adam Johnson
--
Ticket URL: <https://code.djangoproject.com/ticket/34681#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Adam
Type: | Johnson
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
New description:
The cache functino memcache_key_warnings() iterates the key character-by-
character, a slow operation in Python because it has to create and destroy
many individual `str` objects. Instead, we can search the string with a
regular expression for a ~3x speedup on a reasonably sized error-free
cache key.
IPython session comparing old and new approaches:
{{{
In [1]: import re
...:
...: MEMCACHE_MAX_KEY_LENGTH = 250
...:
...:
...: def old(key):
...: if len(key) > MEMCACHE_MAX_KEY_LENGTH:
...: yield (
...: "Cache key will cause errors if used with memcached:
%r "
...: "(longer than %s)" % (key, MEMCACHE_MAX_KEY_LENGTH)
...: )
...: for char in key:
...: if ord(char) < 33 or ord(char) == 127:
...: yield (
...: "Cache key contains characters that will cause
errors if "
...: "used with memcached: %r" % key
...: )
...: break
...:
...: memcached_error_chars_re = re.compile(r"[\x00-\x20\x7f]")
...:
...:
...: def new(key):
...: if len(key) > MEMCACHE_MAX_KEY_LENGTH:
...: yield (
...: "Cache key will cause errors if used with memcached:
%r "
...: "(longer than %s)" % (key, MEMCACHE_MAX_KEY_LENGTH)
...: )
...: if memcached_error_chars_re.search(key):
...: yield (
...: "Cache key contains characters that will cause errors
if "
...: "used with memcached: %r" % key
...: )
In [2]: %timeit list(old('acme-bookstore-user-1234567-book-78910391'))
1.35 µs ± 1.05 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops
each)
In [3]: %timeit list(new('acme-bookstore-user-1234567-book-78910391'))
475 ns ± 1.37 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops
each)
In [4]: %timeit list(old('homepage\n'))
546 ns ± 2.88 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops
each)
In [5]: %timeit list(new('homepage\n'))
413 ns ± 1.35 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops
each)
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/34681#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Adam
Type: | Johnson
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Agreed. I'd only use `_lazy_re_compile()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34681#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Adam
Type: | Johnson
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Adam Johnson):
Yup, done in the patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/34681#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Adam
Type: | Johnson
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/17019 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/34681#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Adam
Type: | Johnson
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"1dbcf9a005842c3fdd97537f1bd4fab5b96b972f" 1dbcf9a]:
{{{
#!CommitTicketReference repository=""
revision="1dbcf9a005842c3fdd97537f1bd4fab5b96b972f"
Fixed #34681 -- Optimized memcache_key_warnings().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34681#comment:6>