perf(attendees): eliminate /attendees N+1 — 83 → 36 queries (-57%)#550
Open
smarcet wants to merge 8 commits into
Open
perf(attendees): eliminate /attendees N+1 — 83 → 36 queries (-57%)#550smarcet wants to merge 8 commits into
smarcet wants to merge 8 commits into
Conversation
…ts/{id}/attendees
Same instrumentation pattern as /events (see adr/002):
- ServerTimingDoctrine on the route (before auth.user)
- timing markers in the shared ParametrizedGetAll trait
- re-enabled temporary SQL pattern logger in QueryTimingCollector +
'N+1 candidate' log entries when db_count >= 20
Diagnostic-only — will be removed in cleanup once N+1s are identified
and fixed.
… — eliminates ~24 queries /attendees profiling showed 3 DISTINCT PresentationSpeaker patterns firing ~8 times each per request (24 total). Traced to SummitAttendeeSerializer:133 $summit->getSpeakerByMember($member), which calls getSpeakerByMemberId() which runs THREE separate queries per call (moderator check, speaker check, assistance check). Two-part fix: 1. Summit gains a request-scoped $speakerByMemberIdCache (unannotated so Doctrine ignores it) and getSpeakerByMemberId() now checks/writes the cache at every return point. By itself this is just insurance against repeated lookups within a request. 2. New Summit::preloadSpeakersByMemberIds(array $ids) runs the same 3 lookup steps but with WHERE mb.id IN (:ids), then populates the cache for every member id (with null for the ones not found). Result: 3 batch queries instead of N×3 per-attendee queries. 3. ParametrizedGetAll trait grows an optional $afterQuery callable param that fires between the data load and the toArray() call so callers can warm caches without modifying the trait body for each endpoint. 4. OAuth2SummitAttendeesApiController::getAttendeesBySummit passes a closure that collects the page's attendee member ids and invokes $summit->preloadSpeakersByMemberIds($ids) before serialization.
… reset on auth context change - DoctrineSummitEventRepository: change 'SELECT sp, p' to 'SELECT sp ... JOIN FETCH sp.presentation p' so getResult() returns SummitSelectedPresentation[] instead of mixed arrays. The prior query caused $sp->getPresentation() to fail on every request, silently falling through the try/catch and leaving the getSelectionStatus() N+1 optimization inactive. - ResourceServerContext: reset cachedCurrentUser/cachedCurrentUserResolved in setAuthorizationContext() and updateAuthContextVar() so a context change mid-request (or between requests in tests) does not return a stale member.
…rQuery hook
Collapses 4 more per-attendee N+1 patterns into 4 batch fetch-join queries:
- Notes (SummitAttendeeNote, 10 q on a 10-row page)
- Tickets (SummitAttendeeTicket, 10 q)
- Tags ManyToMany (10 q)
- Member ManyToOne fetch-join (8 q)
Each runs as 'SELECT a, X FROM SummitAttendee a LEFT JOIN a.X X WHERE a.id
IN (:ids)' which fetch-joins the inverse collection / association onto each
already-loaded SummitAttendee in the UnitOfWork. Subsequent serializer
accesses read from memory.
Expected: 63 -> ~25 queries on /summits/{id}/attendees, db time roughly
halved.
Two follow-up N+1s exposed by removing the upstream lazy loads:
1. Speaker member fetch — the preloadSpeakersByMemberIds queries joined
ps.member mb but did not addSelect('mb'), so Doctrine used the join only
for filtering and the resulting speakers had unloaded Member proxies.
Add ->addSelect('mb') to all three (moderator/speaker/assistance) batch
queries. Saves ~8 Member queries per request.
2. SummitAttendeeBadge — the per-ticket badge lookup (12 q on a 10-row page)
fired once per ticket. Extend the tickets preload from
'SELECT a, t' to 'SELECT a, t, b' with an extra LEFT JOIN t.badge b,
fetch-joining the badge alongside the ticket.
Expected: another ~20 queries removed, total drops from 49 to ~29.
- adr/002: fix heading ADR-0001 → ADR-0002 to match filename - PresentationSpeaker: add clearPreloadedAssignmentOrder(id) and clearAllPreloadedAssignmentOrders() so write paths can invalidate the preloaded assignment-order cache - PresentationSpeakerCacheTest (8 tests, no DB): covers cache hit, null order, clear-single, clear-all, and Presentation preloaded selection-status path (memoization + reset) - ResourceServerContextTest: add setAuthorizationContextResetsUserCache asserting cachedCurrentUserResolved is cleared by setAuthorizationContext()
Follows the same methodology and structure as ADR-002. Records the afterQuery trait hook (reusable for future endpoints), the Summit-level speaker memo + batch preload, and the per-attendee notes/tickets+badges/ tags/member fetch-join preloads. Includes honest reading of the result — query count down 57% but wall-clock only down 12% because DB latency now dominates over N+1 count.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-550/ This page is automatically updated on each push to this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets performance on GET /api/v1/summits/{id}/attendees by collapsing serializer-driven N+1 query patterns into a small set of batch preloads, leveraging a new _getAll “afterQuery” hook to warm Doctrine associations/caches before serialization.
Changes:
- Added an optional
afterQueryhook toParametrizedGetAll::_getAll()and used it on the attendees endpoint to batch preload Notes, Tickets+Badges, Tags, and Member associations. - Added request-scoped memoization plus batch preloading for
Summit::getSpeakerByMemberId()to avoid repeated per-attendee speaker lookups. - Extended Doctrine query timing tooling to optionally bucket SQL patterns for N+1 discovery and added route-level Server-Timing instrumentation to
/attendees.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| routes/api_v1.php | Enables server.timing.doctrine middleware on the attendees listing route for profiling/timing visibility. |
| app/Models/Foundation/Summit/Summit.php | Adds per-request speaker lookup cache and a batch preloader for member→speaker resolution. |
| app/Http/Middleware/ServerTimingDoctrine.php | Adds temporary N+1 pattern logging based on collected SQL timing/pattern stats. |
| app/Http/Middleware/Doctrine/QueryTimingMiddleware.php | Passes SQL text into the timing collector for per-pattern bucketing. |
| app/Http/Middleware/Doctrine/QueryTimingCollector.php | Implements per-pattern aggregation (normalize + topPatterns) for N+1 detection. |
| app/Http/Controllers/Apis/Protected/Summit/Traits/ParametrizedGetAll.php | Adds the afterQuery hook and extra timing markers around controller/serializer phases. |
| app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php | Uses afterQuery to batch preload associations and speaker cache before serialization. |
| adr/003-attendees-endpoint-n-plus-1-elimination.md | Documents the profiling methodology, decisions, and measured impact for /attendees. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
40
to
47
| public function query(string $sql): DBALResult | ||
| { | ||
| $start = microtime(true); | ||
| try { | ||
| return parent::query($sql); | ||
| } finally { | ||
| QueryTimingCollector::record($start); | ||
| QueryTimingCollector::record($start, $sql); | ||
| } |
Comment on lines
+18
to
+38
| /** | ||
| * Per-pattern bucket for finding N+1s during profiling. | ||
| * | ||
| * @var array<string, array{count:int, totalMs:float, sample:string}> | ||
| */ | ||
| public static array $patterns = []; | ||
|
|
||
| public static function record(float $startedAt, ?string $sql = null): void | ||
| { | ||
| self::$totalMs += (microtime(true) - $startedAt) * 1000.0; | ||
| $ms = (microtime(true) - $startedAt) * 1000.0; | ||
| self::$totalMs += $ms; | ||
| self::$count++; | ||
|
|
||
| if ($sql !== null) { | ||
| $pattern = self::normalize($sql); | ||
| if (!isset(self::$patterns[$pattern])) { | ||
| self::$patterns[$pattern] = ['count' => 0, 'totalMs' => 0.0, 'sample' => $sql]; | ||
| } | ||
| self::$patterns[$pattern]['count']++; | ||
| self::$patterns[$pattern]['totalMs'] += $ms; | ||
| } |
Comment on lines
+57
to
+66
| // Temporary N+1 candidate logger (profiling-only — remove on cleanup). | ||
| if ($dbCount >= 20) { | ||
| foreach (\App\Http\Middleware\Doctrine\QueryTimingCollector::topPatterns(10) as $row) { | ||
| Log::warning('N+1 candidate', [ | ||
| 'count' => $row['count'], | ||
| 'totalMs' => $row['totalMs'], | ||
| 'sample' => mb_substr($row['sample'], 0, 240), | ||
| ]); | ||
| } | ||
| } |
Comment on lines
+716
to
+719
| if (method_exists($attendee, 'getMember')) { | ||
| $m = $attendee->getMember(); | ||
| if ($m !== null && method_exists($m, 'getId') && $m->getId()) { | ||
| $memberIds[] = $m->getId(); |
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
Same profiling-driven methodology as #549. Stacked on
hotfix/cache-optimizations.Full notes in
adr/003-attendees-endpoint-n-plus-1-elimination.md.What changed
Reusable infra:
ParametrizedGetAll::_getAllgains an optionalcallable $afterQuery = nullhook. Fires between data load and$response->toArray()— lets callers warm caches / batch-load related entities without touching the trait body. Backward-compatible.Targeted fixes:
Summit::getSpeakerByMemberper-instance memo +preloadSpeakersByMemberIdsbatch (3 lookups × N members → 3 batch queries with member fetch-join)SELECT a, n)SELECT a, t, b)SELECT a, tg)SELECT a, m)All five batch queries run from one closure passed to the trait's new
afterQueryparameter.Why total time only dropped 12% despite -57% queries
DB latency on the model database is ~25-30ms per query. We removed 47 queries' worth of latency but the remaining 36 still cost ~1000ms collectively. DB latency, not N+1 count, is now the dominant component — next investigation would be at the infrastructure layer (connection pool, replica targeting, query batching), not application code.
What was intentionally NOT fixed
PresentationSpeakerSELECT × 7 — deeper-chain access; ~6ms savingsCOUNT(MemberID)× 6 — non-current-user belongsToGroupSET TRANSACTION× 3 — connection lifecycleEach would save 10-50ms; diminishing returns.
Stacked on #549
Includes a merge commit from
hotfix/cache-optimizations(events PR). The traitafterQueryhook is technically introduced here but applies cleanly to both branches and will only flow into main once events PR merges first.