Skip to content

Improvement for branches list API performance

Robert May requested to merge 30536-branches-list-api-performance-degrades-notably-under-load into master

What does this MR do?

This is a first iteration.

This implements a basic cache for merged branch names on the repository. In the interest of being iterative, this is a fairly naive implementation; I'd like to test out the performance improvement this brings whilst I work on a second iteration to swap the way it stores the data to a Redis HSET (which will require a much bigger implementation).

Rather than cache all merged branch names (which could be a massive amount of data), it only caches the ones it is asked to look for. In addition to this I've also set an expiry time of 10 minutes on this, as the original issue is around repeated requests to the same endpoint causing performance degradation, and this lowers the impact it will have (and if the data set is too large, disabling the feature flag will result in the data disappearing within 10 minutes).

Expanded explanation copied from comments

The reason it needs to store the hash (rather than an array) is it's actually caching true/false values of whether a branch name is merged or not, rather than just a list of the merged branch names. Unfortunately if you only cache the merged branch names in an array then it will need to do a disk-read every time still, as it can't know the merged status of the missing branch names. This method basically has two use-cases that I could see currently:

  1. Call it with [], return all the merged branch names (an array would be fine for this use-case)
  2. Call it with an array of branch names for which to check the merge status

In the second use-case, if we only cached an array of the merged branches, it would need to fetch from the disk to check the merge status of any that aren't stored in that cache. Whereas if we use a hash (which will be a Redis HSET in the next iteration), we can cache the merge status in either condition.

Really it should probably be two separate methods for those two use-cases, but this will involve refactoring some existing code; that's on my roadmap for the next iterations.

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Related #30536 (closed)

Edited by Rémy Coutable

Merge request reports