a0665be · 06897fb
@@ -10,7 +10,7 @@
import { getRepoById } from "./repos.ts";
import { repoDir } from "../git/repos.ts";
import { sealedByNumber } from "./sealed.ts";
import type { Database } from "bun:sqlite";
import { diffRange, type Diff } from "../git/diff.ts";
import { diffRange, type Diff, type DiffLaps } from "../git/diff.ts";
import { isSafeRef } from "../git/guard.ts";
import { refTip } from "../git/read.ts";
import { readTips } from "../git/tips.ts";
@@ -685,9 +685,12 @@
export async function diffBetween(
  dir: string,
  baseOid: string,
  headOid: string,
  laps?: DiffLaps,
): Promise<{ from: string; diff: Diff; failed: boolean }> {
  const started = performance.now();
  const from = (await mergeBase(dir, baseOid, headOid)) ?? baseOid;
  const result = await diffRange(dir, from, headOid);
  if (laps) laps.git = performance.now() - started;
  const result = await diffRange(dir, from, headOid, laps);
  return result.ok
    ? { from, diff: result.value, failed: false }
    : { from, diff: { files: [], additions: 0, deletions: 0 }, failed: true };
@@ -73,10 +73,11 @@
export async function diffRange(
  dir: string,
  base: string,
  head: string,
  laps?: DiffLaps,
): Promise<Result<Diff, GitFailure | "unsafe_rev">> {
  if (!safeRange(base, head)) return err("unsafe_rev");
  // `git diff A B` compares two tree-ish and needs no work tree.
  return runDiff(dir, ["diff", ...DIFF_FLAGS, base, head, "--"]);
  return runDiff(dir, ["diff", ...DIFF_FLAGS, base, head, "--"], laps);
}
/** Diff a commit against its first parent (or the empty tree for a root). */
@@ -101,13 +102,25 @@
export async function diffCommit(
  ]);
}
/** Where a diff's time went, in ms: git's own run and our parse of its text. */
export interface DiffLaps {
  git?: number;
  parse?: number;
}
async function runDiff(
  dir: string,
  args: readonly string[],
  laps?: DiffLaps,
): Promise<Result<Diff, GitFailure>> {
  const started = performance.now();
  const raw = await runGitRaw(args, { cwd: dir, timeoutMs: 60_000 });
  const parsing = performance.now();
  if (laps) laps.git = (laps.git ?? 0) + parsing - started;
  if (!raw.ok) return raw;
  return ok(parseUnifiedDiff(new TextDecoder().decode(raw.value)));
  const diff = parseUnifiedDiff(new TextDecoder().decode(raw.value));
  if (laps) laps.parse = (laps.parse ?? 0) + performance.now() - parsing;
  return ok(diff);
}
// ---- the parser ----
@@ -66,6 +66,7 @@
import {
  signInFirst,
} from "../chrome.ts";
import { notePhase, takeSessionTime } from "../../core/route_timing.ts";
import type { DiffLaps } from "../../git/diff.ts";
import { diffCacheCounts, diffKey, keptDiff, keptRender } from "../diff-cache.ts";
import { keptRead } from "../head-reads.ts";
import { diffSyntax } from "../diff-syntax.ts";
@@ -832,11 +833,16 @@
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`).
  // On a miss the stretches say where the time went; a hit has none.
  const stretches: DiffLaps & { highlight?: number } = {};
  const cacheBefore = diffCacheCounts();
  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) };
      const { from, diff, failed } = await diffBetween(dir, status.baseOid, status.headOid, stretches);
      const highlighting = performance.now();
      const syntax = await diffSyntax(dir, diff, from, status.headOid);
      stretches.highlight = performance.now() - highlighting;
      return { from, diff, failed, syntax };
    }),
  );
  const cacheAfter = diffCacheCounts();
@@ -993,6 +999,7 @@
export async function pullFilesPage(ctx: Ctx): Promise<Response> {
  });
  const { diff: diffMs, git, checks, reviews } = spent;
  notePhase(ctx.request, "diff", diffMs, diffHit);
  for (const [name, ms] of Object.entries(stretches)) notePhase(ctx.request, `diff-${name}`, ms);
  notePhase(ctx.request, "git", git);
  notePhase(ctx.request, "checks", checks);
  notePhase(ctx.request, "reviews", reviews);
@@ -0,0 +1,98 @@
/** The Files page's `diff` phase names its stretches, and a big diff is cheap on the second read (#6203). */
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 { addReview, openPull } from "../src/actions/pulls.ts";
import { commitToBranch, createRepo } from "../src/actions/repos.ts";
import { createUser } from "../src/core/users.ts";
import { serve, type Servers } from "../src/http/server.ts";
import { clearDiffCache } from "../src/web/diff-cache.ts";
import { clearHeadReads } from "../src/web/head-reads.ts";
import { quietLogs } from "./helpers/globals.ts";
quietLogs();
let dataDir: string;
let app: App;
let servers: Servers;
let url = "";
const cells = process.env["FORGE_CELLS"];
const FILES = 20;
const LINES = 150;
const lineText = (f: number, i: number) => `export const v${f}_${i} = ${i} * ${f};`;
beforeAll(async () => {
  process.env["FORGE_CELLS"] = "off";
  dataDir = await mkdtemp(join(tmpdir(), "forge-files-stretches-"));
  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: FILES }, (_, f) => ({
      path: `src/m${f}.ts`,
      content: Array.from({ length: LINES }, (_, i) => lineText(f, i + 1)).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`;
  const review = await addReview(app, alice, repo.value, pull.value, {
    body: "anchor-survives-here", path: "src/m7.ts", line: 100, side: "new",
  });
  if (!review.ok) throw new Error(JSON.stringify(review.error));
});
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.]+)`));
  return m ? Number(m[1]) : undefined;
};
test("a 3,000-line, 20-file diff names its stretches cold and draws under 30 ms warm, comments on their lines", async () => {
  clearDiffCache();
  clearHeadReads();
  const first = await fetch(url);
  await first.arrayBuffer();
  const cold = first.headers.get("server-timing") ?? "";
  for (const stretch of ["diff-git", "diff-parse", "diff-highlight"])
    expect(phase(cold, stretch)).toBeGreaterThanOrEqual(0);
  const second = await fetch(url);
  // Highlighting wraps tokens in spans; the rows read as text without them.
  const text = (await second.text()).replace(/<[^>]*>/g, "");
  const warm = second.headers.get("server-timing") ?? "";
  expect(warm).toContain('diff;dur=');
  expect(phase(warm, "diff")!).toBeLessThan(30);
  // A hit did no git, parsing or highlighting, so it names none.
  expect(phase(warm, "diff-git")).toBeUndefined();
  // The comment sits right after line 100 of m7.ts, and no other line's.
  const at = text.indexOf("anchor-survives-here");
  expect(at).toBeGreaterThan(0);
  const before = text.slice(0, at);
  expect(before.lastIndexOf("v7_100 =")).toBeGreaterThan(before.lastIndexOf("v7_99 ="));
  expect(before.lastIndexOf("v7_100 =")).toBeGreaterThan(before.lastIndexOf("v19_150 ="));
  expect(text.indexOf("v7_101 =")).toBeGreaterThan(at);
});
@@ -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", "git", "checks", "reviews", "render"]);
  expect(names.slice(names.indexOf("app") + 1)).toEqual(["session", "diff", "diff-git", "diff-parse", "diff-highlight", "git", "checks", "reviews", "render"]);
});
/** The `app` duration of one Files-page view, in ms. */
		@@ -10,7 +10,7 @@ import { getRepoById } from "./repos.ts";
10	10	import { repoDir } from "../git/repos.ts";
11	11	import { sealedByNumber } from "./sealed.ts";
12	12	import type { Database } from "bun:sqlite";
13		import { diffRange, type Diff } from "../git/diff.ts";
	13	import { diffRange, type Diff, type DiffLaps } from "../git/diff.ts";
14	14	import { isSafeRef } from "../git/guard.ts";
15	15	import { refTip } from "../git/read.ts";
16	16	import { readTips } from "../git/tips.ts";
		@@ -685,9 +685,12 @@ export async function diffBetween(
685	685	dir: string,
686	686	baseOid: string,
687	687	headOid: string,
	688	laps?: DiffLaps,
688	689	): Promise<{ from: string; diff: Diff; failed: boolean }> {
	690	const started = performance.now();
689	691	const from = (await mergeBase(dir, baseOid, headOid)) ?? baseOid;
690		const result = await diffRange(dir, from, headOid);
	692	if (laps) laps.git = performance.now() - started;
	693	const result = await diffRange(dir, from, headOid, laps);
691	694	return result.ok
692	695	? { from, diff: result.value, failed: false }
693	696	: { from, diff: { files: [], additions: 0, deletions: 0 }, failed: true };
		@@ -73,10 +73,11 @@ export async function diffRange(
73	73	dir: string,
74	74	base: string,
75	75	head: string,
	76	laps?: DiffLaps,
76	77	): Promise<Result<Diff, GitFailure \| "unsafe_rev">> {
77	78	if (!safeRange(base, head)) return err("unsafe_rev");
78	79	// `git diff A B` compares two tree-ish and needs no work tree.
79		return runDiff(dir, ["diff", ...DIFF_FLAGS, base, head, "--"]);
	80	return runDiff(dir, ["diff", ...DIFF_FLAGS, base, head, "--"], laps);
80	81	}
81	82
82	83	/** Diff a commit against its first parent (or the empty tree for a root). */
		@@ -101,13 +102,25 @@ export async function diffCommit(
101	102	]);
102	103	}
103	104
	105	/** Where a diff's time went, in ms: git's own run and our parse of its text. */
	106	export interface DiffLaps {
	107	git?: number;
	108	parse?: number;
	109	}
	110
104	111	async function runDiff(
105	112	dir: string,
106	113	args: readonly string[],
	114	laps?: DiffLaps,
107	115	): Promise<Result<Diff, GitFailure>> {
	116	const started = performance.now();
108	117	const raw = await runGitRaw(args, { cwd: dir, timeoutMs: 60_000 });
	118	const parsing = performance.now();
	119	if (laps) laps.git = (laps.git ?? 0) + parsing - started;
109	120	if (!raw.ok) return raw;
110		return ok(parseUnifiedDiff(new TextDecoder().decode(raw.value)));
	121	const diff = parseUnifiedDiff(new TextDecoder().decode(raw.value));
	122	if (laps) laps.parse = (laps.parse ?? 0) + performance.now() - parsing;
	123	return ok(diff);
111	124	}
112	125
113	126	// ---- the parser ----
		@@ -66,6 +66,7 @@ import {
66	66	signInFirst,
67	67	} from "../chrome.ts";
68	68	import { notePhase, takeSessionTime } from "../../core/route_timing.ts";
	69	import type { DiffLaps } from "../../git/diff.ts";
69	70	import { diffCacheCounts, diffKey, keptDiff, keptRender } from "../diff-cache.ts";
70	71	import { keptRead } from "../head-reads.ts";
71	72	import { diffSyntax } from "../diff-syntax.ts";
		@@ -832,11 +833,16 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> {
832	833	);
833	834	// The diff and its highlighting depend on two oids and nothing else, so
834	835	// the next reader of this head gets them from memory (`diff-cache.ts`).
	836	// On a miss the stretches say where the time went; a hit has none.
	837	const stretches: DiffLaps & { highlight?: number } = {};
835	838	const cacheBefore = diffCacheCounts();
836	839	const { diff, syntax, failed } = await lap("diff", () =>
837	840	keptDiff(diffKey(status.baseOid, status.headOid), async () => {
838		const { from, diff, failed } = await diffBetween(dir, status.baseOid, status.headOid);
839		return { from, diff, failed, syntax: await diffSyntax(dir, diff, from, status.headOid) };
	841	const { from, diff, failed } = await diffBetween(dir, status.baseOid, status.headOid, stretches);
	842	const highlighting = performance.now();
	843	const syntax = await diffSyntax(dir, diff, from, status.headOid);
	844	stretches.highlight = performance.now() - highlighting;
	845	return { from, diff, failed, syntax };
840	846	}),
841	847	);
842	848	const cacheAfter = diffCacheCounts();
		@@ -993,6 +999,7 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> {
993	999	});
994	1000	const { diff: diffMs, git, checks, reviews } = spent;
995	1001	notePhase(ctx.request, "diff", diffMs, diffHit);
	1002	for (const [name, ms] of Object.entries(stretches)) notePhase(ctx.request, `diff-${name}`, ms);
996	1003	notePhase(ctx.request, "git", git);
997	1004	notePhase(ctx.request, "checks", checks);
998	1005	notePhase(ctx.request, "reviews", reviews);
		@@ -0,0 +1,98 @@
	1	/** The Files page's `diff` phase names its stretches, and a big diff is cheap on the second read (#6203). */
	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 { addReview, openPull } from "../src/actions/pulls.ts";
	8	import { commitToBranch, createRepo } from "../src/actions/repos.ts";
	9	import { createUser } from "../src/core/users.ts";
	10	import { serve, type Servers } from "../src/http/server.ts";
	11	import { clearDiffCache } from "../src/web/diff-cache.ts";
	12	import { clearHeadReads } from "../src/web/head-reads.ts";
	13	import { quietLogs } from "./helpers/globals.ts";
	14
	15	quietLogs();
	16
	17	let dataDir: string;
	18	let app: App;
	19	let servers: Servers;
	20	let url = "";
	21	const cells = process.env["FORGE_CELLS"];
	22	const FILES = 20;
	23	const LINES = 150;
	24	const lineText = (f: number, i: number) => `export const v${f}_${i} = ${i} * ${f};`;
	25
	26	beforeAll(async () => {
	27	process.env["FORGE_CELLS"] = "off";
	28	dataDir = await mkdtemp(join(tmpdir(), "forge-files-stretches-"));
	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: FILES }, (_, f) => ({
	46	path: `src/m${f}.ts`,
	47	content: Array.from({ length: LINES }, (_, i) => lineText(f, i + 1)).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	const review = await addReview(app, alice, repo.value, pull.value, {
	55	body: "anchor-survives-here", path: "src/m7.ts", line: 100, side: "new",
	56	});
	57	if (!review.ok) throw new Error(JSON.stringify(review.error));
	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.]+)`));
	70	return m ? Number(m[1]) : undefined;
	71	};
	72
	73	test("a 3,000-line, 20-file diff names its stretches cold and draws under 30 ms warm, comments on their lines", async () => {
	74	clearDiffCache();
	75	clearHeadReads();
	76	const first = await fetch(url);
	77	await first.arrayBuffer();
	78	const cold = first.headers.get("server-timing") ?? "";
	79	for (const stretch of ["diff-git", "diff-parse", "diff-highlight"])
	80	expect(phase(cold, stretch)).toBeGreaterThanOrEqual(0);
	81
	82	const second = await fetch(url);
	83	// Highlighting wraps tokens in spans; the rows read as text without them.
	84	const text = (await second.text()).replace(/<[^>]*>/g, "");
	85	const warm = second.headers.get("server-timing") ?? "";
	86	expect(warm).toContain('diff;dur=');
	87	expect(phase(warm, "diff")!).toBeLessThan(30);
	88	// A hit did no git, parsing or highlighting, so it names none.
	89	expect(phase(warm, "diff-git")).toBeUndefined();
	90
	91	// The comment sits right after line 100 of m7.ts, and no other line's.
	92	const at = text.indexOf("anchor-survives-here");
	93	expect(at).toBeGreaterThan(0);
	94	const before = text.slice(0, at);
	95	expect(before.lastIndexOf("v7_100 =")).toBeGreaterThan(before.lastIndexOf("v7_99 ="));
	96	expect(before.lastIndexOf("v7_100 =")).toBeGreaterThan(before.lastIndexOf("v19_150 ="));
	97	expect(text.indexOf("v7_101 =")).toBeGreaterThan(at);
	98	});
		@@ -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", "git", "checks", "reviews", "render"]);
	128	expect(names.slice(names.indexOf("app") + 1)).toEqual(["session", "diff", "diff-git", "diff-parse", "diff-highlight", "git", "checks", "reviews", "render"]);
129	129	});
130	130
131	131	/** The `app` duration of one Files-page view, in ms. */