From: Omar Polo <op@omarpolo.com>
Subject: Re: gotwebd diff is too slow
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Tue, 13 Jun 2023 21:03:53 +0200

On 2023/06/13 10:34:21 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> I noticed that gothub.org is insanely slow when browsing diffs in src.git,
> in the order of minutes (GET request taking 96897ms and similar according
> to firefox dev tools).
> 
> Initially I thought this was due to gothub running in a slow arm64 VM.
> But I can reproduce the same slowness on amd64 bare metal. Compare the
> speed at which tog loads a diff to the speed of gotwebd running on the
> same machine, and cry...

yeah... ^^"

noticed the same but didn't had the time to dig until now.

> The hack below makes it much faster.
> It seems the limit parameter to got_get_repo_commits() is not being respected?

not at all.  it traverses *all* the commits in the
repository... woopsie!

> I don't really understand why all this complicated hunting through commit
> history is even necessary for displaying a diff to the first parent commit.
> Can someone who understands this code come up with a better fix?

got_get_repo_commits is used for two different purposes:

 - get one commit (for diff purposes, but not only)
 - get a list of commits

In the second case, we also need to navigate one commit more than
requested to provide the 'next' button.  We can't use the parent
commit id from the last seen commit because we might be filtering
per-file.

In the first case we don't care.

Diff below is an attempt at simplifying this function.  The idea is to
bump `limit' instead of doing complex logics inside the loop.

There's a small hack: limit can be 1 also if srv->max_commits_display
is one, so we have to handle that condition not to break pagination in
those cases.

For all I care, srv->max_commits_display=1 should be illegal and I'll
send a diff to disallow it.  (may need to do a sweep to see if there
are other places that would be simplified.)

full disclaimer: I hadn't had the time to properly test the diff, only
checked that it loops only once for diffs and pagination of briefs
works.


diffstat refs/remotes/got/main refs/heads/main
 M  gotwebd/got_operations.c  |  19+  30-

1 file changed, 19 insertions(+), 30 deletions(-)

diff refs/remotes/got/main refs/heads/main
commit - c6119c6d1145977d2ae8fb6754a9c369731d8dec
commit + 178793bf8610b2a4b6ca200423a8b5bc063a25e6
blob - 3e66b1cc2a7cd11d9c5f6e4d07a2826a420824e2
blob + aba5a591662378d9b7363a6435d153408a11fb87
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -326,8 +326,17 @@ got_get_repo_commits(struct request *c, int limit)
 	struct querystring *qs = t->qs;
 	struct repo_dir *repo_dir = t->repo_dir;
 	char *in_repo_path = NULL, *repo_path = NULL, *file_path = NULL;
-	int chk_next = 0, chk_multi = 0;
+	int chk_next = 0;
 
+	if (limit != 1 || srv->max_commits_display == 1) {
+		/*
+		 * Traverse one commit more than requested to provide
+		 * the next button.
+		 */
+		limit++;
+		chk_next = 1;
+	}
+
 	TAILQ_INIT(&refs);
 
 	if (qs->file != NULL && strlen(qs->file) > 0)
@@ -404,38 +413,18 @@ got_get_repo_commits(struct request *c, int limit)
 			goto done;
 		}
 
+		if (--limit == 0 && chk_next) {
+			t->more_id = strdup(repo_commit->commit_id);
+			if (t->more_id == NULL)
+				error = got_error_from_errno("strdup");
+			goto done;
+		}
+
 		TAILQ_INSERT_TAIL(&t->repo_commits, repo_commit, entry);
 
-		if (!chk_multi || limit != 1 ||
-		    srv->max_commits_display == 1) {
-			chk_multi = 1;
+		if (limit == 0)
+			goto done;
 
-			/*
-			 * check for one more commit before breaking,
-			 * so we know whether to navigate through briefs
-			 * commits and summary
-			 */
-			if (chk_next && (qs->action == BRIEFS ||
-			    qs->action == COMMITS || qs->action == SUMMARY)) {
-				t->more_id = strdup(repo_commit->commit_id);
-				if (t->more_id == NULL) {
-					error = got_error_from_errno("strdup");
-					goto done;
-				}
-				got_object_commit_close(commit);
-				commit = NULL;
-				TAILQ_REMOVE(&t->repo_commits, repo_commit,
-				    entry);
-				gotweb_free_repo_commit(repo_commit);
-				goto done;
-			}
-		}
-		if (error || (limit && --limit == 0)) {
-			if (qs->file != NULL && *qs->file != '\0')
-				if (chk_multi == 0)
-					break;
-			chk_next = 1;
-		}
 		if (commit) {
 			got_object_commit_close(commit);
 			commit = NULL;