[2.x] perf(mentions): eager load the discussion of mentioning posts - #4876
Merged
Conversation
Serializing a post's "mentionedBy" relation runs a visibility check on every mentioning post, and that check reads the post's discussion. Those posts are materialised by the relation load without it, so each one fetched the same discussion again — and once tags is enabled, its discussion policy fetched that discussion's tags per post too. On a 54-post discussion with 52 mentions, 34 of 77 queries were these repeats, all with identical bindings. Eager loading discussion on the relationship's own scope fixes it. The scope flows into EloquentBuffer::loadLimitedBelongsToMany(), which resolves a windowed set of keys before fetching full rows, so the eager load applies to a bounded set. Putting it on the endpoint's eagerLoad() instead would route through loadMissing() and drag in every mentioning post: a thread with 1402 mentions to one post went from 46 to 5618 queries that way. Loading discussion is enough to remove the tags queries as well, since the tags relation then batch loads across the discussions already in memory. Loading discussion.tags directly would break installs without the tags extension, where Discussion has no tags relation at all. discussion before after 54 posts, 52 mentions 77 queries 45 queries 73 posts, 69 mentions 81 queries 47 queries 112 posts 61 queries 45 queries 1444 posts, 1402 ment. 46 queries 40 queries
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
Serializing a post's
mentionedByrelation runs a visibility check on every mentioning post, andPostPolicy::can()reads$post->discussionto do it. Those posts are materialised by the relation load without that relation, so each one fetched the same discussion over again. With tags enabled, its discussion policy then read$discussion->tagsper post as well.On a 54-post discussion with 52 mentions, 34 of 77 queries were these repeats — all with identical bindings, i.e. the same discussion fetched 17 times.
I confirmed the mechanism by object identity rather than inference: post 6353 existed as two PHP instances, one eager-loaded (
discussionpresent) and one bare (discussionnull). The bare copies are the ones the policy checks.The fix
Eager load
discussionon the relationship's own scope.Why the scope and not the endpoint's
eagerLoad()— this is the part worth reviewing. The scope flows intoEloquentBuffer::loadLimitedBelongsToMany(), which resolves a windowed set of keys first and only then fetches full rows, so the eager load applies to a bounded set. Putting it on the endpoint instead routes throughloadMissing(), which is unbounded: I tried that first, and a thread with 1,402 mentions pointing at one post went from 46 to 5,618 queries and 4.6s.Why
discussionand notdiscussion.tags— loadingdiscussionis sufficient to remove the tags queries too, because the tags relation then batch-loads across the discussions already in memory. Loadingdiscussion.tagsdirectly would crash any install without the tags extension, whereDiscussionhas notagsrelation. That version passed against my dev forum (tags enabled) and only failed once the integration test ran it on a core-only install — see below.Results
A/B by reverting the change, on an install with 74 extensions and tags enabled:
Both policy call sites disappear from the query attribution entirely. The high-fan-in case improves rather than regressing, which is the property the endpoint-level version failed.
Testing
Two integration tests, written RED first. The failing run showed
10x (1 distinct bindings): select * from "discussions" where "discussions"."id" = ?— the N+1 detector added in flarum/testing flagged it independently of my own assertion.Note:
tagshas one pre-existing full-suite failure (ListTest::admin_sees_all) from test-ordering interference. I verified it fails identically on a clean tree — unrelated to this change.Context
Second of the findings from profiling the "discussion views feel slow" reports, after #4875 (model casts memoisation). This is a query-count and DB-load win; the wall-time gain is modest because these are sub-millisecond primary-key lookups, but it removes work that scales with mention density on exactly the long, mention-heavy threads that prompted the reports.