Skip to content

Draft: Preload `avatar_url` for Projects::MergeRequests::CreationsController

Vasilii Iakliushin requested to merge 193481_preload_avatar_url into master

What does this MR do and why?

Problem

We load all avatars of forked projects and that causes a performance problem.

Screenshot_2022-07-20_at_21.54.21

(source https://gitlab.com/gitlab-org/gitlab/-/merge_requests/new)

Solution

By default we use batch loading for avatars, but in this case we want to avoid that. I call avatar_url method early to change the order of the batch load and fix the unoptimized query that fetches all avatars of project's forks.

Screenshots or screen recordings

Before After
Screenshot_2022-07-20_at_20.59.06 Screenshot_2022-07-20_at_20.58.28

Explanation

The query before the fix contains upload paths to original project (id: 4) and forked project (id: 64). For projects with many forks the query will have uploads paths of all of them in the WHERE. But we need the avatar of the original project only and we don't display avatars of forked projects.

The query after the fix correctly fetches only paths of the original project.

Detailed explanation

On merge request new page we have two select boxes to choose the project. The target select box loads names and ids of all available forked projects (see code).

Screenshot_2022-06-02_at_22.50.34

But for each initialized project we add avatar loading task to the batch loader (see code)

The batch loader triggers when we explicitly request the avatar_url. This call loads avatars for all projects added to the batch loader.

Here is a chain of events before the fix

  1. Load the project (here)
  2. Load all forked projects (and add avatars for each of them to the batch loader) (here)
  3. For project breadcrumbs we call the avatar_url the first time
  4. That loads all of the projects added to the batch loader (as a result, terrible performance)

After the fix

  1. Load the project (here)
  2. Trigger the avatar_url for it (that causes an early execution of the batch loader)
  3. Load all forked projects (and add avatars for each of them to the batch loader)
  4. But because we never access these avatars we won't send a query to load them.

Problem with testing this behavior

Without forks this problem is not visible, but every time we create a fork in test after_initialize is triggered. So, it automatically adds an event to the batch loader. That mixes the original chain of events and complicates the test scenario.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Vasilii Iakliushin

Merge request reports