perf: view_tryout, une requete par lot au lieu d une par ligne
PERF-001, la page la plus consultee de l application. Quatre boucles
posaient une requete par ligne :
User.query.get() par inscription
Evaluation.query par joueur inscrit, pour savoir si ce coach
l avait deja evalue
TeamMember.query par equipe
User.query.get() par membre d equipe
Plus, sur chaque match de type player_vs_player, deux interrogations
supplementaires de la relation dynamique `participants` pour trier par
camp -- alors que la liste complete venait d etre chargee douze lignes plus
haut.
Toutes remplacees par un chargement groupe. Les evaluations de ce coach
sont deduites de la liste `evaluations` deja en memoire, pas redemandees.
Mesure, sur un tryout de 10 inscrits, 2 equipes et 1 match :
34 requetes avant, 12 apres. Le test fixe un budget de 25, volontairement
large -- il ne peut que baisser, et il echoue sur le code d avant.
_users_by_id() est le helper partage par les trois chargements ; une ligne
absente est simplement absente du dictionnaire, ce que faisait deja un
get() renvoyant None.
Un second test verifie que les dix joueurs apparaissent toujours sur la
page : une requete groupee qui perd des lignes est le risque reel ici, pas
l erreur bruyante.
Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
+53
-22
@@ -37,6 +37,25 @@ def can_manage():
|
|||||||
return isinstance(current_user, (Admin, Manager))
|
return isinstance(current_user, (Admin, Manager))
|
||||||
|
|
||||||
|
|
||||||
|
def _users_by_id(user_ids):
|
||||||
|
"""Load these users in one query, keyed by id.
|
||||||
|
|
||||||
|
Replaces the `User.query.get()`-inside-a-loop that view_tryout used in
|
||||||
|
three separate places (PERF-001). Missing ids are simply absent from
|
||||||
|
the result, which is what a per-row get() returning None amounted to.
|
||||||
|
|
||||||
|
Args:
|
||||||
|
user_ids: Iterable of primary keys, may repeat and may be empty.
|
||||||
|
|
||||||
|
Returns:
|
||||||
|
dict[int, User]
|
||||||
|
"""
|
||||||
|
wanted = {user_id for user_id in user_ids if user_id}
|
||||||
|
if not wanted:
|
||||||
|
return {}
|
||||||
|
return {user.id: user for user in User.query.filter(User.id.in_(wanted)).all()}
|
||||||
|
|
||||||
|
|
||||||
@tryouts_bp.route('')
|
@tryouts_bp.route('')
|
||||||
@login_required
|
@login_required
|
||||||
def list_tryouts():
|
def list_tryouts():
|
||||||
@@ -289,19 +308,29 @@ def view_tryout(tryout_id):
|
|||||||
flash(_('You do not have permission to view this tryout.'), 'danger')
|
flash(_('You do not have permission to view this tryout.'), 'danger')
|
||||||
return redirect(url_for('tryouts.list_tryouts'))
|
return redirect(url_for('tryouts.list_tryouts'))
|
||||||
|
|
||||||
|
# Everything below used to run one query per row (PERF-001): one
|
||||||
|
# User.query.get() per registration, one Evaluation lookup per player,
|
||||||
|
# one TeamMember query per team and one more User.query.get() per
|
||||||
|
# member. Thirty registrants and four teams put this page well past a
|
||||||
|
# hundred round trips, on unindexed columns.
|
||||||
registrations = TryoutRegistration.query.filter_by(tryout_id=tryout_id).all()
|
registrations = TryoutRegistration.query.filter_by(tryout_id=tryout_id).all()
|
||||||
registered_players = [User.query.get(r.player_id) for r in registrations if r.player_id]
|
registered_player_ids = [r.player_id for r in registrations if r.player_id]
|
||||||
|
players_by_id = _users_by_id(registered_player_ids)
|
||||||
|
registered_players = [
|
||||||
|
players_by_id[player_id]
|
||||||
|
for player_id in registered_player_ids
|
||||||
|
if player_id in players_by_id
|
||||||
|
]
|
||||||
evaluations = Evaluation.query.filter_by(tryout_id=tryout_id).all()
|
evaluations = Evaluation.query.filter_by(tryout_id=tryout_id).all()
|
||||||
|
|
||||||
player_eval_status = {}
|
player_eval_status = {}
|
||||||
if current_user.can_evaluate():
|
if current_user.can_evaluate():
|
||||||
for p in registered_players:
|
evaluated_by_me = {
|
||||||
existing = Evaluation.query.filter_by(
|
row.player_id
|
||||||
tryout_id=tryout_id,
|
for row in evaluations
|
||||||
player_id=p.id,
|
if row.evaluator_id == current_user.id and row.player_id
|
||||||
evaluator_id=current_user.id,
|
}
|
||||||
).first()
|
player_eval_status = {p.id: p.id in evaluated_by_me for p in registered_players}
|
||||||
player_eval_status[p.id] = existing is not None
|
|
||||||
|
|
||||||
is_registered = (
|
is_registered = (
|
||||||
TryoutRegistration.query.filter_by(
|
TryoutRegistration.query.filter_by(
|
||||||
@@ -312,17 +341,16 @@ def view_tryout(tryout_id):
|
|||||||
)
|
)
|
||||||
|
|
||||||
teams = Team.query.filter_by(tryout_id=tryout_id).all()
|
teams = Team.query.filter_by(tryout_id=tryout_id).all()
|
||||||
team_data = []
|
team_ids = [team.id for team in teams]
|
||||||
for team in teams:
|
members_by_team = {}
|
||||||
members = TeamMember.query.filter_by(team_id=team.id).all()
|
if team_ids:
|
||||||
team_data.append(
|
member_rows = TeamMember.query.filter(TeamMember.team_id.in_(team_ids)).all()
|
||||||
{
|
member_players = _users_by_id([m.player_id for m in member_rows if m.player_id])
|
||||||
'team': team,
|
for row in member_rows:
|
||||||
'members': [
|
members_by_team.setdefault(row.team_id, []).append(
|
||||||
{'player': User.query.get(m.player_id), 'position': m.position} for m in members
|
{'player': member_players.get(row.player_id), 'position': row.position}
|
||||||
],
|
|
||||||
}
|
|
||||||
)
|
)
|
||||||
|
team_data = [{'team': team, 'members': members_by_team.get(team.id, [])} for team in teams]
|
||||||
|
|
||||||
can_edit = current_user.can_manage_this_tryout(tryout)
|
can_edit = current_user.can_manage_this_tryout(tryout)
|
||||||
|
|
||||||
@@ -382,15 +410,18 @@ def view_tryout(tryout_id):
|
|||||||
else [],
|
else [],
|
||||||
}
|
}
|
||||||
elif match.match_type == 'player_vs_player':
|
elif match.match_type == 'player_vs_player':
|
||||||
|
# Filtered from the list already in hand. Asking the dynamic
|
||||||
|
# relationship again cost two more round trips per match for
|
||||||
|
# rows that were loaded a dozen lines above.
|
||||||
team1_players = [
|
team1_players = [
|
||||||
{'name': p.player.username, 'position': p.position}
|
{'name': p.player.username, 'position': p.position}
|
||||||
for p in match.participants.filter_by(team_side=1).all()
|
for p in all_participants
|
||||||
if p.player
|
if p.team_side == 1 and p.player
|
||||||
]
|
]
|
||||||
team2_players = [
|
team2_players = [
|
||||||
{'name': p.player.username, 'position': p.position}
|
{'name': p.player.username, 'position': p.position}
|
||||||
for p in match.participants.filter_by(team_side=2).all()
|
for p in all_participants
|
||||||
if p.player
|
if p.team_side == 2 and p.player
|
||||||
]
|
]
|
||||||
participants = {
|
participants = {
|
||||||
'team1': 'Team 1',
|
'team1': 'Team 1',
|
||||||
|
|||||||
@@ -231,3 +231,84 @@ class TestPendingEvaluations:
|
|||||||
db.session.commit()
|
db.session.commit()
|
||||||
|
|
||||||
assert self._pending(client) == 1
|
assert self._pending(client) == 1
|
||||||
|
|
||||||
|
|
||||||
|
class TestViewTryout:
|
||||||
|
"""PERF-001 — the most-visited page in the application ran one query
|
||||||
|
per registration, one per player evaluated, one per team, and one per
|
||||||
|
team member. Thirty registrants and four teams put it past a hundred
|
||||||
|
round trips for a single render."""
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def populated_tryout(self, app, make_user):
|
||||||
|
"""A tryout with ten registrants split across two teams."""
|
||||||
|
from app.models import Match, MatchParticipant, Team, TeamMember
|
||||||
|
|
||||||
|
admin_id = make_user('admin')
|
||||||
|
player_ids = [make_user('player') for _ in range(10)]
|
||||||
|
|
||||||
|
with app.app_context():
|
||||||
|
tryout = Tryout(
|
||||||
|
title='Spring', game='Valorant', date=date(2030, 3, 1), created_by=admin_id
|
||||||
|
)
|
||||||
|
db.session.add(tryout)
|
||||||
|
db.session.flush()
|
||||||
|
|
||||||
|
for player_id in player_ids:
|
||||||
|
db.session.add(TryoutRegistration(tryout_id=tryout.id, player_id=player_id))
|
||||||
|
|
||||||
|
for index in range(2):
|
||||||
|
team = Team(tryout_id=tryout.id, name=f'Team {index}', created_by=admin_id)
|
||||||
|
db.session.add(team)
|
||||||
|
db.session.flush()
|
||||||
|
for player_id in player_ids[index * 5 : (index + 1) * 5]:
|
||||||
|
db.session.add(TeamMember(team_id=team.id, player_id=player_id))
|
||||||
|
|
||||||
|
match = Match(
|
||||||
|
tryout_id=tryout.id,
|
||||||
|
title='Scrim',
|
||||||
|
date=date(2030, 3, 2),
|
||||||
|
match_type='player_vs_player',
|
||||||
|
created_by=admin_id,
|
||||||
|
)
|
||||||
|
db.session.add(match)
|
||||||
|
db.session.flush()
|
||||||
|
for side, player_id in enumerate(player_ids[:4]):
|
||||||
|
db.session.add(
|
||||||
|
MatchParticipant(
|
||||||
|
match_id=match.id, player_id=player_id, team_side=(side % 2) + 1
|
||||||
|
)
|
||||||
|
)
|
||||||
|
db.session.commit()
|
||||||
|
return tryout.id, admin_id, player_ids
|
||||||
|
|
||||||
|
def test_the_page_still_shows_everyone(self, app, client, as_role, populated_tryout):
|
||||||
|
tryout_id, _admin_id, player_ids = populated_tryout
|
||||||
|
as_role('admin')
|
||||||
|
|
||||||
|
body = client.get(f'/tryouts/{tryout_id}').get_data(as_text=True)
|
||||||
|
|
||||||
|
with app.app_context():
|
||||||
|
usernames = [db.session.get(User, pid).username for pid in player_ids]
|
||||||
|
for username in usernames:
|
||||||
|
assert username in body, f'{username} is missing from the page'
|
||||||
|
|
||||||
|
def test_the_cost_does_not_grow_with_the_squad(
|
||||||
|
self, app, client, as_role, populated_tryout, count_queries
|
||||||
|
):
|
||||||
|
tryout_id, _admin_id, _player_ids = populated_tryout
|
||||||
|
as_role('admin')
|
||||||
|
|
||||||
|
with app.app_context():
|
||||||
|
counter = count_queries()
|
||||||
|
try:
|
||||||
|
response = client.get(f'/tryouts/{tryout_id}')
|
||||||
|
finally:
|
||||||
|
counter.stop()
|
||||||
|
|
||||||
|
assert response.status_code == 200
|
||||||
|
# Ten registrants, two teams of five, one match of four. The old
|
||||||
|
# shape spent more than thirty queries on those rows alone; the
|
||||||
|
# budget here is deliberately loose but far below that, and it can
|
||||||
|
# only be lowered.
|
||||||
|
assert 1 <= counter.total <= 25, f'{counter.total} SELECTs to render one tryout'
|
||||||
|
|||||||
Reference in New Issue
Block a user