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
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/8388-the-closed-pulls-list-answers-under-30-m → main 4 commits fork
5 files
changed +252
−3
src/git/merge.ts +28
| @@ -321,6 +321,34 @@ export async function ancestorsOf( | ||
| 321 | 321 | return result.ok ? new Set(result.value.split("\n")) : undefined; |
| 322 | 322 | } |
| 323 | 323 | |
| 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 | ||
| 324 | 352 | /** The commits reachable from head but not base — what a proposal carries. */ |
| 325 | 353 | export async function aheadOids( |
| 326 | 354 | dir: string, |
src/web/studio/ancestry.ts +46 −1
| @@ -6,23 +6,37 @@ | ||
| 6 | 6 | * answer longer than a quarter of the bound is not kept at all, so a few |
| 7 | 7 | * huge repositories cannot hold the process's memory. Holds commit ids |
| 8 | 8 | * 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. | |
| 9 | 15 | */ |
| 10 | 16 | |
| 11 | import { ancestorsOf } from "../../git/merge.ts"; | |
| 17 | import { ancestorsOf, ancestorsSince } from "../../git/merge.ts"; | |
| 12 | 18 | |
| 13 | 19 | type Read = ( |
| 14 | 20 | dir: string, |
| 15 | 21 | oid: string, |
| 16 | 22 | ) => Promise<ReadonlySet<string> | undefined>; |
| 17 | 23 | |
| 24 | type Since = ( | |
| 25 | dir: string, | |
| 26 | oid: string, | |
| 27 | base: string, | |
| 28 | ) => Promise<{ added: ReadonlySet<string>; reaches: boolean } | undefined>; | |
| 29 | ||
| 18 | 30 | export class AncestryCache { |
| 19 | 31 | private readonly kept = new Map<string, ReadonlySet<string>>(); |
| 20 | 32 | private held = 0; |
| 21 | 33 | private asked = 0; |
| 34 | private extended = 0; | |
| 22 | 35 | |
| 23 | 36 | constructor( |
| 24 | 37 | private readonly limit: number, |
| 25 | 38 | private readonly read: Read = ancestorsOf, |
| 39 | private readonly since: Since = ancestorsSince, | |
| 26 | 40 | ) {} |
| 27 | 41 | |
| 28 | 42 | /** The commits reachable from `oid` in `dir`, from memory when git already said. */ |
| @@ -37,12 +51,38 @@ export class AncestryCache { | ||
| 37 | 51 | this.kept.set(key, hit); |
| 38 | 52 | return hit; |
| 39 | 53 | } |
| 54 | const grown = await this.grow(dir, oid); | |
| 55 | if (grown !== undefined) { | |
| 56 | this.remember(key, grown); | |
| 57 | return grown; | |
| 58 | } | |
| 40 | 59 | this.asked++; |
| 41 | 60 | const set = await this.read(dir, oid); |
| 42 | 61 | if (set !== undefined) this.remember(key, set); |
| 43 | 62 | return set; |
| 44 | 63 | } |
| 45 | 64 | |
| 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 | ||
| 46 | 86 | private remember(key: string, set: ReadonlySet<string>): void { |
| 47 | 87 | if (set.size > this.limit / 4 || this.kept.has(key)) return; |
| 48 | 88 | this.kept.set(key, set); |
| @@ -59,6 +99,11 @@ export class AncestryCache { | ||
| 59 | 99 | return this.asked; |
| 60 | 100 | } |
| 61 | 101 | |
| 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 | ||
| 62 | 107 | has(dir: string, oid: string): boolean { |
| 63 | 108 | return this.kept.has(`${dir}\0${oid}`); |
| 64 | 109 | } |
src/web/studio/words.ts +4 −2
| @@ -9,7 +9,7 @@ | ||
| 9 | 9 | */ |
| 10 | 10 | |
| 11 | 11 | import type { Database } from "bun:sqlite"; |
| 12 | import { ancestries } from "./ancestry.ts"; | |
| 12 | import { ancestries, type AncestryCache } from "./ancestry.ts"; | |
| 13 | 13 | import { |
| 14 | 14 | NO_TESTS_TAIL, |
| 15 | 15 | TESTS_PASSED_TAIL, |
| @@ -98,6 +98,8 @@ export interface WordCtx { | ||
| 98 | 98 | readonly history: ServingHistory; |
| 99 | 99 | /** Who is reading: a private app's production is theirs only if they may enter it. */ |
| 100 | 100 | readonly viewer: User | undefined; |
| 101 | /** Where ancestries are kept; the process's own unless a test brings its own. */ | |
| 102 | readonly ancestries?: AncestryCache; | |
| 101 | 103 | } |
| 102 | 104 | |
| 103 | 105 | /** What a page read ahead for its rows, so `pullWord` and `runWord` do not read per row. */ |
| @@ -252,7 +254,7 @@ async function caughtUp( | ||
| 252 | 254 | const key = `${ctx.dir}\0${production.commitOid}`; |
| 253 | 255 | let ancestry = ctx.history.get(key); |
| 254 | 256 | if (ancestry === undefined) { |
| 255 | ancestry = ancestries.of(ctx.dir, production.commitOid); | |
| 257 | ancestry = (ctx.ancestries ?? ancestries).of(ctx.dir, production.commitOid); | |
| 256 | 258 | ctx.history.set(key, ancestry); |
| 257 | 259 | } |
| 258 | 260 | 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 () | ||
| 50 | 50 | await cache.of("/r", "ok"); |
| 51 | 51 | expect(cache.has("/r", "ok")).toBe(true); |
| 52 | 52 | }); |
| 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 | }); |
new above
offline