Stale baselines and file-local statics no longer bounce batches
Before a check counts regressions, the units it would blame are compiled as main has them (once per main commit) and their recorded status is updated: a header change merged later could leave a unit building differently on main than recorded (FrameScript_Sprintf 85.9% vs 86.1%, Spell_C_NeedsCooldownEvent recorded exact, built at 89.7%), and every recompile of the unit was counted as a regression. A file-local static is resolved by object and name (Foo_Scripts.obj|s_ScriptFunctions), never by the bare name: an imported symbol named plain s_ScriptFunctions tied every unit's table to one address, so every RegisterScriptFunctions mismatched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
8e6d8ad1fe
commit
84ec59d626
5 changed files with 99 additions and 2 deletions
|
|
@ -302,6 +302,7 @@ export class Merger {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
await this.verifier.refreshBaselines(check, merged.files, `merge-${batch.id}`);
|
||||||
const targets = new Set(attempts.map((a) => a.symbol_id));
|
const targets = new Set(attempts.map((a) => a.symbol_id));
|
||||||
// The batch a split part came from holds the other functions of these files (still in review,
|
// The batch a split part came from holds the other functions of these files (still in review,
|
||||||
// their source put back as on main); shared changes in the file may change their compiled code.
|
// their source put back as on main); shared changes in the file may change their compiled code.
|
||||||
|
|
|
||||||
|
|
@ -318,6 +318,41 @@ export class Verifier {
|
||||||
return { check, rec: reconcile(check, symbols), changedHeaders: saved.changedHeaders, reused: check.units.length - part.check.units.length, compiled: part.check.units.length };
|
return { check, rec: reconcile(check, symbols), changedHeaders: saved.changedHeaders, reused: check.units.length - part.check.units.length, compiled: part.check.units.length };
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* A recorded status can be older than main: a header change merged later recompiles a unit only
|
||||||
|
* where it mentions what changed (affected.js), so a unit's functions can build differently on
|
||||||
|
* main than recorded. Before a check counts regressions, the units it would blame are compiled
|
||||||
|
* as main has them (once per main commit) and their recorded status is brought up to date, so a
|
||||||
|
* submission is compared with what main actually builds.
|
||||||
|
*/
|
||||||
|
async refreshBaselines(check, files, jobName) {
|
||||||
|
const found = this.regressions(check, files);
|
||||||
|
if (!found.length) return 0;
|
||||||
|
const head = this.repo.head();
|
||||||
|
this.baselineAt ??= new Map();
|
||||||
|
const units = [...new Set(found.map((r) => r.file))].filter((u) => isUnit(u) && this.baselineAt.get(u) !== head);
|
||||||
|
if (!units.length) return 0;
|
||||||
|
const { check: main } = await this.check({}, `baseline-${jobName}`, { onlyUnits: units, lint: false });
|
||||||
|
if (this.repo.head() !== head) return 0; // main moved meanwhile: the next check measures again
|
||||||
|
const compiled = new Set(main.units.filter((u) => u.compiled).map((u) => u.file));
|
||||||
|
const ids = new Set(found.map((r) => r.id));
|
||||||
|
let changed = 0;
|
||||||
|
for (const [id, { f, unit }] of bestResults(main)) {
|
||||||
|
if (!ids.has(id) || !compiled.has(unit)) continue;
|
||||||
|
const old = this.store.get('SELECT status, score, code_hash FROM func_status WHERE symbol_id = ?', id);
|
||||||
|
if (!old || (old.status === f.status && Math.abs(old.score - f.score) <= 1e-4 && old.code_hash === (f.codeHash ?? old.code_hash))) continue;
|
||||||
|
this.store.run(
|
||||||
|
'UPDATE func_status SET file = ?, status = ?, score = ?, code_hash = COALESCE(?, code_hash), commit_hash = ?, updated_at = ? WHERE symbol_id = ?',
|
||||||
|
unit, f.status, f.score, f.codeHash ?? null, head, now(), id,
|
||||||
|
);
|
||||||
|
this.store.event('baseline_refreshed', null, { id, was: old.status, wasScore: old.score, now: f.status, score: f.score, commit: head });
|
||||||
|
this.log.info(`baseline of ${id} was stale (${old.status} ${(old.score * 100).toFixed(1)}%); main builds it as ${f.status} ${(f.score * 100).toFixed(1)}%`);
|
||||||
|
changed++;
|
||||||
|
}
|
||||||
|
for (const u of compiled) this.baselineAt.set(u, head);
|
||||||
|
return changed;
|
||||||
|
}
|
||||||
|
|
||||||
/** Functions that match on main but would no longer match with this change. */
|
/** Functions that match on main but would no longer match with this change. */
|
||||||
regressions(check, files = {}) {
|
regressions(check, files = {}) {
|
||||||
const results = new Map([...bestResults(check)].map(([id, b]) => [id, b.f]));
|
const results = new Map([...bestResults(check)].map(([id, b]) => [id, b.f]));
|
||||||
|
|
@ -467,6 +502,7 @@ export class Verifier {
|
||||||
// Verify fast: the submitted units (enough to judge and route every submitted function). The
|
// Verify fast: the submitted units (enough to judge and route every submitted function). The
|
||||||
// units that only include a changed header are compiled when the batch merges.
|
// units that only include a changed header are compiled when the batch merges.
|
||||||
const { check, rec, changedHeaders } = await this.check(merged.files, `batch-${batch.id}`, { dependents: false, targets: attempts.map((a) => a.symbol_id) });
|
const { check, rec, changedHeaders } = await this.check(merged.files, `batch-${batch.id}`, { dependents: false, targets: attempts.map((a) => a.symbol_id) });
|
||||||
|
await this.refreshBaselines(check, merged.files, `batch-${batch.id}`);
|
||||||
const targets = new Set(attempts.map((a) => a.symbol_id));
|
const targets = new Set(attempts.map((a) => a.symbol_id));
|
||||||
const outside = this.changedOutside(check, merged.files, targets, batch.user_id);
|
const outside = this.changedOutside(check, merged.files, targets, batch.user_id);
|
||||||
let failed = 0;
|
let failed = 0;
|
||||||
|
|
|
||||||
|
|
@ -140,8 +140,18 @@ export function bindAnnotations({ listing, file, annotations, nameIndex = new Ma
|
||||||
export function compareObject({ obj, listing, file, annotations, symbols, nameIndex = new Map(), pe }) {
|
export function compareObject({ obj, listing, file, annotations, symbols, nameIndex = new Map(), pe }) {
|
||||||
const objFns = new Map(obj.functions().map((f) => [f.name, f]));
|
const objFns = new Map(obj.functions().map((f) => [f.name, f]));
|
||||||
const sizes = new Map([...objFns].map(([n, f]) => [n, f.bytes.length]));
|
const sizes = new Map([...objFns].map(([n, f]) => [n, f.bytes.length]));
|
||||||
const { byAnn, bindings, errors } = bindAnnotations({ listing, file, annotations, nameIndex, symbols, sizes });
|
const found = bindAnnotations({ listing, file, annotations, nameIndex, symbols, sizes });
|
||||||
|
const { byAnn, errors } = found;
|
||||||
|
// A file-local static (s_ScriptFunctions, a function's static local) has the same name in every
|
||||||
|
// unit that defines one: it is known by its object and name (Foo_Scripts.obj|s_ScriptFunctions),
|
||||||
|
// never by the name alone, which would tie every unit's table to one address.
|
||||||
|
const unitObj = file.replace(/\.[^./\\]+$/, '') + '.obj';
|
||||||
|
const local = new Set(obj.dataSymbols().filter((s) => s.storageClass === 3 && !obj.literal(s)).map((s) => s.name));
|
||||||
|
const qualify = (name) => (local.has(name) ? `${unitObj}|${name}` : name);
|
||||||
|
const bindings = new Map([...found.bindings].map(([sym, id]) => [qualify(sym), id]));
|
||||||
const resolve = (name) => bindings.get(name) ?? nameIndex.get(name) ?? null;
|
const resolve = (name) => bindings.get(name) ?? nameIndex.get(name) ?? null;
|
||||||
|
const withLocalNames = (fn) =>
|
||||||
|
fn.relocs.some((r) => local.has(r.symbol?.name)) ? { ...fn, relocs: fn.relocs.map((r) => (local.has(r.symbol?.name) ? { ...r, symbol: { ...r.symbol, name: qualify(r.symbol.name) } } : r)) } : fn;
|
||||||
const procsBySymbol = new Map(listing.procs.map((p) => [p.symbol, p]));
|
const procsBySymbol = new Map(listing.procs.map((p) => [p.symbol, p]));
|
||||||
|
|
||||||
const functions = [];
|
const functions = [];
|
||||||
|
|
@ -162,7 +172,7 @@ export function compareObject({ obj, listing, file, annotations, symbols, nameIn
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
const proc = procsBySymbol.get(symbol);
|
const proc = procsBySymbol.get(symbol);
|
||||||
const r = compareFunction({ fn, addr: sym.addr, origSize: sym.size, pe, resolve, symbols, proc, literalOf: (s) => obj.literal(s) });
|
const r = compareFunction({ fn: withLocalNames(fn), addr: sym.addr, origSize: sym.size, pe, resolve, symbols, proc, literalOf: (s) => obj.literal(s) });
|
||||||
inferred.push(...r.inferred);
|
inferred.push(...r.inferred);
|
||||||
delete r.inferred;
|
delete r.inferred;
|
||||||
functions.push({ ...base, addr: sym.addr, ...r, asm: procText(proc) });
|
functions.push({ ...base, addr: sym.addr, ...r, asm: procText(proc) });
|
||||||
|
|
|
||||||
|
|
@ -127,3 +127,20 @@ test('lcsRatio', () => {
|
||||||
assert.equal(lcsRatio(Buffer.from('abcd'), Buffer.from('abxd')), 0.75);
|
assert.equal(lcsRatio(Buffer.from('abcd'), Buffer.from('abxd')), 0.75);
|
||||||
assert.equal(lcsRatio(Buffer.alloc(0), Buffer.alloc(0)), 1);
|
assert.equal(lcsRatio(Buffer.alloc(0), Buffer.alloc(0)), 1);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// Every unit's s_ScriptFunctions is its own table: a file-local static is known by object and name.
|
||||||
|
test('a file-local static resolves by its unit, never by the bare name', () => {
|
||||||
|
const f = fixture();
|
||||||
|
const { annotations } = parseAnnotations(f.source, FILE);
|
||||||
|
const obj = new CoffObject(f.obj);
|
||||||
|
const real = obj.dataSymbols.bind(obj);
|
||||||
|
obj.dataSymbols = () => [...real(), { name: '?g_val@@3HA', section: 2, storageClass: 3 }];
|
||||||
|
const syms = symbols([{ id: 'gl_09999', kind: 'global', addr: 0x405000, size: 4, decorated: '?g_val@@3HA' }]);
|
||||||
|
const compare = (nameIndex) =>
|
||||||
|
compareObject({ obj, listing: parseListing(f.listing), file: FILE, annotations, symbols: syms, nameIndex, pe: new PEImage(f.pe) }).functions[0];
|
||||||
|
// Another unit's static of the same name (at another address) does not make this one wrong.
|
||||||
|
const other = compare(new Map([['?Callee@@YAXXZ', 'fn_00002'], ['?g_val@@3HA', 'gl_09999']]));
|
||||||
|
assert.notEqual(other.status, STATUS.MISMATCH);
|
||||||
|
const own = compare(new Map([['?Callee@@YAXXZ', 'fn_00002'], ['?g_val@@3HA', 'gl_09999'], ['src/player.obj|?g_val@@3HA', 'gl_00001']]));
|
||||||
|
assert.equal(own.status, STATUS.EXACT);
|
||||||
|
});
|
||||||
|
|
|
||||||
|
|
@ -25,3 +25,36 @@ test('header-defined copies are not regressions while the header is unchanged',
|
||||||
const files = { 'Engine/A.cpp': '', 'Engine/H/stpl.h': header, 'Engine/H/other.h': '' };
|
const files = { 'Engine/A.cpp': '', 'Engine/H/stpl.h': header, 'Engine/H/other.h': '' };
|
||||||
assert.deepEqual(v.withoutHeaderCopies(found, results, files).map((r) => r.id), ['fn_00001', 'fn_00003', 'fn_00004']);
|
assert.deepEqual(v.withoutHeaderCopies(found, results, files).map((r) => r.id), ['fn_00001', 'fn_00003', 'fn_00004']);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// A recorded status older than main (a header change merged since): main is compiled once and the
|
||||||
|
// record brought up to date, so the submission is compared with what main builds.
|
||||||
|
test('a stale baseline is measured again on main before it counts as a regression', async () => {
|
||||||
|
const rows = new Map([['fn_02237', { status: 'mismatch', score: 0.8589, code_hash: 'old', confirmed: 1 }]]);
|
||||||
|
const events = [];
|
||||||
|
const v = Object.create(Verifier.prototype);
|
||||||
|
v.log = { info() {} };
|
||||||
|
v.repo = { head: () => 'c0ffee' };
|
||||||
|
v.store = {
|
||||||
|
get: (_, id) => rows.get(id),
|
||||||
|
run: (_, file, status, score, hash, commit, at, id) => rows.set(id, { ...rows.get(id), file, status, score, code_hash: hash }),
|
||||||
|
event: (...e) => events.push(e),
|
||||||
|
};
|
||||||
|
const fn = (score, codeHash) => ({ units: [{ file: 'Engine/FrameScript.cpp', compiled: true, functions: [{ id: 'fn_02237', status: 'mismatch', score, codeHash }] }] });
|
||||||
|
v.regressions = (check) => {
|
||||||
|
const now_ = check.units[0].functions[0];
|
||||||
|
const row = rows.get('fn_02237');
|
||||||
|
return Math.abs(row.score - now_.score) > 1e-4 ? [{ id: 'fn_02237', file: 'Engine/FrameScript.cpp' }] : [];
|
||||||
|
};
|
||||||
|
let compiles = 0;
|
||||||
|
v.check = async () => (compiles++, { check: fn(0.8612, 'main') });
|
||||||
|
const submission = fn(0.8612, 'main');
|
||||||
|
assert.equal(await v.refreshBaselines(submission, {}, 'batch-1'), 1);
|
||||||
|
assert.equal(rows.get('fn_02237').score, 0.8612);
|
||||||
|
assert.equal(rows.get('fn_02237').confirmed, 1, 'the confirmation stays');
|
||||||
|
assert.deepEqual(v.regressions(submission), [], 'no regression against what main builds');
|
||||||
|
// A real regression is measured on main once per commit, not on every batch.
|
||||||
|
assert.equal(await v.refreshBaselines(fn(0.5, 'worse'), {}, 'batch-2'), 0);
|
||||||
|
assert.equal(await v.refreshBaselines(fn(0.5, 'worse'), {}, 'batch-3'), 0);
|
||||||
|
assert.equal(compiles, 1);
|
||||||
|
assert.equal(events.length, 1);
|
||||||
|
});
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue