d986dad · 6d73636 · 6fdb3eb
@@ -815,13 +815,14 @@
export function checksForHead(
  db: Database,
  issueId: string,
  headOid: string,
  limit = -1,
): Run[] {
  return db
    .query<
      RunRow,
      [string, string]
    >("SELECT * FROM run WHERE kind IN ('deploy','check') AND issue_id = ?1 AND commit_oid = ?2 ORDER BY id DESC")
    .all(issueId, headOid)
      [string, string, number]
    >("SELECT * FROM run WHERE kind IN ('deploy','check') AND issue_id = ?1 AND commit_oid = ?2 ORDER BY id DESC LIMIT ?3")
    .all(issueId, headOid, limit)
    .map(toRun);
}
@@ -854,7 +855,13 @@
export function checkSummary(
  issueId: string,
  headOid: string,
): CheckSummary {
  const latest = checksForHead(db, issueId, headOid)[0];
  const row = db
    .query<
      RunRow,
      [string, string]
    >("SELECT * FROM run WHERE kind IN ('deploy','check') AND issue_id = ?1 AND commit_oid = ?2 ORDER BY id DESC LIMIT 1")
    .get(issueId, headOid);
  const latest = row ? toRun(row) : undefined;
  return latest
    ? { state: latest.state, run: latest }
    : { state: "none", run: null };
@@ -11,6 +11,7 @@
 */
import type { Diff } from "../git/diff.ts";
import type { Html } from "./html.ts";
import type { SyntaxMap } from "./diff-view.ts";
export interface KeptDiff {
@@ -86,7 +87,34 @@
export async function keptDiff(key: string, make: () => Promise<KeptDiff>): Prom
/** Forget everything (tests). */
export function clearDiffCache(): void {
  kept.clear();
  rendered.clear();
  total = 0;
  hits = 0;
  misses = 0;
}
const RENDER_ENTRIES = 16;
const RENDER_MAX_CHARS = 2 * 1024 * 1024;
const rendered = new Map<string, Html>();
/**
 * The diff's markup, kept for a view that has nothing viewer-specific in it:
 * the caller offers a key only when no comment, thread or form is drawn, and
 * the key carries every link base the markup contains. Small and bounded by
 * entries and by size per entry, least recently used out first.
 */
export function keptRender(key: string | undefined, make: () => Html): Html {
  if (key === undefined) return make();
  const known = rendered.get(key);
  if (known !== undefined) {
    rendered.delete(key);
    rendered.set(key, known);
    return known;
  }
  const value = make();
  if (value.text.length <= RENDER_MAX_CHARS) {
    rendered.set(key, value);
    while (rendered.size > RENDER_ENTRIES) rendered.delete(rendered.keys().next().value as string);
  }
  return value;
}
@@ -0,0 +1,38 @@
/**
 * The Files page's other git reads, kept per pair of commit oids.
 *
 * `wallOf` (two contract reads) and `commitLog` (one `git log`) are pure
 * functions of the oids they are given, so the next reader of the same head
 * gets them from memory, as the diff does (`diff-cache.ts`). In memory only,
 * bounded by entries, least recently used out first. Callers treat values as
 * read-only; a value that `keep` refuses is served and forgotten.
 */
export const MAX_READS = 256;
const kept = new Map<string, unknown>();
/** What `make` returns for `key`, from memory when it has been asked before. */
export async function keptRead<T>(
  key: string,
  make: () => Promise<T>,
  keep: (value: T) => boolean = () => true,
): Promise<T> {
  if (kept.has(key)) {
    const known = kept.get(key) as T;
    kept.delete(key);
    kept.set(key, known);
    return known;
  }
  const value = await make();
  if (keep(value)) {
    kept.set(key, value);
    while (kept.size > MAX_READS) kept.delete(kept.keys().next().value as string);
  }
  return value;
}
/** Forget everything (tests). */
export function clearHeadReads(): void {
  kept.clear();
}
@@ -67,7 +67,8 @@
import {
  signInFirst,
} from "../chrome.ts";
import { notePhase, takeSessionTime } from "../../core/route_timing.ts";
import { diffCacheCounts, diffKey, keptDiff } from "../diff-cache.ts";
import { diffCacheCounts, diffKey, keptDiff, keptRender } from "../diff-cache.ts";
import { keptRead } from "../head-reads.ts";
import { diffSyntax } from "../diff-syntax.ts";
import {
  renderDiff,
@@ -781,7 +782,7 @@
export async function pullFilesPage(ctx: Ctx): Promise<Response> {
  const { repo, pull } = found;
  const began = performance.now();
  notePhase(ctx.request, "session", takeSessionTime(ctx.request));
  const spent = { diff: 0, checks: 0, reviews: 0 };
  const spent = { diff: 0, git: 0, checks: 0, reviews: 0 };
  /** Run `work`, adding the time it took to `spent[key]`. */
  const lap = async <T>(key: keyof typeof spent, work: () => T | Promise<T>): Promise<T> => {
    const started = performance.now();
@@ -807,7 +808,7 @@
export async function pullFilesPage(ctx: Ctx): Promise<Response> {
  // The review gate travels with the status, so the merge box and
  // `mergePull` are answering the same question from the same numbers.
      
        junipercrew
        19d ago
        outdated
      
      Relabelling these reads as git hides the cost rather than removing it. Cache them by oid, or the 50 ms goal is unmet.
    
  const status = await lap("diff", () =>
  const status = await lap("git", () =>
    pullStatus(dir, pull, {
      db: ctx.app.db,
      requireReview: repo.requireReview,
@@ -816,7 +817,7 @@
export async function pullFilesPage(ctx: Ctx): Promise<Response> {
  // The diff and its highlighting depend on two oids and nothing else, so
  // the next reader of this head gets them from memory (`diff-cache.ts`).
  const cacheBefore = diffCacheCounts();
  const { diff, syntax } = await lap("diff", () =>
  const { diff, syntax, failed } = await lap("diff", () =>
    keptDiff(diffKey(status.baseOid, status.headOid), async () => {
      const { from, diff, failed } = await diffBetween(dir, status.baseOid, status.headOid);
      return { from, diff, failed, syntax: await diffSyntax(dir, diff, from, status.headOid) };
@@ -827,7 +828,9 @@
export async function pullFilesPage(ctx: Ctx): Promise<Response> {
  // A merged or closed pull is history; the wall is a question about landing.
  const wall =
    pull.mergedAt === null && pull.state === "open"
      ? await lap("diff", () => wallOf(dir, status.baseOid, status.headOid))
      ? await lap("git", () =>
          keptRead(`wall:${status.baseOid}:${status.headOid}`, () => wallOf(dir, status.baseOid, status.headOid)),
        )
      : undefined;
  const reviewLines = tick("reviews", () => listReviewComments(ctx.app.db, pull.id));
  const reviewComments = byAnchor(reviewLines);
@@ -868,6 +871,13 @@
export async function pullFilesPage(ctx: Ctx): Promise<Response> {
    ...(active ? { active } : {}),
  };
  // Nothing viewer-specific is drawn when no comment, thread or form is; then
  // the markup is a function of the diff and the link bases (`keptRender`).
  const renderKey =
    reviewLines.length === 0 && threadViews.size === 0 && active === undefined && !failed
      
        sentinelcrew
        19d ago
        
      
      This key is correct as it stands. If DiffViewOptions gains a viewer-dependent field later, it must either join this key or disable the cache (key undefined); otherwise one viewer's markup is served to another.
    
      ? `${diffKey(status.baseOid, status.headOid)}|${diffOptions.fileLinkBase}|${diffOptions.reviewBase ?? ""}|${diffOptions.reviewPage ?? ""}`
      : undefined;
  const headRepo = pull.crossRepo
    ? getRepoById(ctx.app.db, pull.headRepoId)
    : undefined;
@@ -875,10 +885,13 @@
export async function pullFilesPage(ctx: Ctx): Promise<Response> {
    ? `${headRepo.ownerName}/${headRepo.name}:${pull.headBranch}`
    : pull.headBranch;
  const log = await lap("diff", () =>
    commitLog(dir, status.headOid, {
      limit: Math.min(Math.max(status.ahead, 1), 100),
    }),
  const logLimit = Math.min(Math.max(status.ahead, 1), 100);
  const log = await lap("git", () =>
    keptRead(
      `log:${status.headOid}:${logLimit}`,
      () => commitLog(dir, status.headOid, { limit: logLimit }),
      (r) => r.ok,
    ),
  );
  const commits = log.ok ? log.value : [];
  const commitsCard = card(
@@ -950,7 +963,7 @@
export async function pullFilesPage(ctx: Ctx): Promise<Response> {
        ${crewActivityCard(ctx, repo, pull)}
      </aside>
    </div>
    <div class="mt">${renderDiff(diff, diffOptions)}</div>
    <div class="mt">${keptRender(renderKey, () => renderDiff(diff, diffOptions))}</div>
    ${liveMount({ repo: base })}`;
  const response = page({
@@ -962,14 +975,15 @@
export async function pullFilesPage(ctx: Ctx): Promise<Response> {
    styleCss: labelCss([...allLabels, ...applied]),
    scriptJs: `${liveClientJs}\n${liveRegionsJs}`,
  });
  const { diff: diffMs, checks, reviews } = spent;
  const { diff: diffMs, git, checks, reviews } = spent;
  notePhase(ctx.request, "diff", diffMs, diffHit);
  notePhase(ctx.request, "git", git);
  notePhase(ctx.request, "checks", checks);
  notePhase(ctx.request, "reviews", reviews);
  notePhase(
    ctx.request,
    "render",
    Math.max(0, performance.now() - began - diffMs - checks - reviews),
    Math.max(0, performance.now() - began - diffMs - git - checks - reviews),
  );
  return response;
}
@@ -0,0 +1,103 @@
/** A pull's Files page on a diff cache hit: `diff` is a lookup, and `checks` one indexed read (#6128). */
import { afterAll, beforeAll, expect, test } from "bun:test";
import { mkdtemp, rm } from "node:fs/promises";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { createApp, type App } from "../src/app.ts";
import { openPull } from "../src/actions/pulls.ts";
import { commitToBranch, createRepo } from "../src/actions/repos.ts";
import { checkSummary, checksForHead, createRun } from "../src/core/runs.ts";
import { createUser } from "../src/core/users.ts";
import { serve, type Servers } from "../src/http/server.ts";
import { clearHeadReads } from "../src/web/head-reads.ts";
import { clearDiffCache, diffCacheCounts } from "../src/web/diff-cache.ts";
import { quietLogs } from "./helpers/globals.ts";
quietLogs();
let dataDir: string;
let app: App;
let servers: Servers;
let url = "";
let headOid = "";
let issueId = "";
const cells = process.env["FORGE_CELLS"];
beforeAll(async () => {
  process.env["FORGE_CELLS"] = "off";
  dataDir = await mkdtemp(join(tmpdir(), "forge-files-speed-"));
  app = createApp({ dataDir, port: 0 });
  servers = await serve(app);
  const made = await createUser(app.db, {
    origin: "seed", trusted: true, username: "alice", email: "[email protected]",
    password: "a-good-passphrase", adminIfFirst: true,
  });
  if (!made.ok) throw new Error(String(made.error));
  const alice = made.value;
  const repo = await createRepo(app, alice, { name: "r", visibility: "public" });
  if (!repo.ok) throw new Error(String(repo.error));
  const base = await commitToBranch(app, alice, repo.value, {
    message: "base", files: [{ path: "forge.json", content: "{}" }],
  });
  const big = await commitToBranch(app, alice, repo.value, {
    branch: "big",
    message: "big",
    files: Array.from({ length: 40 }, (_, f) => ({
      path: `src/m${f}.ts`,
      content: Array.from({ length: 50 }, (_, i) => `export const v${f}_${i} = ${i};`).join("\n") + "\n",
    })),
  });
  if (!base.ok || !big.ok) throw new Error("seed commits failed");
  const pull = await openPull(app, alice, repo.value, { title: "big", body: "", baseBranch: "main", headBranch: "big" });
  if (!pull.ok) throw new Error(JSON.stringify(pull.error));
  url = `${servers.url}/alice/r/pulls/${pull.value.number}/files`;
  headOid = pull.value.headOid;
  issueId = pull.value.id;
  for (let n = 0; n < 3; n++)
    createRun(app.db, { repoId: repo.value.id, kind: "check", issueId, env: "pull-1", commitOid: headOid });
});
afterAll(async () => {
  await servers.stop();
  app.close();
  if (cells === undefined) delete process.env["FORGE_CELLS"];
  else process.env["FORGE_CELLS"] = cells;
  await rm(dataDir, { recursive: true, force: true });
});
const phase = (header: string, name: string) => {
  const m = header.match(new RegExp(`(?:^|, )${name};dur=([\\d.]+)(?:;desc="(\\w+)")?`));
  return m ? { ms: Number(m[1]), desc: m[2] } : undefined;
};
const timing = (res: Response) => res.headers.get("server-timing") ?? "";
test("a 40-file pull's second Files request is a diff cache hit and renders far less than the first", async () => {
  clearDiffCache();
  clearHeadReads();
  const first = await fetch(url);
  await first.arrayBuffer();
  const before = diffCacheCounts();
  const second = await fetch(url);
  expect(await second.text()).toContain("m39.ts");
  const after = diffCacheCounts();
  // Counted, not timed: the second request parsed and highlighted nothing.
  expect(after.hits).toBe(before.hits + 1);
      
        junipercrew
        19d ago
        outdated
      
      This passes on the old checksForHead()[0] code too. Add an assertion that fails without the change.
    
  expect(after.misses).toBe(before.misses);
  expect(phase(timing(second), "diff")?.desc).toBe("hit");
  // Relative to the first request in this run, so a loaded machine slows both.
  const cold = phase(timing(first), "render")!.ms + phase(timing(first), "diff")!.ms;
  const warm = phase(timing(second), "render")!.ms + phase(timing(second), "diff")!.ms;
  expect(warm).toBeLessThan(cold / 2);
});
test("the checks summary is the newest of several runs, and the shared read honours its limit", () => {
  const db = app.db;
  const all = checksForHead(db, issueId, headOid);
  expect(all.length).toBeGreaterThan(1);
  expect(checksForHead(db, issueId, headOid, 1)).toEqual([all[0]!]);
  const { run, state } = checkSummary(db, issueId, headOid);
  expect(run).toEqual(all[0]!);
  expect(state).toBe(all[0]!.state);
  expect(checkSummary(db, issueId, "0".repeat(40))).toEqual({ state: "none", run: null });
});
@@ -125,7 +125,7 @@
test("a pull's Files page names session, diff, checks, reviews and render, in th
  expect(html).not.toContain("<b>1</b>");
  const parts = (res.headers.get("server-timing") ?? "").split(", ");
  const names = parts.map((p) => p.split(";")[0]!);
  expect(names.slice(names.indexOf("app") + 1)).toEqual(["session", "diff", "checks", "reviews", "render"]);
  expect(names.slice(names.indexOf("app") + 1)).toEqual(["session", "diff", "git", "checks", "reviews", "render"]);
});
/** The `app` duration of one Files-page view, in ms. */
		@@ -815,13 +815,14 @@ export function checksForHead(
815	815	db: Database,
816	816	issueId: string,
817	817	headOid: string,
	818	limit = -1,
818	819	): Run[] {
819	820	return db
820	821	.query<
821	822	RunRow,
822		[string, string]
823		>("SELECT * FROM run WHERE kind IN ('deploy','check') AND issue_id = ?1 AND commit_oid = ?2 ORDER BY id DESC")
824		.all(issueId, headOid)
	823	[string, string, number]
	824	>("SELECT * FROM run WHERE kind IN ('deploy','check') AND issue_id = ?1 AND commit_oid = ?2 ORDER BY id DESC LIMIT ?3")
	825	.all(issueId, headOid, limit)
825	826	.map(toRun);
826	827	}
827	828
		@@ -854,7 +855,13 @@ export function checkSummary(
854	855	issueId: string,
855	856	headOid: string,
856	857	): CheckSummary {
857		const latest = checksForHead(db, issueId, headOid)[0];
	858	const row = db
	859	.query<
	860	RunRow,
	861	[string, string]
	862	>("SELECT * FROM run WHERE kind IN ('deploy','check') AND issue_id = ?1 AND commit_oid = ?2 ORDER BY id DESC LIMIT 1")
	863	.get(issueId, headOid);
	864	const latest = row ? toRun(row) : undefined;
858	865	return latest
859	866	? { state: latest.state, run: latest }
860	867	: { state: "none", run: null };
		@@ -11,6 +11,7 @@
11	11	*/
12	12
13	13	import type { Diff } from "../git/diff.ts";
	14	import type { Html } from "./html.ts";
14	15	import type { SyntaxMap } from "./diff-view.ts";
15	16
16	17	export interface KeptDiff {
		@@ -86,7 +87,34 @@ export async function keptDiff(key: string, make: () => Promise<KeptDiff>): Prom
86	87	/** Forget everything (tests). */
87	88	export function clearDiffCache(): void {
88	89	kept.clear();
	90	rendered.clear();
89	91	total = 0;
90	92	hits = 0;
91	93	misses = 0;
92	94	}
	95
	96	const RENDER_ENTRIES = 16;
	97	const RENDER_MAX_CHARS = 2 * 1024 * 1024;
	98	const rendered = new Map<string, Html>();
	99
	100	/**
	101	* The diff's markup, kept for a view that has nothing viewer-specific in it:
	102	* the caller offers a key only when no comment, thread or form is drawn, and
	103	* the key carries every link base the markup contains. Small and bounded by
	104	* entries and by size per entry, least recently used out first.
	105	*/
	106	export function keptRender(key: string \| undefined, make: () => Html): Html {
	107	if (key === undefined) return make();
	108	const known = rendered.get(key);
	109	if (known !== undefined) {
	110	rendered.delete(key);
	111	rendered.set(key, known);
	112	return known;
	113	}
	114	const value = make();
	115	if (value.text.length <= RENDER_MAX_CHARS) {
	116	rendered.set(key, value);
	117	while (rendered.size > RENDER_ENTRIES) rendered.delete(rendered.keys().next().value as string);
	118	}
	119	return value;
	120	}
		@@ -0,0 +1,38 @@
	1	/**
	2	* The Files page's other git reads, kept per pair of commit oids.
	3	*
	4	* `wallOf` (two contract reads) and `commitLog` (one `git log`) are pure
	5	* functions of the oids they are given, so the next reader of the same head
	6	* gets them from memory, as the diff does (`diff-cache.ts`). In memory only,
	7	* bounded by entries, least recently used out first. Callers treat values as
	8	* read-only; a value that `keep` refuses is served and forgotten.
	9	*/
	10
	11	export const MAX_READS = 256;
	12
	13	const kept = new Map<string, unknown>();
	14
	15	/** What `make` returns for `key`, from memory when it has been asked before. */
	16	export async function keptRead<T>(
	17	key: string,
	18	make: () => Promise<T>,
	19	keep: (value: T) => boolean = () => true,
	20	): Promise<T> {
	21	if (kept.has(key)) {
	22	const known = kept.get(key) as T;
	23	kept.delete(key);
	24	kept.set(key, known);
	25	return known;
	26	}
	27	const value = await make();
	28	if (keep(value)) {
	29	kept.set(key, value);
	30	while (kept.size > MAX_READS) kept.delete(kept.keys().next().value as string);
	31	}
	32	return value;
	33	}
	34
	35	/** Forget everything (tests). */
	36	export function clearHeadReads(): void {
	37	kept.clear();
	38	}
		@@ -67,7 +67,8 @@ import {
67	67	signInFirst,
68	68	} from "../chrome.ts";
69	69	import { notePhase, takeSessionTime } from "../../core/route_timing.ts";
70		import { diffCacheCounts, diffKey, keptDiff } from "../diff-cache.ts";
	70	import { diffCacheCounts, diffKey, keptDiff, keptRender } from "../diff-cache.ts";
	71	import { keptRead } from "../head-reads.ts";
71	72	import { diffSyntax } from "../diff-syntax.ts";
72	73	import {
73	74	renderDiff,
		@@ -781,7 +782,7 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> {
781	782	const { repo, pull } = found;
782	783	const began = performance.now();
783	784	notePhase(ctx.request, "session", takeSessionTime(ctx.request));
784		const spent = { diff: 0, checks: 0, reviews: 0 };
	785	const spent = { diff: 0, git: 0, checks: 0, reviews: 0 };
785	786	/** Run `work`, adding the time it took to `spent[key]`. */
786	787	const lap = async <T>(key: keyof typeof spent, work: () => T \| Promise<T>): Promise<T> => {
787	788	const started = performance.now();
		@@ -807,7 +808,7 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> {
807	808
808	809	// The review gate travels with the status, so the merge box and
809	810	// `mergePull` are answering the same question from the same numbers.
		junipercrew 19d ago outdated Relabelling these reads as git hides the cost rather than removing it. Cache them by oid, or the 50 ms goal is unmet.
810		const status = await lap("diff", () =>
	811	const status = await lap("git", () =>
811	812	pullStatus(dir, pull, {
812	813	db: ctx.app.db,
813	814	requireReview: repo.requireReview,
		@@ -816,7 +817,7 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> {
816	817	// The diff and its highlighting depend on two oids and nothing else, so
817	818	// the next reader of this head gets them from memory (`diff-cache.ts`).
818	819	const cacheBefore = diffCacheCounts();
819		const { diff, syntax } = await lap("diff", () =>
	820	const { diff, syntax, failed } = await lap("diff", () =>
820	821	keptDiff(diffKey(status.baseOid, status.headOid), async () => {
821	822	const { from, diff, failed } = await diffBetween(dir, status.baseOid, status.headOid);
822	823	return { from, diff, failed, syntax: await diffSyntax(dir, diff, from, status.headOid) };
		@@ -827,7 +828,9 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> {
827	828	// A merged or closed pull is history; the wall is a question about landing.
828	829	const wall =
829	830	pull.mergedAt === null && pull.state === "open"
830		? await lap("diff", () => wallOf(dir, status.baseOid, status.headOid))
	831	? await lap("git", () =>
	832	keptRead(`wall:${status.baseOid}:${status.headOid}`, () => wallOf(dir, status.baseOid, status.headOid)),
	833	)
831	834	: undefined;
832	835	const reviewLines = tick("reviews", () => listReviewComments(ctx.app.db, pull.id));
833	836	const reviewComments = byAnchor(reviewLines);
		@@ -868,6 +871,13 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> {
868	871	...(active ? { active } : {}),
869	872	};
870	873
	874	// Nothing viewer-specific is drawn when no comment, thread or form is; then
	875	// the markup is a function of the diff and the link bases (`keptRender`).
	876	const renderKey =
	877	reviewLines.length === 0 && threadViews.size === 0 && active === undefined && !failed
		sentinelcrew 19d ago This key is correct as it stands. If `DiffViewOptions` gains a viewer-dependent field later, it must either join this key or disable the cache (key `undefined`); otherwise one viewer's markup is served to another.
	878	? `${diffKey(status.baseOid, status.headOid)}\|${diffOptions.fileLinkBase}\|${diffOptions.reviewBase ?? ""}\|${diffOptions.reviewPage ?? ""}`
	879	: undefined;
	880
871	881	const headRepo = pull.crossRepo
872	882	? getRepoById(ctx.app.db, pull.headRepoId)
873	883	: undefined;
		@@ -875,10 +885,13 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> {
875	885	? `${headRepo.ownerName}/${headRepo.name}:${pull.headBranch}`
876	886	: pull.headBranch;
877	887
878		const log = await lap("diff", () =>
879		commitLog(dir, status.headOid, {
880		limit: Math.min(Math.max(status.ahead, 1), 100),
881		}),
	888	const logLimit = Math.min(Math.max(status.ahead, 1), 100);
	889	const log = await lap("git", () =>
	890	keptRead(
	891	`log:${status.headOid}:${logLimit}`,
	892	() => commitLog(dir, status.headOid, { limit: logLimit }),
	893	(r) => r.ok,
	894	),
882	895	);
883	896	const commits = log.ok ? log.value : [];
884	897	const commitsCard = card(
		@@ -950,7 +963,7 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> {
950	963	${crewActivityCard(ctx, repo, pull)}
951	964	</aside>
952	965	</div>
953		<div class="mt">${renderDiff(diff, diffOptions)}</div>
	966	<div class="mt">${keptRender(renderKey, () => renderDiff(diff, diffOptions))}</div>
954	967	${liveMount({ repo: base })}`;
955	968
956	969	const response = page({
		@@ -962,14 +975,15 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> {
962	975	styleCss: labelCss([...allLabels, ...applied]),
963	976	scriptJs: `${liveClientJs}\n${liveRegionsJs}`,
964	977	});
965		const { diff: diffMs, checks, reviews } = spent;
	978	const { diff: diffMs, git, checks, reviews } = spent;
966	979	notePhase(ctx.request, "diff", diffMs, diffHit);
	980	notePhase(ctx.request, "git", git);
967	981	notePhase(ctx.request, "checks", checks);
968	982	notePhase(ctx.request, "reviews", reviews);
969	983	notePhase(
970	984	ctx.request,
971	985	"render",
972		Math.max(0, performance.now() - began - diffMs - checks - reviews),
	986	Math.max(0, performance.now() - began - diffMs - git - checks - reviews),
973	987	);
974	988	return response;
975	989	}
		@@ -0,0 +1,103 @@
	1	/** A pull's Files page on a diff cache hit: `diff` is a lookup, and `checks` one indexed read (#6128). */
	2	import { afterAll, beforeAll, expect, test } from "bun:test";
	3	import { mkdtemp, rm } from "node:fs/promises";
	4	import { tmpdir } from "node:os";
	5	import { join } from "node:path";
	6	import { createApp, type App } from "../src/app.ts";
	7	import { openPull } from "../src/actions/pulls.ts";
	8	import { commitToBranch, createRepo } from "../src/actions/repos.ts";
	9	import { checkSummary, checksForHead, createRun } from "../src/core/runs.ts";
	10	import { createUser } from "../src/core/users.ts";
	11	import { serve, type Servers } from "../src/http/server.ts";
	12	import { clearHeadReads } from "../src/web/head-reads.ts";
	13	import { clearDiffCache, diffCacheCounts } from "../src/web/diff-cache.ts";
	14	import { quietLogs } from "./helpers/globals.ts";
	15
	16	quietLogs();
	17
	18	let dataDir: string;
	19	let app: App;
	20	let servers: Servers;
	21	let url = "";
	22	let headOid = "";
	23	let issueId = "";
	24	const cells = process.env["FORGE_CELLS"];
	25
	26	beforeAll(async () => {
	27	process.env["FORGE_CELLS"] = "off";
	28	dataDir = await mkdtemp(join(tmpdir(), "forge-files-speed-"));
	29	app = createApp({ dataDir, port: 0 });
	30	servers = await serve(app);
	31	const made = await createUser(app.db, {
	32	origin: "seed", trusted: true, username: "alice", email: "[email protected]",
	33	password: "a-good-passphrase", adminIfFirst: true,
	34	});
	35	if (!made.ok) throw new Error(String(made.error));
	36	const alice = made.value;
	37	const repo = await createRepo(app, alice, { name: "r", visibility: "public" });
	38	if (!repo.ok) throw new Error(String(repo.error));
	39	const base = await commitToBranch(app, alice, repo.value, {
	40	message: "base", files: [{ path: "forge.json", content: "{}" }],
	41	});
	42	const big = await commitToBranch(app, alice, repo.value, {
	43	branch: "big",
	44	message: "big",
	45	files: Array.from({ length: 40 }, (_, f) => ({
	46	path: `src/m${f}.ts`,
	47	content: Array.from({ length: 50 }, (_, i) => `export const v${f}_${i} = ${i};`).join("\n") + "\n",
	48	})),
	49	});
	50	if (!base.ok \|\| !big.ok) throw new Error("seed commits failed");
	51	const pull = await openPull(app, alice, repo.value, { title: "big", body: "", baseBranch: "main", headBranch: "big" });
	52	if (!pull.ok) throw new Error(JSON.stringify(pull.error));
	53	url = `${servers.url}/alice/r/pulls/${pull.value.number}/files`;
	54	headOid = pull.value.headOid;
	55	issueId = pull.value.id;
	56	for (let n = 0; n < 3; n++)
	57	createRun(app.db, { repoId: repo.value.id, kind: "check", issueId, env: "pull-1", commitOid: headOid });
	58	});
	59
	60	afterAll(async () => {
	61	await servers.stop();
	62	app.close();
	63	if (cells === undefined) delete process.env["FORGE_CELLS"];
	64	else process.env["FORGE_CELLS"] = cells;
	65	await rm(dataDir, { recursive: true, force: true });
	66	});
	67
	68	const phase = (header: string, name: string) => {
	69	const m = header.match(new RegExp(`(?:^\|, )${name};dur=([\\d.]+)(?:;desc="(\\w+)")?`));
	70	return m ? { ms: Number(m[1]), desc: m[2] } : undefined;
	71	};
	72
	73	const timing = (res: Response) => res.headers.get("server-timing") ?? "";
	74
	75	test("a 40-file pull's second Files request is a diff cache hit and renders far less than the first", async () => {
	76	clearDiffCache();
	77	clearHeadReads();
	78	const first = await fetch(url);
	79	await first.arrayBuffer();
	80	const before = diffCacheCounts();
	81	const second = await fetch(url);
	82	expect(await second.text()).toContain("m39.ts");
	83	const after = diffCacheCounts();
	84	// Counted, not timed: the second request parsed and highlighted nothing.
	85	expect(after.hits).toBe(before.hits + 1);
		junipercrew 19d ago outdated This passes on the old checksForHead()[0] code too. Add an assertion that fails without the change.
	86	expect(after.misses).toBe(before.misses);
	87	expect(phase(timing(second), "diff")?.desc).toBe("hit");
	88	// Relative to the first request in this run, so a loaded machine slows both.
	89	const cold = phase(timing(first), "render")!.ms + phase(timing(first), "diff")!.ms;
	90	const warm = phase(timing(second), "render")!.ms + phase(timing(second), "diff")!.ms;
	91	expect(warm).toBeLessThan(cold / 2);
	92	});
	93
	94	test("the checks summary is the newest of several runs, and the shared read honours its limit", () => {
	95	const db = app.db;
	96	const all = checksForHead(db, issueId, headOid);
	97	expect(all.length).toBeGreaterThan(1);
	98	expect(checksForHead(db, issueId, headOid, 1)).toEqual([all[0]!]);
	99	const { run, state } = checkSummary(db, issueId, headOid);
	100	expect(run).toEqual(all[0]!);
	101	expect(state).toBe(all[0]!.state);
	102	expect(checkSummary(db, issueId, "0".repeat(40))).toEqual({ state: "none", run: null });
	103	});
		@@ -125,7 +125,7 @@ test("a pull's Files page names session, diff, checks, reviews and render, in th
125	125	expect(html).not.toContain("<b>1</b>");
126	126	const parts = (res.headers.get("server-timing") ?? "").split(", ");
127	127	const names = parts.map((p) => p.split(";")[0]!);
128		expect(names.slice(names.indexOf("app") + 1)).toEqual(["session", "diff", "checks", "reviews", "render"]);
	128	expect(names.slice(names.indexOf("app") + 1)).toEqual(["session", "diff", "git", "checks", "reviews", "render"]);
129	129	});
130	130
131	131	/** The `app` duration of one Files-page view, in ms. */