From 51ae3a2491bbb249c2cdf209a7041f61c01c6e55 Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Thu, 6 Aug 2026 01:23:25 +0000 Subject: [PATCH] perf(question-feed): memoize contested-card lookups within a single feed 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. --- MPCAutofill/cardpicker/question_feed.py | 32 ++++++++++-- .../cardpicker/tests/test_question_feed.py | 49 ++++++++++++++++++- 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/MPCAutofill/cardpicker/question_feed.py b/MPCAutofill/cardpicker/question_feed.py index 02b25f7e7..3b4f9202d 100644 --- a/MPCAutofill/cardpicker/question_feed.py +++ b/MPCAutofill/cardpicker/question_feed.py @@ -473,6 +473,8 @@ def _tier_2_contested( answered_artist_card_ids: Optional[set[int]] = None, answered_tag_card_ids_by_tag: Optional[dict[str, set[int]]] = None, not_official_art_card_ids: Optional[set[int]] = None, + contested_card_ids: Optional[list[int]] = None, + contested_artist_card_ids: Optional[list[int]] = None, ) -> Optional[tuple[QuestionFeedItem, str]]: if answered_card_ids is None: answered_card_ids = _voter_answered_printing_card_ids(anonymous_id) @@ -482,9 +484,13 @@ def _tier_2_contested( answered_tag_card_ids_by_tag = _voter_answered_tag_card_ids_by_tag(anonymous_id) if not_official_art_card_ids is None: not_official_art_card_ids = _not_official_art_card_ids() + if contested_card_ids is None: + contested_card_ids = get_contested_card_ids() + if contested_artist_card_ids is None: + contested_artist_card_ids = get_contested_artist_card_ids() printing_card = ( - Card.objects.filter(printing_tag_status=PrintingTagStatus.UNRESOLVED, pk__in=get_contested_card_ids()) + Card.objects.filter(printing_tag_status=PrintingTagStatus.UNRESOLVED, pk__in=contested_card_ids) .exclude(pk__in=answered_card_ids) .order_by("-date_created") .first() @@ -493,7 +499,7 @@ def _tier_2_contested( return _identify_printing_item(printing_card), "tier_2_contested_printing" artist_card = ( - Card.objects.filter(artist_vote_status=ArtistVoteStatus.CONTESTED, pk__in=get_contested_artist_card_ids()) + Card.objects.filter(artist_vote_status=ArtistVoteStatus.CONTESTED, pk__in=contested_artist_card_ids) .exclude(pk__in=answered_artist_card_ids) .exclude(pk__in=not_official_art_card_ids) .order_by("-date_created") @@ -537,6 +543,7 @@ def _tier_4_fresh( anonymous_id: str, answered_card_ids: Optional[set[int]] = None, not_official_art_card_ids: Optional[set[int]] = None, + contested_card_ids: Optional[list[int]] = None, ) -> Optional[tuple[QuestionFeedItem, str]]: # named "tier 4" (not renumbered to 3) even though moderation's former tier 3 was removed # (see module docstring) - keeps this name stable against every docstring/test/comment @@ -563,9 +570,11 @@ def _tier_4_fresh( answered_card_ids = _voter_answered_printing_card_ids(anonymous_id) if not_official_art_card_ids is None: not_official_art_card_ids = _not_official_art_card_ids() + if contested_card_ids is None: + contested_card_ids = get_contested_card_ids() printing_card = ( Card.objects.filter(printing_tag_status=PrintingTagStatus.UNRESOLVED) - .exclude(pk__in=get_contested_card_ids()) + .exclude(pk__in=contested_card_ids) .exclude(pk__in=answered_card_ids) .annotate(vote_count=Count("printing_tags", distinct=True)) .annotate(origin_reason=_latest_stage_d_origin_reason_subquery()) @@ -679,18 +688,33 @@ def get_next_question_feed_item(anonymous_id: str) -> Optional[QuestionFeedItem] if tier_1_item is not None: return _log_served(anonymous_id, tier_1_item, QuestionFeedServedPool.REMAINDER, "tier_1_confirm_suggestion") + # `contested_card_ids`/`contested_artist_card_ids` are resolved ONCE here, only once we've + # actually fallen through to the tiers that consult them (tier 1 and the likely-resolve pool + # above never touch either), and reused by both tier 2 and tier 4 below - each is otherwise + # identical on repeat calls within this same request (no vote can be cast mid-request), so + # recomputing it once per tier just paid the same cost twice for one answer. + contested_card_ids = get_contested_card_ids() + contested_artist_card_ids = get_contested_artist_card_ids() + tier_2_result = _tier_2_contested( anonymous_id, answered_card_ids, answered_artist_card_ids=answered_artist_card_ids, answered_tag_card_ids_by_tag=answered_tag_card_ids_by_tag, not_official_art_card_ids=not_official_art_card_ids, + contested_card_ids=contested_card_ids, + contested_artist_card_ids=contested_artist_card_ids, ) if tier_2_result is not None: tier_2_item, tier_2_reason = tier_2_result return _log_served(anonymous_id, tier_2_item, QuestionFeedServedPool.REMAINDER, tier_2_reason) - tier_4_result = _tier_4_fresh(anonymous_id, answered_card_ids, not_official_art_card_ids=not_official_art_card_ids) + tier_4_result = _tier_4_fresh( + anonymous_id, + answered_card_ids, + not_official_art_card_ids=not_official_art_card_ids, + contested_card_ids=contested_card_ids, + ) if tier_4_result is not None: tier_4_item, tier_4_reason = tier_4_result return _log_served(anonymous_id, tier_4_item, QuestionFeedServedPool.REMAINDER, tier_4_reason) diff --git a/MPCAutofill/cardpicker/tests/test_question_feed.py b/MPCAutofill/cardpicker/tests/test_question_feed.py index 9ace9d2ef..7d5025564 100644 --- a/MPCAutofill/cardpicker/tests/test_question_feed.py +++ b/MPCAutofill/cardpicker/tests/test_question_feed.py @@ -1,7 +1,12 @@ +from unittest.mock import patch + from django.urls import reverse from cardpicker import views -from cardpicker.artist_consensus import resolve_and_persist_artist +from cardpicker.artist_consensus import ( + get_contested_artist_card_ids, + resolve_and_persist_artist, +) from cardpicker.local_calculate_verdicts import ( JOIN_KEY_ANONYMOUS_ID, JOIN_KEY_UNKNOWN_SET_CODE_SKIP_REASON, @@ -17,7 +22,10 @@ VotePolarity, VoteSource, ) -from cardpicker.printing_consensus import resolve_and_persist_printing +from cardpicker.printing_consensus import ( + get_contested_card_ids, + resolve_and_persist_printing, +) from cardpicker.question_feed import ( _artist_item, _scryfall_illustration_url, @@ -213,6 +221,43 @@ def test_own_vote_exclusion_is_scoped_to_the_specific_tag_not_the_whole_card(sel assert item.tagName == tag_b.name +class TestContestedIdsMemoizedPerRequest: + """`get_contested_card_ids`/`get_contested_artist_card_ids` are expensive (issue #726: + 330-400ms each on production data) and, before this fix, were recomputed once per tier that + consulted them instead of once per `get_next_question_feed_item` call.""" + + def test_get_contested_card_ids_computed_once_when_tier_2_and_tier_4_are_both_consulted(self, db): + # a plain unresolved card with no votes: tier 2 finds nothing contested and falls + # through to tier 4, which - before this fix - called get_contested_card_ids() a + # second time for the same answer within the same request + card = CardFactory(printing_tag_status=PrintingTagStatus.UNRESOLVED) + + with ( + patch("cardpicker.question_feed.get_contested_card_ids", wraps=get_contested_card_ids) as mock_contested, + patch( + "cardpicker.question_feed.get_contested_artist_card_ids", wraps=get_contested_artist_card_ids + ) as mock_contested_artist, + ): + item = get_next_question_feed_item("anon-1") + + assert item is not None + assert item.card.identifier == card.identifier + assert mock_contested.call_count == 1 + assert mock_contested_artist.call_count == 1 + + def test_get_contested_card_ids_not_called_when_tier_1_serves_the_item(self, db): + # tier 1 (confirm_suggestion) and the likely-resolve pool both resolve before tier 2 is + # ever reached, so neither contested-ids function should run at all + make_ai_suggested_card() + + with patch("cardpicker.question_feed.get_contested_card_ids", wraps=get_contested_card_ids) as mock_contested: + item = get_next_question_feed_item("anon-1") + + assert item is not None + assert item.type.value == "confirm_suggestion" + assert mock_contested.call_count == 0 + + class TestPhaseCNotOfficialArtRouting: """ 2026-08-04 gate on the phase-C/md5 routing brief (item 2): a card carrying a positive,