Compile only the includers a header change can affect
A verification of a header template compiled every includer (705 for stpl.h) and a merge every includer of every submitted header (up to 1199 per header), even when the header only differed in line endings; one such job took up to an hour and held up every other batch. Now: - merge: headers unchanged from main are skipped; for changed ones, the includers whose source mentions a name the change touched (identifiers on changed lines, the enclosing class/struct), all of them only when that is more than 300; - verification: the includers that mention the template arguments of the annotation (or the class), at most 16. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
1038c1d390
commit
a9eb651688
3 changed files with 184 additions and 7 deletions
122
src/server/affected.js
Normal file
122
src/server/affected.js
Normal file
|
|
@ -0,0 +1,122 @@
|
||||||
|
// Which units a header change can affect. Compiling every includer of a widely used header (700 to
|
||||||
|
// 1200 units for stpl.h and friends) took half an hour or more per batch and held up everyone
|
||||||
|
// else. A unit can only compile differently when its source mentions something the change
|
||||||
|
// touched: a name on a changed line, or the class/struct/union/enum the changed lines sit in
|
||||||
|
// (a new field moves every offset after it). Those are found with git grep over the includers.
|
||||||
|
//
|
||||||
|
// The judgement is textual, so a unit that reaches a changed type only through another header's
|
||||||
|
// inline code is missed; such a unit is compiled again whenever its own file or any of its
|
||||||
|
// headers change, and by the full rebuild.
|
||||||
|
|
||||||
|
const KEYWORDS = new Set(
|
||||||
|
`alignas alignof and asm auto bool break case catch char class const constexpr const_cast continue decltype default delete do double
|
||||||
|
dynamic_cast else enum explicit export extern false float for friend goto if inline int long mutable namespace new noexcept not nullptr
|
||||||
|
operator or private protected public register reinterpret_cast return short signed sizeof static static_assert static_cast struct switch
|
||||||
|
template this throw true try typedef typeid typename union unsigned using virtual void volatile wchar_t while define ifdef ifndef endif
|
||||||
|
include pragma undef elif line error __cdecl __stdcall __fastcall __thiscall __forceinline __inline __declspec int8_t int16_t int32_t
|
||||||
|
int64_t uint8_t uint16_t uint32_t uint64_t size_t NULL TRUE FALSE BOOL DWORD WORD BYTE INT UINT LONG ULONG VOID FUNCTION INLINED STUB`.split(/\s+/),
|
||||||
|
);
|
||||||
|
|
||||||
|
// Whole identifiers only (not the "x16" of 0x16).
|
||||||
|
const IDENT = /(?<![A-Za-z0-9_])[A-Za-z_][A-Za-z0-9_]{2,}/g;
|
||||||
|
|
||||||
|
// Plain lowercase words on a changed line are parameter and local names (rect, count, key):
|
||||||
|
// they say nothing about which units use the change and match nearly every file. Names with a
|
||||||
|
// capital, a digit or an underscore (CRect, m_buffer, unk_0124) are kept.
|
||||||
|
const telling = (w) => !KEYWORDS.has(w) && /[A-Z0-9_]/.test(w);
|
||||||
|
|
||||||
|
// The code of a line: no comments, no #include (its path is not code).
|
||||||
|
function code(line, state) {
|
||||||
|
let s = line;
|
||||||
|
if (state.block) {
|
||||||
|
const end = s.indexOf('*/');
|
||||||
|
if (end < 0) return '';
|
||||||
|
s = s.slice(end + 2);
|
||||||
|
state.block = false;
|
||||||
|
}
|
||||||
|
s = s.replace(/\/\*.*?\*\//g, ' ');
|
||||||
|
const open = s.indexOf('/*');
|
||||||
|
if (open >= 0) {
|
||||||
|
state.block = true;
|
||||||
|
s = s.slice(0, open);
|
||||||
|
}
|
||||||
|
s = s.replace(/\/\/.*$/, '');
|
||||||
|
if (/^\s*#\s*include\b/.test(s)) return '';
|
||||||
|
// String and character literals are text, not names.
|
||||||
|
return s.replace(/"(?:[^"\\]|\\.)*"/g, '""').replace(/'(?:[^'\\]|\\.)*'/g, "''");
|
||||||
|
}
|
||||||
|
|
||||||
|
/** For each line, the innermost named class/struct/union/enum it is inside (or null). */
|
||||||
|
function enclosingTypes(lines) {
|
||||||
|
const out = new Array(lines.length).fill(null);
|
||||||
|
const stack = []; // names (or null for other braces)
|
||||||
|
let pending = null; // a type head seen, waiting for its opening brace
|
||||||
|
for (let i = 0; i < lines.length; i++) {
|
||||||
|
const line = lines[i].replace(/\/\/.*$/, '').replace(/"(?:[^"\\]|\\.)*"/g, '""');
|
||||||
|
// "class T" in a template parameter list is not a type being defined.
|
||||||
|
const head = /\b(?:class|struct|union|enum)\s+(?:__declspec\([^)]*\)\s*)?([A-Za-z_]\w*)[^;]*$/.exec(line.replace(/template\s*<[^>]*>/g, ''));
|
||||||
|
if (head && head[1].length > 2) pending = head[1];
|
||||||
|
out[i] = [...stack].reverse().find(Boolean) ?? pending ?? null;
|
||||||
|
for (const c of line) {
|
||||||
|
if (c === '{') {
|
||||||
|
stack.push(pending);
|
||||||
|
pending = null;
|
||||||
|
} else if (c === '}') stack.pop();
|
||||||
|
else if (c === ';' && pending && !line.includes('{')) pending = null; // a forward declaration
|
||||||
|
}
|
||||||
|
if (out[i] === null) out[i] = [...stack].reverse().find(Boolean) ?? null;
|
||||||
|
}
|
||||||
|
return out;
|
||||||
|
}
|
||||||
|
|
||||||
|
/** Names a header change touches: identifiers on changed lines and the types around them. */
|
||||||
|
export function changedNames(before, after) {
|
||||||
|
const a = (before ?? '').replace(/\r\n/g, '\n').split('\n');
|
||||||
|
const b = (after ?? '').replace(/\r\n/g, '\n').split('\n');
|
||||||
|
const inA = new Map();
|
||||||
|
const inB = new Map();
|
||||||
|
for (const l of a) inA.set(l, (inA.get(l) ?? 0) + 1);
|
||||||
|
for (const l of b) inB.set(l, (inB.get(l) ?? 0) + 1);
|
||||||
|
const names = new Set();
|
||||||
|
const take = (lines, other) => {
|
||||||
|
const types = enclosingTypes(lines);
|
||||||
|
const seen = new Map();
|
||||||
|
const state = { block: false };
|
||||||
|
lines.forEach((l, i) => {
|
||||||
|
const c = code(l, state);
|
||||||
|
seen.set(l, (seen.get(l) ?? 0) + 1);
|
||||||
|
// A line that is new (or appears more often than before) is a changed line.
|
||||||
|
if (seen.get(l) <= (other.get(l) ?? 0) || !c.trim()) return;
|
||||||
|
for (const m of c.match(IDENT) ?? []) if (telling(m)) names.add(m);
|
||||||
|
if (types[i]) names.add(types[i]);
|
||||||
|
});
|
||||||
|
};
|
||||||
|
take(b, inA);
|
||||||
|
take(a, inB);
|
||||||
|
return [...names];
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Of `units`, those whose source mentions any of `names` (git grep -w over HEAD's tree plus the
|
||||||
|
* submitted files). Returns null when the change is too broad to narrow down (every includer
|
||||||
|
* is compiled then).
|
||||||
|
*/
|
||||||
|
export function unitsMentioning(repo, units, names, files = {}, { maxNames = 400 } = {}) {
|
||||||
|
if (!names.length || !units.length) return [];
|
||||||
|
if (names.length > maxNames) return null;
|
||||||
|
const hits = new Set();
|
||||||
|
const inSubmission = units.filter((u) => typeof files[u] === 'string');
|
||||||
|
for (const u of inSubmission) if (names.some((n) => new RegExp(`\\b${n}\\b`).test(files[u]))) hits.add(u);
|
||||||
|
const onMain = units.filter((u) => typeof files[u] !== 'string');
|
||||||
|
// In chunks: the pathspec list can be long.
|
||||||
|
for (let i = 0; i < onMain.length; i += 400) {
|
||||||
|
const part = onMain.slice(i, i + 400);
|
||||||
|
try {
|
||||||
|
const out = repo.git(['grep', '-l', '-w', '-F', ...names.flatMap((n) => ['-e', n]), 'HEAD', '--', ...part]);
|
||||||
|
for (const line of out.split('\n')) if (line.startsWith('HEAD:')) hits.add(line.slice(5));
|
||||||
|
} catch {
|
||||||
|
// git grep exits 1 when nothing matches
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return [...hits];
|
||||||
|
}
|
||||||
|
|
@ -9,6 +9,8 @@ import { checkUnits } from '../shared/check.js';
|
||||||
import { reconcile } from '../shared/reconcile.js';
|
import { reconcile } from '../shared/reconcile.js';
|
||||||
import { STATUS } from '../shared/compare.js';
|
import { STATUS } from '../shared/compare.js';
|
||||||
import { listSources, isUnit } from '../shared/project.js';
|
import { listSources, isUnit } from '../shared/project.js';
|
||||||
|
import { changedNames, unitsMentioning } from './affected.js';
|
||||||
|
import { className } from './grouping.js';
|
||||||
import { now } from './db.js';
|
import { now } from './db.js';
|
||||||
import { returnBatch, batchAttempts, settleBatch } from './batches.js';
|
import { returnBatch, batchAttempts, settleBatch } from './batches.js';
|
||||||
import { analyzeInWorkers } from './analyze.js';
|
import { analyzeInWorkers } from './analyze.js';
|
||||||
|
|
@ -151,9 +153,27 @@ export class Verifier {
|
||||||
if (at && isUnit(at)) units.add(at);
|
if (at && isUnit(at)) units.add(at);
|
||||||
else unknown++;
|
else unknown++;
|
||||||
}
|
}
|
||||||
// Never compiled before and defined in a changed header: any includer may be the one.
|
// Never compiled before and defined in a changed header: the includers that mention its template
|
||||||
|
// arguments or class are the ones that can instantiate it (every includer took up to an hour).
|
||||||
if (unknown && !dependents && changedHeaders.length) {
|
if (unknown && !dependents && changedHeaders.length) {
|
||||||
for (const h of changedHeaders) for (const r of this.store.all('SELECT unit FROM unit_deps WHERE header = ?', h)) units.add(r.unit);
|
// The template arguments of the annotation (<CMapObj, HASHKEY_NONE>) are the sharp test: the
|
||||||
|
// class (TSHashTable) is mentioned by many more units. The class only when there are none.
|
||||||
|
const args = new Set();
|
||||||
|
const classes = new Set();
|
||||||
|
for (const id of targets) {
|
||||||
|
for (const h of changedHeaders) {
|
||||||
|
const m = new RegExp(`FUNCTION:\\s*${id}\\s*<([^>\\n]*)>`).exec(String(files[h] ?? ''));
|
||||||
|
for (const n of m?.[1].match(/[A-Za-z_]\w{2,}/g) ?? []) args.add(n);
|
||||||
|
}
|
||||||
|
const cls = className(this.store.get('SELECT name FROM symbols WHERE id = ?', id)?.name);
|
||||||
|
if (cls) classes.add(cls.replace(/<.*$/, '').split('::').pop());
|
||||||
|
}
|
||||||
|
const names = [...(args.size ? args : classes)];
|
||||||
|
for (const h of changedHeaders) {
|
||||||
|
const deps = this.store.all('SELECT unit FROM unit_deps WHERE header = ? ORDER BY unit', h).map((r) => r.unit);
|
||||||
|
const hits = names.length ? unitsMentioning(this.repo, deps, names, files) ?? [] : [];
|
||||||
|
for (const u of (hits.length ? hits : deps).slice(0, 16)) units.add(u);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -228,12 +248,18 @@ export class Verifier {
|
||||||
// A unit the merged files add that verification did not compile.
|
// A unit the merged files add that verification did not compile.
|
||||||
const known = new Set(saved.check.units.map((u) => u.file));
|
const known = new Set(saved.check.units.map((u) => u.file));
|
||||||
for (const f of Object.keys(files)) if (isUnit(f) && !known.has(f)) stale.push({ file: f });
|
for (const f of Object.keys(files)) if (isUnit(f) && !known.has(f)) stale.push({ file: f });
|
||||||
// Verification compiled only the submitted units; nothing reaches main without every includer
|
// Verification compiled only the submitted units. Before a header change reaches main, the
|
||||||
// of a changed header compiled and checked for regressions.
|
// includers it can affect are compiled and checked for regressions: the ones that mention a
|
||||||
|
// name it touched (affected.js); every includer when the change is too broad to narrow down.
|
||||||
|
const head = this.repo.head();
|
||||||
for (const h of Object.keys(files).filter((f) => !isUnit(f))) {
|
for (const h of Object.keys(files).filter((f) => !isUnit(f))) {
|
||||||
for (const r of this.store.all('SELECT unit FROM unit_deps WHERE header = ?', h)) {
|
const before = this.repo.show(head, h);
|
||||||
if (!known.has(r.unit) && !stale.some((u) => u.file === r.unit)) stale.push({ file: r.unit });
|
if (before === files[h]) continue;
|
||||||
}
|
const deps = this.store.all('SELECT unit FROM unit_deps WHERE header = ?', h).map((r) => r.unit);
|
||||||
|
const narrowed = before === null ? null : unitsMentioning(this.repo, deps, changedNames(before, files[h]), files);
|
||||||
|
const pick = narrowed && narrowed.length <= 300 ? narrowed : deps;
|
||||||
|
if (pick !== deps) this.log.info(`batch ${batch.id}: ${h} changed; compiling ${pick.length} of its ${deps.length} includers (the ones that mention what changed)`);
|
||||||
|
for (const u of pick) if (!known.has(u) && !stale.some((x) => x.file === u)) stale.push({ file: u });
|
||||||
}
|
}
|
||||||
if (!stale.length) return { ...saved, reused: saved.check.units.length, compiled: 0 };
|
if (!stale.length) return { ...saved, reused: saved.check.units.length, compiled: 0 };
|
||||||
const part = await this.check(files, `merge-${batch.id}`, { lint: false, onlyUnits: stale.map((u) => u.file) });
|
const part = await this.check(files, `merge-${batch.id}`, { lint: false, onlyUnits: stale.map((u) => u.file) });
|
||||||
|
|
|
||||||
29
test/affected.test.js
Normal file
29
test/affected.test.js
Normal file
|
|
@ -0,0 +1,29 @@
|
||||||
|
import { test } from 'node:test';
|
||||||
|
import assert from 'node:assert/strict';
|
||||||
|
import { changedNames } from '../src/server/affected.js';
|
||||||
|
|
||||||
|
test('a header change names what it touched, not parameters, paths, strings or hex', () => {
|
||||||
|
const before = [
|
||||||
|
'#include "Engine/Source/Base/Types.h"',
|
||||||
|
'struct Foo {',
|
||||||
|
' int a;',
|
||||||
|
'};',
|
||||||
|
'template <class T> struct Bar { T x; };',
|
||||||
|
'bool GetRect(CRect& rect);',
|
||||||
|
'#define K 1',
|
||||||
|
].join('\n');
|
||||||
|
const after = [
|
||||||
|
'#include "Engine/Source/Base/Other.h"',
|
||||||
|
'struct Foo {',
|
||||||
|
' int a;',
|
||||||
|
' int m_new; // 0x16 the new field',
|
||||||
|
'};',
|
||||||
|
'template <class T> struct Bar { T x; char pad[0x16]; };',
|
||||||
|
'bool GetRect(CRect& rect, int count);',
|
||||||
|
'#define K "d:\\BuildServer\\WoW"',
|
||||||
|
].join('\n');
|
||||||
|
const names = new Set(changedNames(before, after));
|
||||||
|
for (const n of ['Foo', 'm_new', 'Bar', 'GetRect', 'CRect']) assert.ok(names.has(n), `expected ${n}`);
|
||||||
|
for (const n of ['rect', 'count', 'Engine', 'Other', 'x16', 'BuildServer', 'WoW', 'T', 'the']) assert.ok(!names.has(n), `did not expect ${n}`);
|
||||||
|
assert.deepEqual(changedNames('a\r\nb\r\n', 'a\nb\n'), [], 'line endings alone change nothing');
|
||||||
|
});
|
||||||
Loading…
Add table
Add a link
Reference in a new issue