perf: eliminate N+1 queries for follower/following counts and follow status#52
perf: eliminate N+1 queries for follower/following counts and follow status#52huoxin233 wants to merge 6 commits intoimorland:1.xfrom
Conversation
…status - Introduced caching for follow state and counts in FollowState to optimize performance. - Updated AddBasicUserAttributes to utilize cached follow state from relations. - Implemented LoadRelations class to batch-load follower and following counts for users in discussions. - Adjusted user model relationships to include subscription data in the pivot table.
imorland
left a comment
There was a problem hiding this comment.
Thanks for this — the diagnosis is correct and the withPivot('subscription') addition is actually a correctness fix that was needed independently of the performance work. The batch loadCount() approach is the right tool here. A few things to address before merging:
[Blocking] Static caches persist across requests in long-running runtimes
FollowState::$followStateCache, $followerCountCache, and $followingCountCache are PHP static arrays on the class. In standard PHP-FPM this is fine — each request gets a fresh process. But Flarum can be run under Octane, RoadRunner, or Swoole, where the process is reused across requests. In those environments these caches will survive across requests and serve stale data to other users.
The proper fix is to reset the caches at the start of each request. The simplest approach is to add a public static function resetCaches(): void method and call it from a middleware or a Booting event listener. Alternatively, move the mutable state into a per-request service bound in the container.
[Blocking] Cache not invalidated after a write in SaveFollowedToDatabase
SaveFollowedToDatabase::handle() calls FollowState::updateOrCreate() / detach() and then $actor->load('followedUsers') and $user->load('followedBy') — but it never clears $followStateCache, $followerCountCache, or $followingCountCache. If the follow/unfollow response then serializes the actor or user (which it does — the PATCH /api/users/{id} response includes both), getFollowerCount() / getFollowingCount() / for() will return pre-write values from the cache. The counts shown in the API response after a follow/unfollow will be stale.
The fix is to invalidate the relevant cache keys in SaveFollowedToDatabase after the write, e.g.:
FollowState::invalidateCache($actor->id, $user->id);with a corresponding static method that unsets the affected keys from all three arrays.
[Minor] ShowDiscussionController — countRelation may be a no-op for post authors
In countRelation(), when $data instanceof Discussion, the post-user collection is only built if $data->relationLoaded('posts') is true. But prepareDataForSerialization fires before Flarum resolves the include parameter and eager-loads relations — so posts will not be loaded at that point. The loadActorFollows callback still fires correctly and helps with follow state, but the count-cache seeding for post authors in ShowDiscussionController will always silently do nothing. Worth either removing the ShowDiscussionController registration for countRelation to avoid confusion, or finding another hook point.
[Minor] if ... if should be if ... elseif in the Collection branch of countRelation()
if ($data->first() instanceof Discussion) { ... }
if ($data->first() instanceof Post) { ... }These are two independent if statements. In practice a collection will never contain both types, but the logic should be if ... elseif to make the mutual exclusion explicit and avoid two first() calls.
Happy to take another look once these are addressed.
|
[Blocking] Static caches persist across requests in long-running runtimes [Blocking] Cache not invalidated after a write in SaveFollowedToDatabase [Minor] ShowDiscussionController — countRelation may be a no-op for post authors So by the time the callback runs, $data->relationLoaded('posts') will be true if the client requested [Minor] if ... if should be if ... elseif in the Collection branch of countRelation() |
Fixes #51
Tried to fix on 1.x first
Problem
AddBasicUserAttributesfires up to 3 queries per serialized user with no batching or caching.Solution
Batch loading
LoadRelations::countRelation()hooks intoListDiscussionsControllerandListUsersControllerviaprepareDataForSerialization. It resolves all discussion authors and last posters, then calls Eloquent'sloadCount().Request-scoped cache
FollowStatenow memoizes counts and follow status per user ID. Results fromloadCount()are seeded into the cache so users appearing in multiple serialization contexts (different model instances) don't re-query.In-memory follow status
followedattribute is now resolved from the actor's already-loadedfollowedUsersrelation, not a live DB query.Before
After