Skip to content
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 75 additions & 24 deletions scripts/benchmarks/bundle-size/pr-report.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import path from 'node:path'
import { parseArgs as parseNodeArgs } from 'node:util'

const DEFAULT_MARKER = '<!-- bundle-size-benchmark -->'
const TREND_GRAPH_URL = 'https://tiny-graph.florianpellet.com/'
const INT_FORMAT = new Intl.NumberFormat('en-US', {
maximumFractionDigits: 0,
})
Expand Down Expand Up @@ -132,29 +133,62 @@ function formatDelta(current, baseline) {
return `${formatBytes(delta, { signed: true })} (${sign}${PERCENT_FORMAT.format(ratio)})`
}

function sparkline(values) {
if (!values.length) {
function toUnixTimestamp(value) {
if (Number.isFinite(value)) {
return Math.floor(Number(value) / 1000)
}

if (typeof value !== 'string' || !value) {
return undefined
}

const parsed = Date.parse(value)
if (Number.isNaN(parsed)) {
return undefined
}

return Math.floor(parsed / 1000)
}

function resolveHistoryEntryTimestamp(entry) {
return (
toUnixTimestamp(entry?.date) ?? toUnixTimestamp(entry?.commit?.timestamp)
)
}

function resolveCurrentTimestamp(current) {
return (
toUnixTimestamp(current?.measuredAt) ??
toUnixTimestamp(current?.generatedAt)
)
}

function buildTrendGraph(points) {
if (!points.length) {
return 'n/a'
}

const blocks = '▁▂▃▄▅▆▇█'
const min = Math.min(...values)
const max = Math.max(...values)
const coords = []

for (const point of points) {
if (!Number.isFinite(point?.x) || !Number.isFinite(point?.y)) {
continue
}

if (max === min) {
return '▅'.repeat(values.length)
coords.push(point.x, point.y)
}

return values
.map((value) => {
const normalized = (value - min) / (max - min)
const idx = Math.min(
blocks.length - 1,
Math.max(0, Math.round(normalized * (blocks.length - 1))),
)
return blocks[idx]
})
.join('')
if (!coords.length) {
return 'n/a'
}

const params = new URLSearchParams({
coords: coords.join(','),
width: '165',
height: '45',
})
const src = `${TREND_GRAPH_URL}?${params.toString()}`
return `![Historical gzip bytes trend](${src})`
}

function normalizeHistoryEntries(history, benchmarkName) {
Expand All @@ -177,6 +211,12 @@ function buildSeriesByScenario(historyEntries) {
const map = new Map()

for (const entry of historyEntries) {
const timestamp = resolveHistoryEntryTimestamp(entry)

if (!Number.isFinite(timestamp)) {
continue
}

for (const bench of entry?.benches || []) {
if (typeof bench?.name !== 'string' || !Number.isFinite(bench?.value)) {
continue
Expand All @@ -186,7 +226,10 @@ function buildSeriesByScenario(historyEntries) {
map.set(bench.name, [])
}

map.get(bench.name).push(Number(bench.value))
map.get(bench.name).push({
x: timestamp,
y: Number(bench.value),
})
}
}

Expand Down Expand Up @@ -263,6 +306,7 @@ async function main() {

const historyEntries = normalizeHistoryEntries(history, current.benchmarkName)
const seriesByScenario = buildSeriesByScenario(historyEntries)
const currentTimestamp = resolveCurrentTimestamp(current)

const baseline =
baselineCurrent != null
Expand All @@ -274,15 +318,22 @@ async function main() {
for (const metric of current.metrics || []) {
const baselineValue = baseline.benchesByName.get(metric.id)
const historySeries = (seriesByScenario.get(metric.id) || []).slice(
// Reserve one slot for the current metric so the sparkline stays at trendPoints.
// Reserve one slot for the current metric so the trend stays at trendPoints.
-args.trendPoints + 1,
)
const currentPoint = {
x: currentTimestamp,
y: metric.gzipBytes,
}
const lastPoint = historySeries[historySeries.length - 1]

if (
!historySeries.length ||
historySeries[historySeries.length - 1] !== metric.gzipBytes
Number.isFinite(currentPoint.x) &&
(!lastPoint ||
lastPoint.x !== currentPoint.x ||
lastPoint.y !== currentPoint.y)
) {
historySeries.push(metric.gzipBytes)
historySeries.push(currentPoint)
}
Comment on lines 320 to 337
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Only reserve a slot when the current point is actually appended.

Lines 320-337 always pre-trim to trendPoints - 1, but the current point is skipped when it exactly matches the latest historical point. In that case the chart drops to 11 points instead of 12. Check duplication against the full scenario series first, then reserve a slot only when currentPoint will be added.

🐛 Suggested fix
-    const historySeries = (seriesByScenario.get(metric.id) || []).slice(
-      // Reserve one slot for the current metric so the trend stays at trendPoints.
-      -args.trendPoints + 1,
-    )
+    const scenarioSeries = seriesByScenario.get(metric.id) || []
     const currentPoint = {
       x: currentTimestamp,
       y: metric.gzipBytes,
     }
-    const lastPoint = historySeries[historySeries.length - 1]
+    const lastPoint = scenarioSeries[scenarioSeries.length - 1]
+    const shouldAppendCurrent =
+      Number.isFinite(currentPoint.x) &&
+      Number.isFinite(currentPoint.y) &&
+      (!lastPoint ||
+        lastPoint.x !== currentPoint.x ||
+        lastPoint.y !== currentPoint.y)
+    const historySeries = scenarioSeries.slice(
+      -(args.trendPoints - (shouldAppendCurrent ? 1 : 0)),
+    )
 
-    if (
-      Number.isFinite(currentPoint.x) &&
-      (!lastPoint ||
-        lastPoint.x !== currentPoint.x ||
-        lastPoint.y !== currentPoint.y)
-    ) {
+    if (shouldAppendCurrent) {
       historySeries.push(currentPoint)
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/benchmarks/bundle-size/pr-report.mjs` around lines 320 - 337, The
code trims history before checking whether to append the current point, which
causes the series to shrink when the current point is a duplicate; fix by first
getting the full series (const fullSeries = seriesByScenario.get(metric.id) ||
[]), compute currentPoint (using currentTimestamp and metric.gzipBytes) and
determine duplication against fullSeries's last point (check
Number.isFinite(currentPoint.x) and compare x/y); then set historySeries =
fullSeries.slice(willAppend ? -args.trendPoints + 1 : -args.trendPoints) and
only push currentPoint when willAppend is true (push to historySeries when not
duplicate). Ensure you reference historySeries, seriesByScenario, currentPoint,
currentTimestamp, metric.gzipBytes, and args.trendPoints.


rows.push({
Expand All @@ -291,7 +342,7 @@ async function main() {
raw: metric.rawBytes,
brotli: metric.brotliBytes,
deltaCell: formatDelta(metric.gzipBytes, baselineValue),
trendCell: sparkline(historySeries.slice(-args.trendPoints)),
trendCell: buildTrendGraph(historySeries.slice(-args.trendPoints)),
})
}

Expand Down Expand Up @@ -321,7 +372,7 @@ async function main() {

lines.push('')
lines.push(
'_Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better._',
'_Trend chart uses historical gzip bytes plotted by measurement date, ending with this PR measurement; lower is better._',
)

const markdown = lines.join('\n') + '\n'
Expand Down
Loading