Performance: scope treenode's cache keys per model - #234
Open
stumpylog wants to merge 3 commits into
Open
Conversation
Every treenode model in an app shared two cache keys
("treenode_list"/"treenode_dict"), each a defaultdict keyed by model
class -- so clearing or repopulating one model's cache did a full
get+set round-trip of every other model's cached rows too, on an
out-of-process cache backend that means re-pickling and re-transmitting
unrelated models' data on every write to any one model.
Give each model its own cache keys instead
(f"treenode_{suffix}:{cls._meta.label_lower}"), and drop the
defaultdict-of-all-models layer entirely since each entry now belongs
to exactly one model. clear_cache() also gets cheaper: two plain
cache.delete() calls instead of a read-modify-write of the combined
structure.
No public API change, no migration needed (the cache is a fully
derived, rebuildable artifact).
Assert the dict key is isolated too, not just the list key (an implementation that isolated one but not the other would have passed before), and add a tearDown so the test doesn't leak a cache entry that happened to only be overwritten by test_fixtures.py's loaddata running later in the suite. From the final whole-branch review of this branch.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #234 +/- ##
==========================================
- Coverage 95.07% 95.07% -0.01%
==========================================
Files 11 11
Lines 711 710 -1
==========================================
- Hits 676 675 -1
Misses 35 35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Every treenode model in an app shared two cache keys (
"treenode_list"/"treenode_dict"), each adefaultdictkeyed by model class. Clearing or repopulating one model's cache did a full get+set round-trip of that combined structure, so a write to model A re-pickled and re-transmitted model B's cached rows too, on a real out-of-process cache backend, even though B never changed.Give each model its own cache keys instead (
f"treenode_{suffix}:{cls._meta.label_lower}"), and drop the defaultdict-of-all-models layer since each entry now belongs to exactly one model.Measured (two treenode models, one with 2000 cached rows, 20 writes to the other):
Same query count, less than half the time: pure Python/pickling overhead from no longer touching an unrelated model's cached data on every write. Even a single-model app sees a smaller but real win (28% faster on 50 sequential writes into a 300-row table, again at identical query counts).
No public API change, no migration needed (the cache is a fully derived, rebuildable artifact; old keys just go stale under the new scheme).
Related issue
Found while investigating the whole-table-recompute discussion in #45.
Checklist before requesting a review