Skip to content

Commit a1583bb

Browse files
committed
Total what the two runs spent per phase
The report says which models changed phase and which got slower, and nothing about what the run as a whole cost. Testing pull request 16696 moved 83 of 19842 models and spent 8% more in the backend, which the report had no way of saying. The page and the comment now carry a table of both runs summed per phase over the models they have in common: | Phase | master | pr/16696 | Change | Change, same phase | | -------- | -------- | -------- | ------ | ------------------ | | frontend | 1:44:57 | 1:44:37 | -0.3% | -0.6% | | backend | 3:12:28 | 3:27:54 | +8.0% | +7.6% | The last column is the same sum over only the models that reached the same phase in both runs. A model that now fails in the backend stops paying for the compilation and the simulation it no longer reaches, so without that column a run that broke models reads as a faster one: here the whole simulation looks 1.9% cheaper, while the models that still simulate spend 0.4% more. `exectime`, the whole run of a model, is the total. Assisted-by: Claude Opus 5 (1M context)
1 parent 1de2afa commit a1583bb

3 files changed

Lines changed: 61 additions & 0 deletions

File tree

README.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -462,6 +462,11 @@ kind of page as the nightly regression reports, next to `00_comment.md`, a
462462
summary to comment on the pull request with. Both are published with the other
463463
reports.
464464

465+
Both also total what the two runs spent per phase on the models they have in
466+
common, next to the same totals over only the models that reached the same
467+
phase in both: a model that fails earlier stops paying for the phases it no
468+
longer reaches, which makes those phases look cheaper than they are.
469+
465470
`--comment` posts that summary on the pull request, replacing the one an earlier
466471
run posted. It posts as whoever the token belongs to: `GITHUB_TOKEN` or
467472
`GH_TOKEN` in the environment, or the account [`gh`](https://cli.github.com) is

pr-report.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@
4040
timeAbs = 10 # Ignore performance regressions for times <10s...
4141

4242
PHASES = [(1,"frontend"),(2,"backend"),(3,"simcode"),(4,"templates"),(5,"compile"),(6,"simulate")]
43+
# The columns the totals are summed over; exectime is the whole run of a model.
44+
TIMES = ["frontend","backend","simcode","templates","compile","simulate","verify","exectime"]
4345

4446
m = re.match(r"^(?:pr[-/])?([0-9]+)$", args.pullrequest.strip())
4547
if not m:
@@ -197,6 +199,25 @@ def changedModels(table1, date1, table2, date2, libnames):
197199
return cursor.fetchall()
198200

199201

202+
def phaseTotals(table1, date1, table2, date2, libnames):
203+
"""What the two runs spent, per phase, on the models they both have.
204+
205+
Twice: over every compared model, and over those that reached the same phase
206+
in both runs. A model that now fails earlier stops paying for the phases it
207+
no longer reaches, which makes those phases look cheaper than they are.
208+
"""
209+
inlibs = ",".join("'%s'" % libname for libname in sorted(libnames))
210+
cols = ["sum(a.%s),sum(b.%s)" % (t, t) for t in TIMES]
211+
cols += ["sum(CASE WHEN a.finalphase=b.finalphase THEN a.%s ELSE 0 END),"
212+
"sum(CASE WHEN a.finalphase=b.finalphase THEN b.%s ELSE 0 END)" % (t, t)
213+
for t in TIMES]
214+
cols += [db.countIf("a.finalphase=b.finalphase")]
215+
query = """SELECT %s FROM %s AS a JOIN %s AS b ON a.libname=b.libname AND a.model=b.model
216+
WHERE a.date=? AND b.date=? AND a.libname IN (%s) AND a.finalphase>=0 AND b.finalphase>=0
217+
""" % (",".join(cols), db.quote(table1), db.quote(table2), inlibs)
218+
return [v or 0 for v in cursor.execute(query, (date1, date2)).fetchone()]
219+
220+
200221
prdate = newestRun(branch, args.date)
201222
if not prdate:
202223
raise SystemExit("No results for %s%s" % (branch, " at or before %d" % args.date if args.date else ""))
@@ -223,8 +244,10 @@ def changedModels(table1, date1, table2, date2, libnames):
223244
groups.setdefault((d1, d2), []).append(libname)
224245

225246
changes = []
247+
totals = [0] * (4*len(TIMES) + 1)
226248
for ((d1, d2), libs) in sorted(groups.items()):
227249
changes += changedModels(baseline, d1, branch, d2, libs)
250+
totals = [t + v for (t, v) in zip(totals, phaseTotals(baseline, d1, branch, d2, libs))]
228251
changes = sorted(changes, key=lambda x: (x[1], x[0]))
229252

230253
# Models one of the runs has and the other does not: a library that grew a model,
@@ -328,6 +351,24 @@ def classify(group, times):
328351
"difference can also come from something merged into %s since the pull request was "
329352
"branched." % (baseline, baseline))
330353

