Skip to content
Snippets Groups Projects

optimize graphqlbackend gitserver API usage to improve performance of admin repositories page

Created by: slimsag

Prior to this change, for every repository in the connection resolver (which on e.g. the /site-admin/repositories page is often a lot), we would independently ask gitserver for information about the repository via its API. This was costly because it meant performing literally thousands of tiny sub-millisecond HTTP requests, individually JSON encoding them (which may create lots of GC pressure, too).

The solution here is simple: When we ask gitserver for info about thousands of repositories, do so using a single request instead of thousands.

On a dev instance with ~12k repositories, before:

01:37:35               frontend | took 3.96825239s
01:37:50               frontend | took 4.236735448s
01:38:01               frontend | took 5.147227239s
01:38:11               frontend | took 6.093235795s
01:38:33               frontend | took 10.372046862s
01:38:57               frontend | took 13.729139161s
01:39:51               frontend | took 14.654338045s
01:42:08               frontend | took 3.814422443s
01:42:19               frontend | took 4.560987174s
01:42:31               frontend | took 4.626022391s
01:42:40               frontend | took 4.8749788s
01:42:48               frontend | took 3.687233796s
01:42:55               frontend | took 3.738414004s
01:43:04               frontend | took 3.672425721s
01:43:14               frontend | took 3.682802146s

And after:

02:07:51               frontend | took 1.25605805s
02:07:58               frontend | took 1.441502375s
02:08:03               frontend | took 1.43875787s
02:08:12               frontend | took 1.410966593s
02:08:18               frontend | took 1.416104619s
02:08:26               frontend | took 1.382661535s
02:08:30               frontend | took 1.282073601s
02:08:34               frontend | took 1.635967947s
02:08:38               frontend | took 1.231089455s
02:08:42               frontend | took 1.230096699s
02:08:47               frontend | took 1.272247102s
02:08:51               frontend | took 1.267581576s
02:08:55               frontend | took 1.307818437s
02:08:59               frontend | took 1.272283421s
02:09:05               frontend | took 1.165448496s

I suspect the result may be even more pronounced on a larger instance, but haven't yet verified that.

Test plan: Covered by existing tests + new tests added (see diff).

Fixes #2779

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Created by: slimsag

    I plan to update this + merge soon, but some other customer work became more important. @keegancsmith after I address @tsenart's comment (i.e. change to just a singular gitserver HTTP endpoint); any further comments?

  • Created by: slimsag

    I will update + merge this Mon morning PST. If you want to take a look before then @keegancsmith

  • Created by: keegancsmith

    I'm keen to review the updated version. I agree with Tomas's feedback about simplifying the client API

  • Created by: slimsag

    @keegancsmith @tsenart PTAL

    I am positive you'll have more suggestions on how to make the code nicer (I can think of a few ways I may try tomorrow), but please review now.

    If you think this is risky / I should not push to land this in 3.3 let me know.

  • Created by: tsenart

    It seems that the last missing thing to agree on is whether the roll-out needs to be backwards compatible or not.

  • Created by: codecov[bot]

    Codecov Report

    Merging #2884 into master will decrease coverage by 0.03%. The diff coverage is 32.39%.

    Impacted Files Coverage Δ
    cmd/frontend/graphqlbackend/repositories.go 23.73% <0%> (-0.62%) :arrow_down:
    cmd/repo-updater/repos/purge.go 0% <0%> (ø) :arrow_up:
    cmd/frontend/graphqlbackend/repository_mirror.go 10.29% <0%> (-0.08%) :arrow_down:
    cmd/gitserver/server/server.go 46.25% <100%> (+0.07%) :arrow_up:
    cmd/gitserver/server/repo_info.go 22% <43.75%> (-4.79%) :arrow_down:
  • Created by: slimsag

    @tsenart thanks for the feedback here & raising the important point of rollout. I've thought on this a bit today and:

    1. I added back the deprecated gitserver /repo route, so that gitserver is backwards compat (can run old frontend with new gitserver).
    2. I have not made new frontend compatible with old gitserver, I think it would be very tricky to do this correctly and if we wanted this a better approach would be rolling out the gitserver change in one version then the frontend change in the next (but this has the obvious drawback of 2 versions needed for making this change).
    3. For Sourcegraph.com, rollout will be as simple as merging the gitserver renovate PR first since we have back compat (see #1 above), then the frontend renovate PR.
    4. For customer Kubernetes deployments, there will be a small period where either gitserver or frontend could be up first. This period wouldn't last long, though, so I think it's OK. It would be exactly the amount of time it takes for the new gitserver to fully spin up -- which seems reasonable. In this timeframe, requests for code intel would fail.

    Hopefully all of that sounds reasonable (if not I can revert before we release).

Please register or sign in to reply
Loading