Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Activities profile page N+1 issue #4

Open
FHoulbreque opened this issue May 12, 2017 · 5 comments
Open

Activities profile page N+1 issue #4

FHoulbreque opened this issue May 12, 2017 · 5 comments

Comments

@FHoulbreque
Copy link

Hello Jeffrey,

I tried to ask this question on multiple place but without getting any answer about how I can handle the eager loading with the Activities polymorphic relationship.

If I have a lot of activities, it generates 3 queries for each one. Which is kind of a performance issue there.

Is it possible to eager load child and subchild models with a Polymorphic relationship in Eloquent or do we have to build the query manually?

Thank you for fabulous tutorial which answer a lot of my questions, except this one where I'm stuck with.

Hope this could be accounted as an issue on your app.

Kind regards.

@derekmd
Copy link

derekmd commented Jun 5, 2017

I had a similar morphs problem pop up in my day job and I was able to solve it using a new Collection macro:

Collection::macro('loadMorph', function ($relation, $relations) {
    $this->pluck($relation)
        ->groupBy(function ($model) {
            return get_class($model);
        })
        ->filter(function ($models, $className) use ($relations) {
            return array_has($relations, $className);
        })
        ->each(function ($models, $className) use ($relations) {
            $className::with($relations[$className])
                ->eagerLoadRelations($models->all());
        });

    return $this;
});

Since the activity query has doubly-nested polymorphic relations needing to be eager loaded to generate model path() URIs, the fixed query and collection pipeline comes out pretty gnarly:

    public static function feed($user, $take = 50)
    {
        return static::where('user_id', $user->id)
            ->latest()
            ->with('subject')
            ->take($take)
            ->get()
            ->loadMorph('subject', [
                Favorite::class => 'favorited',
                Reply::class => 'thread',
                Thread::class => 'channel',
            ])
            ->tap(function ($activity) {
                $activity->where('parentable_type', Favorite::class)
                    ->loadMorph('favorited.subject', [
                        Reply::class => 'thread',
                        Thread::class => 'channel',
                    ]);
            })
            ->groupBy(function ($activity) {
                return $activity->created_at->format('Y-m-d');
            });
    }

Here's a forked branch containing this source code along with a Collection::loadMorph() test: https://github.com/derekmd/Lets-Build-a-Forum-in-Laravel/commits/profile-n-plus-1 It required removing always enabled protected $with = [] on some models so other queries will have to be adjusted to fix separate N+1s.

I also wrote a blog post Eager Loading Laravel Polymorphic Relations detailing how I arrived at the solution.

@MadMikeyB
Copy link

@derekmd very excited to try this - been looking for a solution for around a week now. I ended up removing all instances of $with on my models. Looks like you came to a neater conclusion!

@derekmd
Copy link

derekmd commented Jun 6, 2017

re: your email notification, you may still see duplicate queries since the eager-loads don't cross each model type. so e.g., with('subject.channel') for Thread::class and with('subject.favorited.channel') for Favorite::class->Thread::class may have to retrieve the same Channel model keys since those are separate queries being run.

But if an author is posting a thread and liking it themselves, maybe they deserve some duplicate queries!

For basic sample data of 10 or so threads, the intersection of models here would be dense. This activity feed wouldn't show performance improvements from Collection::loadMorph() until there were hundreds of threads and thousands of replies for a single user, at which point that profile would become paginated for any significant web traffic.

If absolute deduping of queries is required, I have some old code kicking around of a RelationsDictionary class written by a colleague. Instead of a collection macro, other queries have to be manually put together by plucking keys for each class type (including that nested polymorphic relation.) So separate Collection instances have to be handled for each polymorphic relation.

In this case the profile partials wouldn't use $activity->subject->someMethodOrProperty. Instead it'd be $favoritedRelationsDictionary[$activity->subject_id][$activity->subject->subject_id], $replyRelationsDictionary[$activity->subject_id][$activity->subject->thread_id], etc. I dread to even think about what the code would look like for this activity feed.

For a problem of this scale, I'd truthfully just leave it as is until it really becomes a problem in production that affects user experience.

@MadMikeyB
Copy link

MadMikeyB commented Jun 6, 2017

@derekmd Thank you for the well thought out and well explained reply. Foolishly, it turned out that I had not cloned the correct branch of yours to check the query count! (Hence the deletion of my previous comment).

Upon cloning the correct branch the query count dropped to 9 queries overall for a dataset of around 20 items showing from the feed. Not bad work!

My own version of this uses Forum instead of Channel and Post instead of Reply, as I have been following along the videos but adapting where naming made little sense to me. As such your code required some tweaks.

I'm still on quite a large amount of queries on my own codebase for the profile (around 90 queries for a dataset of 100), however I feel that may be because my blade partials attempt to touch a lot more relations than the basic ones which Jeffrey and yourself is using, as you can see from the screenshot attached.

screen shot 2017-06-06 at 20 39 55

The main reason I am concerned with the query count is that I plan to have the Activity Feed power the home page of my app, so it's the first thing which users will see, and the entry point to the application.
The reduction in queries your method offered on your code is much nicer and does resolve the N+1 issue, so it's time I go digging into my Blade files to see what is going wrong there! 👍

PS, one question I did have was about parentable_type - where is this set? Is this something I may be missing?

@derekmd
Copy link

derekmd commented Jun 6, 2017

Cool, good work. For a homepage dashboard, I'd suggest introducing a cache for each user like:

$activity = Cache::rememberForever("dashboard:{$user->id}", function () {
    return Actvity::feed($user, etc.);
});

Then bust that cache using Eloquent's created event for any model that affects the feed for that user.

  • User likes something
  • User posts a thread
  • User posts a reply
  • Someone posts a reply to that user's thread
  • Someone likes that user's thread or reply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants