For the nearly 10 years I’ve been using Django and the 5 years I’ve been using Wagtail CMS, I’ve always wanted to contribute to open source. I’ve previously tried with a micro-optimisation in Django but it was rightly rejected. After first considering a monkey patch, I’ve finally made my first contribution.

Back in June I moved the Django caches on my employer project (Corsair Explorer) from a self-hosted Redis box onto ElastiCache Serverless and pointed some django.core.cache.backends.locmem.LocMemCache caches at the new ElastiCache service. The initial box was a basic t3a.small AWS instance that was also hosting an Elasticsearch instance.

Response times got worse after the changeovers but the extra reliability / worker memory savings it gave us made it seem an overall positive change. My assumption was the new network latency was causing the performance regression.

A couple weeks ago I couldn’t shake the feeling of something being off. Even with extra network latency, the better cache hit ratio should be making up for that on the averages. Thankfully we push our ALB access logs into ClickHouse, so there were a few hundred million rows to query rather than going on vibes. Using Claude to parse and summarise the data I was left with this:

  p50 p90 p99
before 78ms 163ms 288ms
after 95ms 209ms 373ms

We were still serving requests quicker than our targets but it could be faster.

My initial guess was image renditions as I’ve seen these take up a large amount of queries in Streamfields in the past using Django Debug Toolbar. Wagtail caches them, most of our pages have a lot of images, and we’d swapped a near instant in-process cache for a TLS connection to another host. I patched the cache backend locally and saw some pages were doing 4 reads against 40 writes.

Why

Pointing Claude at the Wagtail AbstractImage source code quickly led to the issue.

Here is get_rendition() in wagtail/images/models.py:

try:
    rendition = self.find_existing_rendition(filter)
except Rendition.DoesNotExist:
    rendition = self.create_rendition(filter)
    # Reuse this rendition if requested again from this object
    self._add_to_prefetched_renditions(rendition)

cache_key = Rendition.construct_cache_key(
    self, filter.get_cache_key(self), filter.spec
)
Rendition.cache_backend.set(cache_key, rendition)

The set() at the bottom isn’t conditional on anything. It runs for every image, regardless of where the rendition came from.

get_renditions(), the plural version a bit further down the same file, does this instead:

# Update the cache
cache_additions = {
    Rendition.construct_cache_key(
        self, filter.get_cache_key(self), filter.spec
    ): rendition
    for filter, rendition in renditions.items()
    # prevent writing of cached data back to the cache
    if not getattr(rendition, "_from_cache", False)
}
if cache_additions:
    Rendition.cache_backend.set_many(cache_additions)

There’s a _from_cache flag, and the plural method checks it before writing. The singular one, which is what the {% image %} tag ends up calling, doesn’t.

The fix

A super simple change that mimics the plural version.

# prevent writing of cached data back to the cache
if not getattr(rendition, "_from_cache", False):
    cache_key = Rendition.construct_cache_key(
        self, filter.get_cache_key(self), filter.spec
    )
    Rendition.cache_backend.set(cache_key, rendition)

While getting Claude to write unit tests after reviewing my PR, Matt Westcott spotted another issue:

Thanks @MasonLyons! Getting Claude to write unit tests for this has revealed a secondary issue - the _from_cache flag was only set when the result came from a prefetch query, not when it actually came from the cache. I’ll combine that fix (+ tests) with this one and merge.

So the check in get_renditions() had never worked for cache hits. Thankfully the docs tell you to do prefetch_related("thumbnail__renditions"), so we were safe from the issue Matt fixed. But it does explain why there was such a large imbalance between cache reads and writes. Prefetching means the renditions are already in memory so the cache never gets read, but still written to.

My PR, merged as c1c39fe, should be going out in version 8.1.

Reflection

In a world full of vibe coding and AI slop it’s easy to just hand an LLM the keys and let it go to town. But while Claude very likely could have found the issue itself eventually, even with the ever increasing number of bots trying to spam PRs on open source projects and Wagtail being popular, it still needed a human touch to find it and fix it. LLMs still feel like a floor raiser and force multiplier. I had to steer it in roughly the right direction and Matt only got it to spot the issues when using it to write unit tests.

Thanks to Matt Westcott for being so quick with reviewing the initial PR.

<
Previous Post
How to add a custom language to Django
>
Blog Archive
Archive of all previous blog posts