[2.x] perf: point endpoint eager loads back at their parent models - #4877
Merged
Conversation
Relations pre-loaded by an endpoint eager load arrive through loadMissing(), which wires nothing back to the models they were loaded for. The relationship buffer would set the declared inverse, but it skips relations that are already loaded — so nothing did. Serializing an included post then re-fetched its discussion one row at a time for every visibility check (canEdit, canHide, canFlag), even though that discussion is the very model being listed. Installs running flarum/likes never saw this: likes carries a hand-written workaround that eager loads firstPost.discussion and its tags precisely "to avoid N+1s in DiscussionPolicy::can()". Any leaner install paid one discussion fetch per included post — a pure-core reproduction with a single eagerLoadWhenIncluded extender shows 10 identical single-row fetches on a 10-discussion page. loadRelations() now points each loaded relation back at its parent, using the relationship's declared inverse and falling back to the same class-name derivation EloquentBuffer::load() already uses for buffered loads. The parent is already in memory, so the wiring costs no queries. Likes' workaround is retired in the same change: the parent discussions it re-fetched (with their tags) are exactly what the inverse now points at, and the tags extension already eager loads tags on those parents. On a 74-extension install, the discussions index with firstPost and lastPost included drops from 43 to 39 queries and the index document from 41 to 39, with responses byte-identical — the removed queries are likes' now-redundant batched re-fetches.
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.
The problem
Relations pre-loaded by an endpoint eager load (
eagerLoad(),eagerLoadWhenIncluded(),eagerLoadWhere()) arrive throughloadMissing(), which wires nothing back to the models they were loaded for. The relationship buffer would set the declared->inverse()— but it skips relations that are already loaded, so for pre-loaded relations nobody did.Serializing an included post then re-fetched its discussion one row at a time for every visibility check (
canEdit,canHide,canFlag→PostPolicy::can()→$post->discussion) — even though that discussion is the very model being listed, sitting in memory the whole time.A pure-core reproduction (one
eagerLoadWhenIncluded(['firstPost' => ['firstPost.user']])extender — the exact shape sticky+geoip, mentions and likes all use) on a 10-discussion page:The flarum/testing N+1 detector fails the request on its own before the test's assertion even runs.
Why nobody noticed
flarum/likes carries a hand-written workaround for exactly this bug — a conditional block eager loading
firstPost.discussion+firstPost.discussion.tags(and the lastPost pair), whose own comment says it exists "to avoid N+1s in DiscussionPolicy::can()". Any install running likes was masked; any leaner install paid one discussion fetch per included post. FoF/geoip's test suite surfaced it as a30x (15 distinct)repeated-query warning.The fix
After
loadRelations()finishes itsloadMissing()calls,setInverseRelations()points each loaded relation back at its parent — using the relationship's declared->inverse()and falling back to the same class-name derivationEloquentBuffer::load()already uses for buffered loads. The parent is already in memory, so the wiring costs zero queries. Only the first segment of each eager-load path is touched (deeper segments belong to other parents), andisRelation()guards against setting a relation the related model doesn't have.Likes' workaround is retired in the same change. The parent discussions it re-fetched (with their tags) are exactly what the inverse now points at, and the tags extension already eager loads
tagson those parents unconditionally, soDiscussionPolicy::can()'s$discussion->tagsread stays batch-loaded.Measured (74-extension install, responses byte-identical on all compared endpoints)
/api/discussions?include=firstPost,lastPost/index documentThe removed queries are likes' now-redundant batched re-fetches. On installs without likes, the win is the N+1 itself: one query per included post, gone.
Testing
ListWithIncludedPostsQueryCountTest, 3 tests: buffer-path regression guard (include=firstPost, was already correct), the loadMissing-path case written RED first, and a serialization-equivalence guard (linkage unchanged, all posts still included)Follow-up (separate): flarum/sticky still forces full rendered
firstPostinto every index document for a plain-text excerpt shown only on stickied discussions — that's the remaining ~110ms/index-view item from the discuss profiling.