diff --git a/docs/findings.md b/docs/findings.md index ba61fab9..031e5f54 100644 --- a/docs/findings.md +++ b/docs/findings.md @@ -2632,14 +2632,28 @@ each read was a separate forge request, made one at a time. The same tree holds the answers, so the requests did not depend on each other. Listing the tree and reading its files now overlap, at most eight requests in -flight. Across the thirteen CI runs after the change the same tree, all 93 -directories and 396 files, reads in a median of 16.0s, from 7.3s to 40.5s. The -widest values land on the most loaded runners, so the range is the number to -keep; the baseline is the one pre-change run at 1m6s. Model batch time swings as +flight. Across the thirteen CI runs after the change the same 93 directories +read in a median of 16.0s, from 7.3s to 40.5s. The widest values land on the +most loaded runners, so the range is the number to keep; the baseline is the +one pre-change run at 1m6s. Model batch time swings as widely, 4s to 2m7s over the same runs with a median near 1m25s, so it is still the larger part of a review. What is left to measure is the fixed pre-batch step as a whole, not this read alone. +The read still cost one forge request per file and directory, about 490, plus +two more per file because a read that named no revision fetched the pull +request and its head commit first. The Action already holds the history, so +the provider now answers from the checkout when it has the reviewed commit: +file contents from the commit's objects, listings from one `git ls-tree`, the +head commit once per pull request. The read covers whatever the reviewed +commit holds, so the file count rises as this branch adds tests: the +concurrent runs held 396 or 397 files, the checkout runs 398 or 399. Those +five reads covered all 93 directories in 290ms, 196ms, 277ms, 285ms and +165ms, against 16.6s in the concurrent run before it and 1m6s before that. +`design assembled` fell from 17.1s to a median near 0.9s. Five runs are not a +distribution; the range from the earlier thirteen is why 16s was reported as a +median and not a best case. + `TestDesignSourceCaptureOverlapsRequestsAndStaysWithinTheLimit` pins the overlap as peak requests in flight rather than wall clock, so a loaded runner cannot fail it without a regression. Three siblings pin what the overlap must diff --git a/internal/vcs/github.go b/internal/vcs/github.go index ad754aa6..b587c669 100644 --- a/internal/vcs/github.go +++ b/internal/vcs/github.go @@ -37,6 +37,9 @@ type GitHub struct { // more than 300 files; with a checkout the diff is taken from git // instead, which has no such limit. Checkout string + + // index caches each commit's tree read from Checkout. + index checkoutIndex } // GitHubOptions configure the provider. @@ -299,11 +302,16 @@ func (g *GitHub) FileContent(ctx context.Context, ref Ref, path string) ([]byte, sha := ref.Head if sha == "" { - pr, err := g.PullRequest(ctx, ref) - if err != nil { + var err error + if sha, err = g.headSHA(ctx, ref); err != nil { return nil, err } - sha = pr.HeadSHA + } + + if tree := g.checkoutTree(ctx, sha); tree != nil && tree.regular(path) { + if data, err := (&Local{Dir: g.Checkout}).FileContent(ctx, Ref{Head: sha}, path); err == nil { + return data, nil + } } // DownloadContents falls back to listing the parent even after a file 404, @@ -361,11 +369,16 @@ func (g *GitHub) ListDir(ctx context.Context, ref Ref, dir string) ([]string, er sha := ref.Head if sha == "" { - pr, err := g.PullRequest(ctx, ref) - if err != nil { + var err error + if sha, err = g.headSHA(ctx, ref); err != nil { return nil, err } - sha = pr.HeadSHA + } + + if tree := g.checkoutTree(ctx, sha); tree != nil { + if names, ok := tree.dirs[strings.Trim(dir, "/")]; ok { + return append([]string(nil), names...), nil + } } file, entries, resp, err := g.client.Repositories.GetContents(ctx, ref.Owner, ref.Repo, strings.Trim(dir, "/"), diff --git a/internal/vcs/github_checkout.go b/internal/vcs/github_checkout.go new file mode 100644 index 00000000..8450f7bb --- /dev/null +++ b/internal/vcs/github_checkout.go @@ -0,0 +1,181 @@ +package vcs + +import ( + "bytes" + "context" + "fmt" + "log/slog" + "sort" + "strings" + "sync" +) + +// checkoutTree is one commit's file list, read from the checkout in a single +// git call. Directory listings and file modes are answered from it, so a +// repository of ninety directories costs one process instead of ninety +// requests. +type checkoutTree struct { + modes map[string]string + dirs map[string][]string +} + +// checkoutIndex holds the trees read so far, keyed by commit. +// +// The lock guards the maps only. A tree is built outside it and then +// published, so one slow git call does not stall reads of other commits; two +// callers racing on an uncached commit read it twice and keep one. +type checkoutIndex struct { + mu sync.Mutex + trees map[string]*checkoutTree + heads map[string]string + // warned holds the commits already reported, so a checkout that keeps + // failing logs once and not once per file. + warned map[string]bool +} + +// checkoutTree returns the commit's tree from the checkout, or nil when the +// checkout cannot supply it. A nil answer sends the caller to the API. +// +// A commit the checkout does not hold is remembered, because asking again +// cannot change the answer. A git call that failed after the commit was found +// is not remembered: it may be transient, and caching it would send every +// later read of the commit through the API. +// +// The commit is read from git objects and never the working tree: the review +// is of the commit named, and the checkout may have something else out. +func (g *GitHub) checkoutTree(ctx context.Context, sha string) *checkoutTree { + if g.Checkout == "" || sha == "" || strings.HasPrefix(sha, "-") { + return nil + } + g.index.mu.Lock() + t, seen := g.index.trees[sha] + g.index.mu.Unlock() + if seen { + return t + } + t, held, err := readCheckoutTree(ctx, g.Checkout, sha) + if err != nil { + if ctx.Err() != nil { + return nil + } + g.warnOnce(sha, "checkout could not be read; reading through the API", err) + if held { + return nil + } + } + g.index.mu.Lock() + defer g.index.mu.Unlock() + if kept, raced := g.index.trees[sha]; raced { + return kept + } + if g.index.trees == nil { + g.index.trees = map[string]*checkoutTree{} + } + g.index.trees[sha] = t + return t +} + +// warnOnce logs a checkout problem the first time it is seen for a commit. +func (g *GitHub) warnOnce(sha, msg string, err error) { + g.index.mu.Lock() + seen := g.index.warned[sha] + if g.index.warned == nil { + g.index.warned = map[string]bool{} + } + g.index.warned[sha] = true + g.index.mu.Unlock() + if !seen { + slog.Warn(msg, "checkout", g.Checkout, "commit", short(sha), "err", err) + } +} + +// readCheckoutTree reads one commit's tree. held reports whether the checkout +// has the commit: false with an error is a miss to remember, true with an +// error is a failure that should be retried. +func readCheckoutTree(ctx context.Context, dir, sha string) (t *checkoutTree, held bool, err error) { + local := &Local{Dir: dir} + if _, err := local.gitRaw(ctx, "cat-file", "-e", sha+"^{commit}"); err != nil { + return nil, false, fmt.Errorf("commit not in checkout: %w", err) + } + out, err := local.gitRaw(ctx, "ls-tree", "-r", "-t", "-z", sha) + if err != nil { + return nil, true, fmt.Errorf("list tree: %w", err) + } + t = &checkoutTree{modes: map[string]string{}, dirs: map[string][]string{}} + for _, rec := range bytes.Split(out, []byte{0}) { + meta, name, ok := strings.Cut(string(rec), "\t") + if !ok || name == "" { + continue + } + fields := strings.Fields(meta) + if len(fields) < 2 { + continue + } + parent := "" + if i := strings.LastIndex(name, "/"); i >= 0 { + parent = name[:i] + } + base := name[strings.LastIndex(name, "/")+1:] + switch fields[1] { + case "tree": + t.dirs[parent] = append(t.dirs[parent], base+"/") + if _, has := t.dirs[name]; !has { + t.dirs[name] = nil + } + case "blob": + t.modes[name] = fields[0] + // The contents API types a symlink as a file, so it is listed here + // too. Only reading one is refused, in regular below. + t.dirs[parent] = append(t.dirs[parent], base) + case "commit": + // A submodule is typed as a file by the contents API as well, and has + // no blob here to read. + t.modes[name] = fields[0] + t.dirs[parent] = append(t.dirs[parent], base) + } + } + for _, names := range t.dirs { + sort.Strings(names) + } + return t, true, nil +} + +// regular reports whether path is a regular file in the tree. A symlink or a +// submodule is not source, and the API path refuses them with its own error. +func (t *checkoutTree) regular(path string) bool { + mode := t.modes[path] + return mode == "100644" || mode == "100755" +} + +// headSHA is the pull request's head commit, asked for once per pull request. +// It serves file and directory reads only. The approval check, Propose and its +// final re-read call PullRequest themselves, so a push during the run is still +// caught there. +// +// A read that names no revision used to fetch the pull request and its head +// commit again for every file, so a review reading four hundred files made +// well over a thousand requests. It also meant a push during the run could +// split the reads across two commits. +func (g *GitHub) headSHA(ctx context.Context, ref Ref) (string, error) { + key := ref.String() + g.index.mu.Lock() + sha, ok := g.index.heads[key] + g.index.mu.Unlock() + if ok { + return sha, nil + } + pr, err := g.PullRequest(ctx, ref) + if err != nil { + return "", err + } + if pr.HeadSHA == "" { + return "", nil + } + g.index.mu.Lock() + if g.index.heads == nil { + g.index.heads = map[string]string{} + } + g.index.heads[key] = pr.HeadSHA + g.index.mu.Unlock() + return pr.HeadSHA, nil +} diff --git a/internal/vcs/github_checkout_test.go b/internal/vcs/github_checkout_test.go new file mode 100644 index 00000000..709decd8 --- /dev/null +++ b/internal/vcs/github_checkout_test.go @@ -0,0 +1,185 @@ +package vcs + +import ( + "context" + "encoding/json" + "net/http" + "os" + "path/filepath" + "slices" + "strings" + "sync/atomic" + "testing" +) + +// countingGitHub serves one head commit and counts what it was asked for. +func countingGitHub(t *testing.T, sha string, pulls, contents *atomic.Int64) *GitHub { + t.Helper() + return newFakeGitHub(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.Contains(r.URL.Path, "/pulls/7"): + pulls.Add(1) + _ = json.NewEncoder(w).Encode(map[string]any{"number": 7, "head": map[string]any{"sha": sha}}) + case strings.Contains(r.URL.Path, "/contents/"): + contents.Add(1) + http.NotFound(w, r) + default: + http.NotFound(w, r) + } + }) +} + +func headOf(t *testing.T, dir string) string { + t.Helper() + return strings.TrimSpace(gitIn(t, dir, "rev-parse", "HEAD")) +} + +// The checkout answers reads of the commit under review, so a repository's +// size no longer sets the number of requests. The pull request is asked for +// once however many files follow. +func TestGitHubReadsTheReviewedCommitFromTheCheckout(t *testing.T) { + dir := newRepo(t) + write(t, dir, "pkg/b.go", "package pkg\n") + gitIn(t, dir, "add", "-A") + gitIn(t, dir, "commit", "-qm", "second") + var pulls, contents atomic.Int64 + gh := countingGitHub(t, headOf(t, dir), &pulls, &contents) + gh.Checkout = dir + + for range 5 { + got, err := gh.FileContent(context.Background(), testRef(), "pkg/b.go") + if err != nil || string(got) != "package pkg\n" { + t.Fatalf("FileContent = %q, %v", got, err) + } + } + names, err := gh.ListDir(context.Background(), testRef(), "") + if err != nil || !slices.Equal(names, []string{"a.go", "pkg/"}) { + t.Fatalf("ListDir = %v, %v", names, err) + } + if pulls.Load() != 1 || contents.Load() != 0 { + t.Fatalf("pull request asked %d times and contents %d times, want 1 and 0", pulls.Load(), contents.Load()) + } +} + +// The commit is read from git objects. A checkout with something else out, or +// an edited file, must not change what is reviewed. +func TestGitHubCheckoutReadsIgnoreTheWorkingTree(t *testing.T) { + dir := newRepo(t) + sha := headOf(t, dir) + write(t, dir, "a.go", "package edited\n") + write(t, dir, "untracked.go", "package untracked\n") + var pulls, contents atomic.Int64 + gh := countingGitHub(t, sha, &pulls, &contents) + gh.Checkout = dir + + got, err := gh.FileContent(context.Background(), testRef(), "a.go") + if err != nil || strings.Contains(string(got), "edited") { + t.Fatalf("read %q, %v: the working tree leaked into the review", got, err) + } + names, err := gh.ListDir(context.Background(), testRef(), "") + if err != nil || contents.Load() != 0 { + t.Fatalf("ListDir = %v, %v after %d API reads: the checkout did not answer, so the isolation below proves nothing", names, err, contents.Load()) + } + if slices.Contains(names, "untracked.go") { + t.Fatalf("ListDir = %v, want only what the commit holds", names) + } +} + +// A git failure after the commit was found may be transient, so it must not be +// remembered. The tree object is hidden for one read and put back; the second +// read has to come from the checkout, not stay on the API for the rest of the +// run. +func TestGitHubRetriesTheCheckoutAfterATransientFailure(t *testing.T) { + dir := newRepo(t) + sha := headOf(t, dir) + tree := strings.TrimSpace(gitIn(t, dir, "rev-parse", sha+"^{tree}")) + object := filepath.Join(dir, ".git", "objects", tree[:2], tree[2:]) + hidden := object + ".hidden" + if err := os.Rename(object, hidden); err != nil { + t.Skipf("tree object is not loose: %v", err) + } + var pulls, contents atomic.Int64 + gh := countingGitHub(t, sha, &pulls, &contents) + gh.Checkout = dir + + _, _ = gh.ListDir(context.Background(), testRef(), "") + if contents.Load() != 1 { + t.Fatalf("API asked %d times while the checkout was broken, want 1", contents.Load()) + } + if err := os.Rename(hidden, object); err != nil { + t.Fatal(err) + } + names, err := gh.ListDir(context.Background(), testRef(), "") + if err != nil || !slices.Equal(names, []string{"a.go"}) || contents.Load() != 1 { + t.Fatalf("ListDir = %v, %v after %d API reads, want the recovered checkout to answer", names, err, contents.Load()) + } +} + +// A checkout that lacks the commit, or a file the checkout cannot serve as +// source, goes to the API as before. +func TestGitHubFallsBackToTheAPIWhenTheCheckoutCannotAnswer(t *testing.T) { + dir := newRepo(t) + if err := os.Symlink("a.go", filepath.Join(dir, "link.go")); err != nil { + t.Skip("symlinks unavailable") + } + gitIn(t, dir, "add", "-A") + gitIn(t, dir, "commit", "-qm", "link") + + var pulls, contents atomic.Int64 + gh := countingGitHub(t, headOf(t, dir), &pulls, &contents) + gh.Checkout = dir + if _, err := gh.FileContent(context.Background(), testRef(), "link.go"); err == nil { + t.Fatal("a symlink was served as source") + } + if contents.Load() != 1 { + t.Fatalf("contents asked %d times for a symlink, want 1: the API must refuse it", contents.Load()) + } + + var pulls2, contents2 atomic.Int64 + absent := countingGitHub(t, strings.Repeat("0", 40), &pulls2, &contents2) + absent.Checkout = dir + _, _ = absent.FileContent(context.Background(), testRef(), "a.go") + _, _ = absent.ListDir(context.Background(), testRef(), "") + if contents2.Load() != 2 { + t.Fatalf("contents asked %d times with a commit the checkout lacks, want 2", contents2.Load()) + } +} + +// The contents API types a symlink as a file, measured against git/git's +// RelNotes. A directory must list the same names whichever side answers. +func TestGitHubListsASymlinkWhetherTheCheckoutOrTheAPIAnswers(t *testing.T) { + dir := newRepo(t) + if err := os.Symlink("a.go", filepath.Join(dir, "link.go")); err != nil { + t.Skip("symlinks unavailable") + } + gitIn(t, dir, "add", "-A") + gitIn(t, dir, "commit", "-qm", "link") + var pulls, contents atomic.Int64 + gh := countingGitHub(t, headOf(t, dir), &pulls, &contents) + gh.Checkout = dir + + names, err := gh.ListDir(context.Background(), testRef(), "") + if err != nil || !slices.Equal(names, []string{"a.go", "link.go"}) { + t.Fatalf("ListDir = %v, %v, want the symlink listed beside its target", names, err) + } +} + +// The contents API types a submodule as a file, measured against apache/arrow's +// cpp/submodules/parquet-testing. The checkout must list it and must not claim +// to read it, since there is no blob behind it. +func TestGitHubListsASubmoduleButDoesNotReadIt(t *testing.T) { + dir := newRepo(t) + gitIn(t, dir, "update-index", "--add", "--cacheinfo", "160000,"+headOf(t, dir)+",vendor") + gitIn(t, dir, "commit", "-qm", "submodule") + var pulls, contents atomic.Int64 + gh := countingGitHub(t, headOf(t, dir), &pulls, &contents) + gh.Checkout = dir + + names, err := gh.ListDir(context.Background(), testRef(), "") + if err != nil || !slices.Equal(names, []string{"a.go", "vendor"}) { + t.Fatalf("ListDir = %v, %v, want the submodule listed as a file", names, err) + } + if _, err := gh.FileContent(context.Background(), testRef(), "vendor"); err == nil || contents.Load() != 1 { + t.Fatalf("FileContent = %v after %d API reads, want the API to answer and refuse", err, contents.Load()) + } +}