354+
def relative(before, after):
355+
return "%+.1f%%" % (100.0*(after-before)/before) if before else ""
356+
357+
numSamePhase = totals[-1]
358+
totalrows = []
359+
markdowntotals = []
360+
for (i, name) in enumerate(TIMES):
361+
(t1, t2) = totals[2*i:2*i+2]
362+
(s1, s2) = totals[2*len(TIMES)+2*i:2*len(TIMES)+2*i+2]
363+
cells = ["total" if name == "exectime" else name,
364+
friendlyStr(t1), friendlyStr(t2), relative(t1, t2), relative(s1, s2)]
365+
totalrows.append("<tr>%s</tr>" % "".join("<td>%s</td>" % c for c in cells))
366+
markdowntotals.append("| %s |" % " | ".join(cells))
367+
totalnote = ("Time spent on the %d models both runs have. The last column counts only the %d "
368+
"that reached the same phase in both: a model that fails earlier stops paying "
369+
"for the phases it no longer reaches."
370+
% (numCompared, numSamePhase))
371+
331372
reportname = "%s..%s.html" % (dateStr(basedate), dateStr(prdate))
332373
historydir = os.path.join(args.historypath, branch)
333374
os.makedirs(historydir, exist_ok=True)
@@ -347,6 +388,8 @@ def classify(group, times):
347388
("#HOST1#", html.escape(", ".join(sorted(basehosts)))),
348389
("#HOST2#", html.escape(", ".join(sorted(prhosts)))),
349390
("#NUMCOMPARED#", str(numCompared)),
391+
("#TOTALS#", "\n".join(totalrows)),
392+
("#TOTALNOTE#", totalnote),
350393
("#NUMIMPROVE#", str(counts["improved"])),
351394
("#NUMREGRESSION#", str(counts["regression"])),
352395
("#NUMPERFIMPROVE#", str(counts["performance improved"])),
@@ -389,6 +432,10 @@ def classify(group, times):
389432
"%d models compared, **%d improved, %d regressions**, performance %d improved, %d regressions."
390433
% (numCompared, counts["improved"], counts["regression"],
391434
counts["performance improved"], counts["performance regression"]),
435+
"",
436+
"| Phase | `%s` | `%s` | Change | Change, same phase |" % (baseline, branch),
437+
"| --- | --- | --- | --- | --- |"] + markdowntotals + [
438+
"", totalnote,
392439
"", "[Full report](%s)" % reporturl, ""]
393440
if markdownrows:
394441
markdown += ["<details><summary>%d models affected</summary>" % len(markdownrows), "",

pr.html.tpl

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,15 @@ the pull request was branched.</p>
4242
<tr><td>Models only in the baseline run</td><td>#NUMONLYBASELINE#</td></tr>
4343
</table>
4444

45+
<h2>Time</h2>
46+
47+
<p>#TOTALNOTE#</p>
48+
49+
<table>
50+
<tr><th>Phase</th><th>#BASELINE#</th><th>#BRANCH#</th><th>Change</th><th>Change, same phase</th></tr>
51+
#TOTALS#
52+
</table>
53+
4554
<h2>Library Changes</h2>
4655
<table>
4756
<tr><th>Library</th><th>Change</th></tr>

0 commit comments

Comments
 (0)