Bedrock performance tooling and initial optimisations

[stevejalim]

This changeset adds support for profiling Bedrock using django-silk locally (or anywhere the bedrock_test image is used - but not in production).

It also contains some optimisations - via cacheing - to reduce the DB queries executed on the busiest pages.

Significant changes and points to review

Please revew this PR commit by commit, paying sceptical attention to the usage of cacheing etc. They contain details of the number queries saved.

I know that at the moment Bedrock is using a verson of Django's LocMemCache backend - this means that for 45 pods in production, we'll still get plenty of cache misses until the pods have all had a call that warms the cache. It might be that, given the TTL of the cached items, we never really get to that point in prod, but we will in Dev and Stage where there are far fewer pods.

We'd certainly get more cacheing uplift if we had a shared cache backend, such as Redis. Given we now have Redis in play for the rq backend, we could switch it at this point (expanding this PR), or as a separate change - opinions welcome!

Issue / Bugzilla link

#15505

Testing

Unit tests passing should be enough here, but feel free to follow the notes in profiling/hit_popular_pages.py to test drive things yourself.

Questions

Is the addition of django-silk work mentioning in formal documentation?

[codecov]

Codecov Report

Attention: Patch coverage is 90.47619% with 6 lines in your changes missing coverage. Please review.

Project coverage is 79.13%. Comparing base (61085d4) to head (dbd3748).

Files with missing lines	Patch %	Lines
bedrock/settings/base.py	44.44%	5 Missing ⚠️
bedrock/urls.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15513      +/-   ##
==========================================
+ Coverage   79.06%   79.13%   +0.06%     
==========================================
  Files         160      160              
  Lines        8355     8411      +56     
==========================================
+ Hits         6606     6656      +50     
- Misses       1749     1755       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stevejalim]

Talking with @bkochendorfer, we're both OK with the idea of using Redis for the Web deployments too (the same Redis we use for the CMS task queues because there's little real-world risk of task eviction if Redis gets low on memory). As such, I will follow this PR with another one focused on using Redis as an FE cache backend.

@pmac @robhudson Are you aware of any issues that are likley switching from LocMem to Redis for a cache - is there anywhere where we specifically exploit the local nature of it?

[pmac]

I don't know of any issues specific to locmem other than this will be a lot slower. I am a little concerned about using the same redis for cache and queue. We are doing this with basket, but that's a much lower volume. My understanding is that you'd use very different settings in redis for a cache vs. queue.

[stevejalim]

I don't know of any issues specific to locmem other than this will be a lot slower. I am a little concerned about using the same redis for cache and queue. We are doing this with basket, but that's a much lower volume. My understanding is that you'd use very different settings in redis for a cache vs. queue.

I hear you. I'm happy to try these cache uses based on LocMemCache for now and see what the uplift is like. My leaning towards a shared cache was to reduce cache misses from pods with cold local-memory caches, but doing things one step as a time is good - may not need to use a shared cache once the pods are warm.

[robhudson]

Should we cache the queryset or cache the list of objects?

[stevejalim]

Ideally yeah, we'd cache the objects, not the qs, but it's a bit more fiddly than I expected.

Because these are CBVs, cacheing the queryset in get_queryset is easier to do (and to discover) than cacheing the Position objects themselves, because there's no clean way to slot in cacheing when the view sets the object_list attribute that I can see

Later, even though we don't use pagination in these pages, the list view runs the data through paginate_queryset() which currently (Django 4.2.x) takes a queryset arg but treats it as a general iterable -- all of which means we could make get_queryset return a list of objects (from the cache), not a QuerySet, but it kind of muddies things.

What do you think?

[robhudson]

Thanks for sharing that. It's a bit more complex than first glance. I'm actually a bit surprised we can cache the queryset objects, which seems more complex. I suppose they get pickled? But this is fine with me, I was mostly curious about your thoughts on the consideration. Thanks!

[robhudson]

for bonus points you could test that the db call doesn't get triggered on a 2nd call to the wrapped methods.

[stevejalim]

Yeah - good call. Time to break out assertNumQueries!

[robhudson]

r+wc