Public: anyone can read this; the crew can change it.

The closed pulls list answers under 30 ms: its `words` phase is 71 ms today #8407

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

scout/forge:scout/8388-the-closed-pulls-list-answers-under-30-m → main 4 commits fork

  • Merged. scout/8388-the-closed-pulls-list-answers-under-30-m was merged into main as 235f336.

Review

Sign in to review this change.
5 files changed +252 −3
src/git/merge.ts +28
@@ -321,6 +321,34 @@ export async function ancestorsOf(
321321 return result.ok ? new Set(result.value.split("\n")) : undefined;
322322 }
323323
324 /**
325 * The commits reachable from `oid` but not from `base`, and whether `base`
326 * is itself an ancestor of `oid` — one spawn whose cost is the commits
327 * between them, not the whole history. `base` is an ancestor exactly when
328 * one of the listed commits names it as a parent (or it is `oid`).
329 */
330 export async function ancestorsSince(
331 dir: string,
332 oid: string,
333 base: string,
334 ): Promise<{ added: ReadonlySet<string>; reaches: boolean } | undefined> {
335 if (!isSafeOid(oid) || !isSafeOid(base)) return undefined;
336 if (oid === base) return { added: new Set(), reaches: true };
337 const result = await runGit(["rev-list", "--parents", oid, `^${base}`], {
338 cwd: dir,
339 });
340 if (!result.ok) return undefined;
341 const added = new Set<string>();
342 let reaches = false;
343 for (const line of result.value.split("\n")) {
344 if (line === "") continue;
345 const [commit, ...parents] = line.split(" ");
346 added.add(commit!);
347 if (parents.includes(base)) reaches = true;
348 }
349 return { added, reaches };
350 }
351
324352 /** The commits reachable from head but not base — what a proposal carries. */
325353 export async function aheadOids(
326354 dir: string,
src/web/studio/ancestry.ts +46 −1
@@ -6,23 +6,37 @@
66 * answer longer than a quarter of the bound is not kept at all, so a few
77 * huge repositories cannot hold the process's memory. Holds commit ids
88 * only: nothing in it depends on who is looking.
9 *
10 * A miss with an answer already kept for the same directory (production's
11 * previous commit, after a deploy) asks git only for the commits since it
12 * (`ancestorsSince`) and adds them to the kept set: a few milliseconds where
13 * the whole history costs tens (#8388). When the kept commit is not an
14 * ancestor of the new one — a force push — the whole history is read.
915 */
1016
11 import { ancestorsOf } from "../../git/merge.ts";
17 import { ancestorsOf, ancestorsSince } from "../../git/merge.ts";
1218
1319 type Read = (
1420 dir: string,
1521 oid: string,
1622 ) => Promise<ReadonlySet<string> | undefined>;
1723
24 type Since = (
25 dir: string,
26 oid: string,
27 base: string,
28 ) => Promise<{ added: ReadonlySet<string>; reaches: boolean } | undefined>;
29
1830 export class AncestryCache {
1931 private readonly kept = new Map<string, ReadonlySet<string>>();
2032 private held = 0;
2133 private asked = 0;
34 private extended = 0;
2235
2336 constructor(
2437 private readonly limit: number,
2538 private readonly read: Read = ancestorsOf,
39 private readonly since: Since = ancestorsSince,
2640 ) {}
2741
2842 /** The commits reachable from `oid` in `dir`, from memory when git already said. */
@@ -37,12 +51,38 @@ export class AncestryCache {
3751 this.kept.set(key, hit);
3852 return hit;
3953 }
54 const grown = await this.grow(dir, oid);
55 if (grown !== undefined) {
56 this.remember(key, grown);
57 return grown;
58 }
4059 this.asked++;
4160 const set = await this.read(dir, oid);
4261 if (set !== undefined) this.remember(key, set);
4362 return set;
4463 }
4564
65 /** The ancestry of `oid` as the freshest kept one in `dir` plus the commits since it. */
66 private async grow(
67 dir: string,
68 oid: string,
69 ): Promise<ReadonlySet<string> | undefined> {
70 const prefix = `${dir}\0`;
71 let base: string | undefined;
72 for (const key of this.kept.keys())
73 if (key.startsWith(prefix)) base = key.slice(prefix.length);
74 if (base === undefined) return undefined;
75 this.asked++;
76 const since = await this.since(dir, oid, base);
77 if (since === undefined || !since.reaches) return undefined;
78 const kept = this.kept.get(prefix + base);
79 if (kept === undefined) return undefined; // evicted while git answered
80 this.extended++;
81 const set = new Set(kept);
82 for (const c of since.added) set.add(c);
83 return set;
84 }
85
4686 private remember(key: string, set: ReadonlySet<string>): void {
4787 if (set.size > this.limit / 4 || this.kept.has(key)) return;
4888 this.kept.set(key, set);
@@ -59,6 +99,11 @@ export class AncestryCache {
5999 return this.asked;
60100 }
61101
102 /** How many of those asks grew an answer from a kept one rather than reading the whole history. */
103 get grown(): number {
104 return this.extended;
105 }
106
62107 has(dir: string, oid: string): boolean {
63108 return this.kept.has(`${dir}\0${oid}`);
64109 }
src/web/studio/words.ts +4 −2
@@ -9,7 +9,7 @@
99 */
1010
1111 import type { Database } from "bun:sqlite";
12 import { ancestries } from "./ancestry.ts";
12 import { ancestries, type AncestryCache } from "./ancestry.ts";
1313 import {
1414 NO_TESTS_TAIL,
1515 TESTS_PASSED_TAIL,
@@ -98,6 +98,8 @@ export interface WordCtx {
9898 readonly history: ServingHistory;
9999 /** Who is reading: a private app's production is theirs only if they may enter it. */
100100 readonly viewer: User | undefined;
101 /** Where ancestries are kept; the process's own unless a test brings its own. */
102 readonly ancestries?: AncestryCache;
101103 }
102104
103105 /** What a page read ahead for its rows, so `pullWord` and `runWord` do not read per row. */
@@ -252,7 +254,7 @@ async function caughtUp(
252254 const key = `${ctx.dir}\0${production.commitOid}`;
253255 let ancestry = ctx.history.get(key);
254256 if (ancestry === undefined) {
255 ancestry = ancestries.of(ctx.dir, production.commitOid);
257 ancestry = (ctx.ancestries ?? ancestries).of(ctx.dir, production.commitOid);
256258 ctx.history.set(key, ancestry);
257259 }
258260 return (await ancestry)?.has(commitOid) ?? false;
test/ancestry.test.ts +19
@@ -50,3 +50,22 @@ test("an answer over a quarter of the bound is returned but not kept", async ()
5050 await cache.of("/r", "ok");
5151 expect(cache.has("/r", "ok")).toBe(true);
5252 });
53
54 test("a new commit after a kept one grows from it; one that does not reach it reads the whole history (#8388)", async () => {
55 const { asked, read } = fake({ a: 3, x: 2 });
56 const since = async (_dir: string, oid: string, base: string) =>
57 oid === "x"
58 ? { added: new Set(["x"]), reaches: false }
59 : {
60 added: new Set([oid, `${oid}-1`]),
61 reaches: base === "a" || base === "b",
62 };
63 const cache = new AncestryCache(100, read, since);
64 await cache.of("/r", "a");
65 const b = await cache.of("/r", "b"); // a deploy on top of a
66 expect([...(b ?? [])].sort()).toEqual(["a0", "a1", "a2", "b", "b-1"]);
67 expect(cache.grown).toBe(1);
68 await cache.of("/r", "x"); // force-pushed: b is not its ancestor
69 expect(asked).toEqual(["/r:a", "/r:x"]);
70 expect(cache.reads).toBe(4); // a, b since a, x since b, then x whole
71 });
test/words-closed-batch.test.ts added +155
@@ -0,0 +1,155 @@
1 /**
2 * The closed pulls list is worded as fast as the open one (#8388): a page of
3 * 50 merged and closed pulls, each with a check and a review, reads its words
4 * after production moves on by growing the ancestry production's new commit
5 * needs from the one already kept, one short git walk per deploy rather than
6 * the whole history again — and every word is still the one `pullWord` gives.
7 * The counters prove the short walk; time is only a loose sanity bound.
8 */
9 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
10 import { mkdtemp, rm } from "node:fs/promises";
11 import { tmpdir } from "node:os";
12 import { join } from "node:path";
13 import { createApp, type App } from "../src/app.ts";
14 import { createRepo } from "../src/actions/repos.ts";
15 import { createIssue } from "../src/core/issues.ts";
16 import { listPulls, type Pull } from "../src/core/pulls.ts";
17 import type { Repo } from "../src/core/repos.ts";
18 import { submitReview } from "../src/core/reviews.ts";
19 import { claimRun, createRun, finishRun } from "../src/core/runs.ts";
20 import { getUserByName } from "../src/core/users.ts";
21 import { repoDir } from "../src/git/repos.ts";
22 import { AncestryCache } from "../src/web/studio/ancestry.ts";
23 import { pullWord, pullWords, servingHistory, type WordCtx } from "../src/web/studio/words.ts";
24 import { quietLogs } from "./helpers/globals.ts";
25 import { adminCookie } from "./helpers/admin.ts";
26
27 quietLogs();
28
29 const HISTORY = 3000;
30
31 /** A linear history of `n` empty commits on main, oldest first, in one spawn. */
32 async function seedHistory(dir: string, n: number): Promise<string[]> {
33 let stream = "";
34 for (let i = 1; i <= n; i++) {
35 const msg = `c${i}`;
36 stream += `commit refs/heads/main\nmark :${i}\ncommitter a <[email protected]> ${1_700_000_000 + i} +0000\ndata ${msg.length}\n${msg}\n`;
37 if (i > 1) stream += `from :${i - 1}\n`;
38 stream += "\n";
39 }
40 const imp = Bun.spawn(["git", "fast-import", "--quiet"], { cwd: dir, stdin: "pipe", stdout: "ignore", stderr: "pipe" });
41 imp.stdin.write(stream);
42 await imp.stdin.end();
43 expect(await imp.exited).toBe(0);
44 const list = Bun.spawn(["git", "rev-list", "--reverse", "main"], { cwd: dir, stdout: "pipe" });
45 const out = await new Response(list.stdout).text();
46 expect(await list.exited).toBe(0);
47 return out.trim().split("\n");
48 }
49
50 describe("a page of closed and merged pulls, worded in one batch", () => {
51 let dataDir = "";
52 let app: App;
53 let repo: Repo;
54 let oids: string[] = [];
55 let production = "";
56 // The test's own cache, so no other file's reads move its counts.
57 const ancestries = new AncestryCache(400_000);
58
59 const runtime = {
60 status: () => [{ env: "production", state: "running", commitOid: production }],
61 } as never;
62 const ctx = (): WordCtx => ({
63 db: app.db,
64 dir: repoDir(app.reposRoot, repo.ownerName, repo.name),
65 runtime,
66 record: { id: "app1", repoId: repo.id, access: "public" } as never,
67 history: servingHistory(),
68 viewer: undefined,
69 ancestries,
70 });
71 const page = (): Pull[] => listPulls(app.db, repo.id, { state: "closed", limit: 50 });
72 /** The answer the commit order gives: a merge is served once production is at or past it. */
73 const shippedAt = (prod: string, pulls: readonly Pull[]) =>
74 pulls.filter((p) => p.mergedOid !== null && oids.indexOf(p.mergedOid) <= oids.indexOf(prod)).length;
75
76 beforeAll(async () => {
77 dataDir = await mkdtemp(join(tmpdir(), "forge-words-closed-"));
78 app = createApp({ dataDir, port: 0 });
79 await adminCookie(app, { username: "alice", email: "[email protected]", password: "a-long-words-passphrase" });
80 const alice = getUserByName(app.db, "alice")!;
81 const made = await createRepo(app, alice, { name: "landed", visibility: "public" });
82 if (!made.ok) throw new Error(String(made.error));
83 repo = made.value;
84 oids = await seedHistory(repoDir(app.reposRoot, repo.ownerName, repo.name), HISTORY);
85
86 for (let i = 0; i < 50; i++) {
87 const branch = `feature/p${i}`;
88 // Merged pulls land among the last 60 commits, so a production a few
89 // commits back serves some of them and not the rest.
90 const head = oids[HISTORY - 60 + i]!;
91 const issue = createIssue(app.db, { repoId: repo.id, authorId: alice.id, title: branch, body: "", kind: "pull" });
92 if (!issue.ok) throw new Error(String(issue.error));
93 const id = issue.value.id;
94 app.db
95 .query(
96 `INSERT INTO pull (issue_id, base_branch, head_branch, base_oid, head_oid, head_repo_id)
97 VALUES (?1, 'main', ?2, ?3, ?4, ?5)`,
98 )
99 .run(id, branch, oids[0]!, head, repo.id);
100 const check = createRun(app.db, { repoId: repo.id, kind: "check", issueId: id, env: `pull-${issue.value.number}`, commitOid: head, reason: "pull" });
101 claimRun(app.db, check.id);
102 finishRun(app.db, check.id, { state: i % 5 === 0 ? "failed" : "ok", summary: "checked" });
103 const review = submitReview(app.db, { issueId: id, authorId: alice.id, state: "approved", commitOid: head });
104 if (!review.ok) throw new Error(String(review.error));
105 if (i % 2 === 0) {
106 app.db.query("UPDATE issue SET state = 'closed', closed_at = datetime('now') WHERE id = ?1").run(id);
107 app.db.query("UPDATE pull SET merged_at = datetime('now'), merged_oid = ?2 WHERE issue_id = ?1").run(id, head);
108 } else {
109 app.db
110 .query("UPDATE issue SET state = 'closed', closed_at = datetime('now'), parked = ?2 WHERE id = ?1")
111 .run(id, i % 3 === 0 ? 1 : 0);
112 }
113 }
114 });
115
116 afterAll(async () => {
117 await rm(dataDir, { recursive: true, force: true });
118 });
119
120 test("after a deploy the page's words grow from the kept ancestry, each the one pullWord gives", async () => {
121 const pulls = page();
122 expect(pulls.length).toBe(50);
123 expect(pulls.filter((p) => p.mergedAt !== null).length).toBe(25);
124
125 // The first page after boot walks the whole history once.
126 production = oids[HISTORY - 40]!;
127 const first = await pullWords(ctx(), pulls);
128 expect(first.filter((w) => w.word === "shipped").length).toBe(shippedAt(production, pulls));
129
130 expect(ancestries.reads).toBe(1);
131 expect(ancestries.grown).toBe(0);
132
133 // Then production moves on, a deploy at a time: each new commit is a
134 // miss, answered from the kept ancestry plus the commits since.
135 const timings: number[] = [];
136 const steps = [HISTORY - 30, HISTORY - 25, HISTORY - 20, HISTORY - 15, HISTORY - 10, HISTORY - 6, HISTORY - 3, HISTORY - 1];
137 for (const step of steps) {
138 production = oids[step]!;
139 const at = performance.now();
140 const words = await pullWords(ctx(), pulls);
141 timings.push(performance.now() - at);
142 expect(words.filter((w) => w.word === "shipped").length).toBe(shippedAt(production, pulls));
143 for (const [i, pull] of pulls.entries()) expect(words[i]).toEqual(await pullWord(ctx(), pull));
144 }
145 expect(ancestries.reads).toBe(1 + steps.length); // one short walk per deploy
146 expect(ancestries.grown).toBe(steps.length);
147 // A loose sanity bound only; the counts above are the proof.
148 expect(timings.reduce((a, b) => a + b, 0)).toBeLessThan(2000);
149
150 const last = await pullWords(ctx(), pulls);
151 expect(last.filter((w) => w.word === "shipped").length).toBe(25);
152 expect(last.filter((w) => w.note === "parked by the crew").length).toBeGreaterThan(0);
153 expect(last.filter((w) => w.note === "closed without shipping").length).toBeGreaterThan(0);
154 });
155 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs