Skip to content

  • Projects
  • Groups
  • Snippets
  • Help
  • This project
    • Loading...
  • Sign in / Register
GitLab Community Edition
GitLab Community Edition
  • Overview
    • Overview
    • Details
    • Activity
    • Cycle Analytics
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
    • Charts
    • Locked Files
  • Issues 10,436
    • Issues 10,436
    • List
    • Boards
    • Labels
    • Service Desk
    • Milestones
  • Merge Requests 551
    • Merge Requests 551
  • CI / CD
    • CI / CD
    • Pipelines
    • Jobs
    • Schedules
    • Charts
  • Snippets
    • Snippets
  • Members
    • Members
  • Collapse sidebar
  • Activity
  • Graph
  • Charts
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
  • GitLab.org
  • GitLab Community EditionGitLab Community Edition
  • Merge Requests
  • !2752

Merged
Opened Feb 08, 2016 by Yorick Peterse - OOO until May 4th@yorickpeterse 
  • Report abuse
Report abuse

Cache various Repository Git operations

Related comment: gitlab-com/operations#42 (comment 3609898)

cc @joshfng

  • Discussion 11
  • Commits 1
  • Pipelines 1
  • Changes
{{ resolvedDiscussionCount }}/{{ discussionCount }} {{ resolvedCountText }} resolved
  • Yorick Peterse - OOO until May 4th @yorickpeterse commented Feb 08, 2016
    Master

    TODO:

    • Flush the cache for Repository#has_visible_content? whenever a user deletes the last remaining branch or creates a new one.
    • Pushing commits currently flushes all caches, which does more harm than good. On a busy project it would make the caching changes introduced in this MR pretty much useless.
    • Measure if this has any impact on any other pages.
    Edited Feb 08, 2016 by Yorick Peterse - OOO until May 4th
    TODO: * [x] Flush the cache for `Repository#has_visible_content?` whenever a user deletes the last remaining branch _or_ creates a new one. * [x] Pushing commits currently flushes _all_ caches, which does more harm than good. On a busy project it would make the caching changes introduced in this MR pretty much useless. * [x] Measure if this has any impact on any other pages.
  • Yorick Peterse - OOO until May 4th @yorickpeterse commented Feb 08, 2016
    Master

    Worth mentioning: our Redis cache expiration time is 2 weeks, this means that after 2 weeks of the initial caching certain pages might be slower the first until the cache is built. For now this should be good enough.

    Worth mentioning: our Redis cache expiration time is 2 weeks, this means that after 2 weeks of the initial caching certain pages _might_ be slower the first until the cache is built. For now this should be good enough.
  • Yorick Peterse - OOO until May 4th @yorickpeterse

    mentioned in issue gitlab-com/operations#42 (closed)

    Feb 08, 2016

    mentioned in issue gitlab-com/operations#42 (closed)

    mentioned in issue gitlab-com/operations#42
    Toggle commit list
  • Yorick Peterse - OOO until May 4th
    @yorickpeterse started a discussion on the diff Feb 08, 2016
    app/models/repository.rb
    44 44 end
    45 45  
    46 46 def empty?
    47 raw_repository.empty?
    47 return @empty unless @empty.nil?
    • Yorick Peterse - OOO until May 4th @yorickpeterse commented Feb 08, 2016
      Master

      The use of instance variables here (and below) ensures we don't end up loading data from Redis every time these methods are called on the same Repository instance within the same request.

      Edited Feb 08, 2016 by Yorick Peterse - OOO until May 4th
      The use of instance variables here (and below) ensures we don't end up loading data from Redis every time these methods are called on the same Repository instance within the same request.
    Please register or sign in to reply
  • Yorick Peterse - OOO until May 4th
    @yorickpeterse started a discussion on an old version of the diff Feb 08, 2016
    app/models/repository.rb
    Unable to load the diff.
    • Yorick Peterse - OOO until May 4th @yorickpeterse commented Feb 08, 2016
      Master

      This ensures that the root reference cache is flushed whenever the user changes the default branch (which triggers a call to expire_branch_cache).

      This ensures that the root reference cache is flushed whenever the user changes the default branch (which triggers a call to `expire_branch_cache`).
    Please register or sign in to reply
  • Yorick Peterse - OOO until May 4th @yorickpeterse

    Added 1 commit:

    • f96c6ff5 - Cache various Repository Git operations
    Feb 08, 2016

    Added 1 commit:

    • f96c6ff5 - Cache various Repository Git operations
    Added 1 commit: * f96c6ff5 - Cache various Repository Git operations
    Toggle commit list
  • Yorick Peterse - OOO until May 4th @yorickpeterse commented Feb 08, 2016
    Master

    I tested the following pages to see how these changes impact performance elsewhere:

    • GitLab CE dashboard
    • GitLab CE commits list
    • GitLab CE issues list

    The timings are as following (here before/after indicates before/after the changes in this MR):

    Before

    • Dashboard: 13 seconds
    • Commits: 28 seconds
    • Issues: 5.5 seconds

    After

    • Dashboard: 5 seconds (= 2.6x faster)
    • Commits: 10 seconds (= 2.8x faster)
    • Issues: 1.8 seconds (= 3x faster)
    I tested the following pages to see how these changes impact performance elsewhere: * GitLab CE dashboard * GitLab CE commits list * GitLab CE issues list The timings are as following (here before/after indicates before/after the changes in this MR): ## Before * Dashboard: 13 seconds * Commits: 28 seconds * Issues: 5.5 seconds ## After * Dashboard: 5 seconds (= 2.6x faster) * Commits: 10 seconds (= 2.8x faster) * Issues: 1.8 seconds (= 3x faster)
  • Yorick Peterse - OOO until May 4th @yorickpeterse

    Title changed from WIP: Cache various Repository Git operations to Cache various Repository Git operations

    Feb 08, 2016

    Title changed from WIP: Cache various Repository Git operations to Cache various Repository Git operations

    Title changed from **WIP: Cache various Repository Git operations** to **Cache various Repository Git operations**
    Toggle commit list
  • Yorick Peterse - OOO until May 4th @yorickpeterse commented Feb 08, 2016
    Master

    To spare reading of gitlab-com/operations#42 (closed), this basically reduces the loading time of issue #1 (closed) from around 21 seconds (locally) to around 3 seconds (also locally).

    To spare reading of https://gitlab.com/gitlab-com/operations/issues/42, this basically reduces the loading time of issue #1 from around 21 seconds (locally) to around 3 seconds (also locally).
  • Yorick Peterse - OOO until May 4th @yorickpeterse

    Added 1 commit:

    • 9a99d8e4 - Cache various Repository Git operations
    Feb 08, 2016

    Added 1 commit:

    • 9a99d8e4 - Cache various Repository Git operations
    Added 1 commit: * 9a99d8e4 - Cache various Repository Git operations
    Toggle commit list
  • Yorick Peterse - OOO until May 4th @yorickpeterse

    Reassigned to @rspeicher

    Feb 08, 2016

    Reassigned to @rspeicher

    Reassigned to @rspeicher
    Toggle commit list
  • Yorick Peterse - OOO until May 4th @yorickpeterse

    Milestone changed to 8.5

    Feb 08, 2016

    Milestone changed to 8.5

    Milestone changed to [8.5](https://gitlab.com/gitlab-org/gitlab-ce/milestones/20)
    Toggle commit list
  • Yorick Peterse - OOO until May 4th @yorickpeterse commented Feb 08, 2016
    Master

    Yay build finally green, comments welcome!

    Yay build finally green, comments welcome!
  • 🏝 Robert Speicher 🏝 @rspeicher commented Feb 08, 2016
    Owner

    @yorickpeterse 👍 LGTM.

    My only question is whether we need to add these new cache keys to cache_keys so they can be cleared by expire_cache, or if they'll only ever need to be cleared manually, as they currently are.

    Anyway, you can merge at your discretion.

    @yorickpeterse :+1: LGTM. My only question is whether we need to add these new cache keys to [`cache_keys`](https://gitlab.com/gitlab-org/gitlab-ce/blob/9a99d8e49dc07faaaa2fae436423e11dab5a7d7e/app/models/repository.rb#L203-205) so they can be cleared by [`expire_cache`](https://gitlab.com/gitlab-org/gitlab-ce/blob/9a99d8e49dc07faaaa2fae436423e11dab5a7d7e/app/models/repository.rb#L238-241), or if they'll only ever need to be cleared manually, as they currently are. Anyway, you can merge at your discretion.
  • 🏝 Robert Speicher 🏝 @rspeicher

    Reassigned to @yorickpeterse

    Feb 08, 2016

    Reassigned to @yorickpeterse

    Reassigned to @yorickpeterse
    Toggle commit list
  • Rémy Coutable @rymai commented Feb 09, 2016
    Master

    @rspeicher I think because of:

    Pushing commits currently flushes all caches, which does more harm than good. On a busy project it would make the caching changes introduced in this MR pretty much useless.

    it's better to not expire these new cache keys?

    @rspeicher I think because of: > Pushing commits currently flushes all caches, which does more harm than good. On a busy project it would make the caching changes introduced in this MR pretty much useless. it's better to not expire these new cache keys?
  • Yorick Peterse - OOO until May 4th @yorickpeterse commented Feb 09, 2016
    Master

    @rspeicher @rymai Rémy is correct here, the methods are not added to cache_keys as that would result in the cache being flushed upon every push even when not needed.

    @rspeicher @rymai Rémy is correct here, the methods are not added to `cache_keys` as that would result in the cache being flushed upon every push even when not needed.
  • Yorick Peterse - OOO until May 4th @yorickpeterse

    Status changed to merged

    Feb 09, 2016

    Status changed to merged

    Status changed to merged
    Toggle commit list
  • Yorick Peterse - OOO until May 4th @yorickpeterse

    mentioned in commit 366f617e

    Feb 09, 2016

    mentioned in commit 366f617e

    mentioned in commit 366f617ecfd31ec1fd4ccdcf7d7f6af98e8f5bb8
    Toggle commit list
  • 🏝 Robert Speicher 🏝 @rspeicher commented Feb 09, 2016
    Owner

    I really need to remember to read @yorickpeterse's commit messages since they're actually useful. 😀

    I really need to remember to read @yorickpeterse's commit messages since they're actually useful. :grinning:
  • Yorick Peterse - OOO until May 4th @yorickpeterse

    mentioned in merge request gitlab-com/www-gitlab-com!1434 (merged)

    Feb 11, 2016

    mentioned in merge request gitlab-com/www-gitlab-com!1434 (merged)

    mentioned in merge request gitlab-com/www-gitlab-com!1434
    Toggle commit list
  • Rubén Dávila
    @rdavila started a discussion on commit 9a99d8e4 Feb 20, 2016
    • Rubén Dávila @rdavila

      mentioned in issue #13387 (closed)

      Feb 20, 2016

      mentioned in issue #13387 (closed)

      mentioned in issue #13387
      Toggle commit list
    Please register or sign in to reply
  • Write
  • Preview
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment
Yorick Peterse - OOO until May 4th
Assignee
Yorick Peterse - OOO until May 4th @yorickpeterse
Assign to
8.5
Milestone
8.5
Assign milestone
Time tracking
1
Labels
performance
Assign labels
  • View project labels
Reference: gitlab-org/gitlab-ce!2752