feat: restructure benchmark tables to pivot layout grouped by method#84
feat: restructure benchmark tables to pivot layout grouped by method#84
Conversation
There was a problem hiding this comment.
Pull request overview
This PR restructures benchmark markdown output into a pivoted layout grouped by install method, making tables significantly shorter and more readable in PR comments and release notes.
Changes:
- Update
scripts/run.shmarkdown generation to output one table per method with commands as columns. - Update
scripts/format-release.shto emit the same pivoted layout (optionally with baseline deltas).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| scripts/run.sh | Switch markdown results from row-per-command to pivot tables grouped by method. |
| scripts/format-release.sh | Rework release-note performance summary to the same pivot format (and keep delta support). |
| mem="N/A" | ||
| for cmd in "${CMD_LIST[@]}"; do | ||
| raw_file="$RAW_DIR/${fixture}-${tool}-${method}-${cmd}.json" | ||
| if [[ -f "$raw_file" ]]; then | ||
| median=$(extract_median "$raw_file" 2>/dev/null || echo "N/A") | ||
| row="$row ${median}ms |" | ||
| # Use memory from any command (they share the same peak RSS roughly) | ||
| m=$(cat "$RAW_DIR/${fixture}-${tool}-${method}-${cmd}.mem" 2>/dev/null || echo "") | ||
| [[ -n "$m" && "$m" != "N/A" ]] && mem="$m" | ||
| else | ||
| row="$row - |" | ||
| fi | ||
| done | ||
| if [[ "$mem" != "N/A" ]]; then | ||
| row="$row ${mem} MB |" | ||
| else | ||
| row="$row N/A |" | ||
| fi | ||
| echo "$row" |
There was a problem hiding this comment.
The "Peak RSS" value is not actually the peak across commands: mem is set to the last non-N/A .mem file encountered while iterating commands (in sorted command order). If memory differs per command, this will report an arbitrary command’s RSS. Consider tracking the maximum RSS across all commands for the fixture/tool/method (or explicitly label it as a representative command) so the column matches its name.
| # Parse: tool-method-cmd (tool may be multi-part like semantic-release) | ||
| for t in semantic-release release-please changesets ferrflow; do | ||
| if [[ "$rest" == "${t}-"* ]]; then | ||
| after="${rest#"${t}-"}" | ||
| method="${after%%-*}" | ||
| cmd="${after#*-}" | ||
| [[ "$cmd" == "$method" ]] && cmd="" | ||
| SEEN_METHODS[$method]=1 | ||
| [[ -n "$cmd" ]] && SEEN_CMDS[$cmd]=1 | ||
| break |
There was a problem hiding this comment.
Tool name parsing is hard-coded to semantic-release release-please changesets ferrflow. Since the benchmarked tool set is sourced from tools.json earlier (TOOLS=$(jq -r 'keys[]' ...)), adding a new tool will produce results in $RAW_DIR but it will never appear in the markdown tables. Consider driving this parsing/iteration from $TOOLS (and matching longest names first if needed) so markdown output stays in sync with configured tools.
| method=$(echo "$rest" | awk -F- '{ | ||
| # tool is first field, method is second | ||
| for (t in KNOWN_TOOLS) {} | ||
| print $2 | ||
| }') |
There was a problem hiding this comment.
method=$(echo "$rest" | awk ...) appears to be unused (it is overwritten in the subsequent known-tool parsing loop) and the embedded KNOWN_TOOLS reference isn’t defined anywhere. This adds an extra process spawn per raw file and makes the parsing harder to follow; consider removing the awk block and relying solely on the tool-prefix parsing below.
| [[ -n "$cmd" ]] && BENCH_DATA["${fixture}|${tool}|${method}|${cmd}"]="${median}ms${delta}" | ||
| if [[ "$mem" != "N/A" ]]; then | ||
| mem_display="${mem} MB" | ||
| BENCH_MEM["${fixture}|${tool}|${method}"]="${mem} MB" | ||
| fi |
There was a problem hiding this comment.
BENCH_MEM["${fixture}|${tool}|${method}"] is overwritten for each command key, so the final "Peak RSS" value depends on key sort order rather than being the peak across commands. If memory varies by command, the release table will report an arbitrary value. Consider storing the maximum RSS seen for the fixture/tool/method (or renaming the column to reflect what’s actually shown).
Summary
run.shandformat-release.shto use a pivot layoutCloses #76