perf(question-feed): memoize contested-card lookups within a single feed request - #729
Merged
Merged
Conversation
…eed request get_contested_card_ids() and get_contested_artist_card_ids() were each called separately by every tier that consulted them (_tier_2_contested, _tier_4_fresh), recomputing an identical answer within one get_next_question_feed_item() call - the same duplication shape #482 already fixed for the voter-exclusion sets. Both are now resolved once, right before the tier-2 fallthrough (so a request that resolves via the likely-resolve pool or tier 1 never pays this cost), and threaded through as optional parameters, matching the existing convention for answered_card_ids et al. The tiers keep the parameter optional (default None, computed lazily if absent) so a direct caller - a test or a shell - can still invoke one tier by anonymous_id alone.
This was referenced Aug 6, 2026
WilfordGrimley
added a commit
that referenced
this pull request
Aug 6, 2026
…_card_ids across the view (#738) - _voter_answered_printing_card_ids now also reads CardIllustrationVote, not just CardPrintingTag. The illustration-cluster answer path (cast_illustration_vote, N>1 candidates sharing an illustration) writes only CardIllustrationVote, which the printing-tier exclusion could not see - the card stayed eligible and was immediately re-served to the same voter. - get_next_question_feed_item and get_remaining_estimate both accept an optional contested_card_ids parameter (extending PR #729's convention across the view boundary); views.get_question_feed resolves get_contested_card_ids() once per request and threads it into both, instead of each independently paying the ~520-627ms query cost.
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
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.
Summary
get_contested_card_ids()andget_contested_artist_card_ids()were each invoked separately by every tier that consulted them (_tier_2_contested,_tier_4_fresh), recomputing an identical answer within a singleget_next_question_feed_item()call - the same duplication shape Pool printing-consensus votes across md5 identity groups #482 already fixed for the voter-exclusion sets (answered_card_idset al). Both are now resolved once, right before the tier-2 fallthrough, and threaded through as optional parameters that default toNoneand compute lazily if absent, matching the existing convention exactly.get_next_question_feed_item(unlike theanswered_*sets, which are always needed): a request served by the likely-resolve pool or tier 1 never reaches tier 2/4, so it never pays this cost either way, before or after this change.get_contested_card_ids()520-562ms,get_contested_artist_card_ids()18-21ms (currently near-free since the contested-artist set is empty right now).question_feed.pyonly (keeping the new test) and confirmed the new memoization test fails against the old code withcall_count=2, then restored the fix and confirmed it passes withcall_count=1.get_remaining_estimate()(called by the view alongsideget_next_question_feed_item()for every/2/questionFeed/request) also callsget_contested_card_ids()once, already memoized to exactly one call within itself. That is a second, independent top-level call fromviews.pythis PR does not touch, following Pool printing-consensus votes across md5 identity groups #482's own precedent of scoping the memoization to withinget_next_question_feed_item()'s own call tree rather than spanning the view boundary - noted as an open item below.TestContestedIdsMemoizedPerRequestwith two cases: one provingget_contested_card_ids/get_contested_artist_card_idsare each called exactly once when a request falls through tier 2 into tier 4, and one proving neither is called at all when tier 1 serves the item.Separately, investigated (read-only, no index created) whether the four tier queries apply a LIMIT before evaluation, to settle whether a stopgap
date_createdindex is worth building:_likely_resolve_printing_card(question_feed.py:391-433) and_tier_1_confirm_suggestion(question_feed.py:448-467): no LIMIT/slice/.first()/.exists()- both iterate the full.order_by("date_created")queryset via.iterator(), stopping the Python loop early on a match, but the SQL itself has no LIMIT.EXPLAIN (ANALYZE, BUFFERS)against production confirms noLimitnode at all: anexternal mergedisk sort over the full pre-filtered set runs to completion (Sort Method: external merge, ~27-28MB per worker) before any row is available._tier_2_contested(question_feed.py:470-519) and_tier_4_fresh(question_feed.py:542-608): both use.first()on their printing/artist branches, which does addLIMIT 1. Confirmed viaEXPLAINon the actualqs[:1]Django executes: both plans have a top-levelLimitnode._tier_2_contested's printing branch, theLimitsits above a cheap in-memory top-N heapsort (not a disk sort) - thepk__in=contested_card_idsfilter (8,339 ids today) already narrows the row count enough that Postgres never spills to disk, Limit or not. An index would not meaningfully change this query._tier_4_fresh's printing branch, theLimitsits above aGroupAggregateover the full ~222k-row unresolved population, which must be fully computed - including a correlated per-rowCardScanLogsubquery fororigin_reason, executed 301,902 times in this run - before the final sort/limit can pick row 1. TheLimitcannot be pushed below the aggregation, so it buys nothing here: this specific run showed no external-merge disk sort (Postgres chose a merge-join plan instead this time), but the ~5.2s cost is dominated by the repeated correlated subquery, not a sort. Adate_createdindex would not fix this query's dominant cost either.date_createdindex would help_likely_resolve_printing_card/_tier_1_confirm_suggestion(letting Postgres produce rows in index order lazily, so the existing.iterator()early-exit can actually save wall-clock time instead of waiting on a full sort) but would not help_tier_2_contested(already cheap) or_tier_4_fresh(bottlenecked upstream of the sort, by the correlated subquery and aggregation, not by the sort itself).(printing_tag_status, date_created)vs plaindate_created: sinceprinting_tag_status = 'unresolved'matches 230,385 of 230,501 rows (99.9%), the composite's leading column adds essentially no selectivity a plaindate_createdindex doesn't already provide for this predicate - the two are practically equivalent for this workload. No index was created; snip this is a report only.Test plan
pytest cardpicker/tests/test_question_feed.py cardpicker/tests/test_artist_votes.py(84 passed) against an isolated test Postgres instance, not the production databasecall_count=2) and passes against the fix (call_count=1)python -m py_compileand the repo's pre-commit hooks (ruff, isort, black, mypy, prettier) all pass on the changed files