Skip to content

ci: clarify LLGo dashboard benchmark groups - #15

Merged
zhouguangyuan0718 merged 1 commit into
xgo-dev:mainfrom
zhouguangyuan0718:codex/clear-benchmark-groups
Aug 3, 2026
Merged

ci: clarify LLGo dashboard benchmark groups#15
zhouguangyuan0718 merged 1 commit into
xgo-dev:mainfrom
zhouguangyuan0718:codex/clear-benchmark-groups

Conversation

@zhouguangyuan0718

Copy link
Copy Markdown
Collaborator

Summary

  • display each benchmark name once at the start of its build-mode group
  • indent continuation build modes within the compact sticky label column
  • add a full-width separator between benchmark groups in both matrices
  • preserve accessible benchmark and mode labels on every row
  • keep A/B selected-column outlines visible across group separators

Validation

  • go test ./cmd/bent
  • bash -n ci/llgo-size/report.sh
  • git diff --check
  • browser-tested with the current 41-run Pages history: 9 visible benchmark names for 54 rows in each matrix, 7px group separators, intact A/B highlighting, and no console warnings or errors

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review summary

This is a small, focused UI change that groups the matrix rows by benchmark (name shown once per group, full-width separator before each subsequent benchmark). The implementation is clean and correct:

  • HTML/attribute escaping is handled correctly — the new aria-label at app.js:312 wraps benchmarkName in escapeHtml, which encodes "/</>, preventing attribute breakout in the double-quoted value.
  • The higher-specificity .benchmark-group-start ... > .selected-a/.selected-b rule (style.css:85-86) correctly layers the group separator on top of the existing accent side-borders rather than clobbering them.
  • The README description (README.md:44-46) accurately matches the implemented behavior.

No blocking issues. One optional observation and one inline nit below.

Optional (pre-existing, not introduced here): rankFor (app.js:247, reached per cell via cellHtml) re-derives the full per-(benchmark, measure) ranking — filter + new Set + sort — once for every config row, even though the result is identical across all configs of the same benchmark. This PR's new outer-benchmarkName/inner-config loop shape now makes it natural to compute the ranking once per (column, benchmark) and reuse it across config rows. Not user-perceptible at current data scale; flagging only because the changed loop is the surrounding hot path.

Comment thread ci/llgo-size/site/app.js
configs.forEach(function (config) {
const label = '<th class="matrix-label-cell"><span class="benchmark-name">' + escapeHtml(benchmarkName) + '</span><span class="config-name">' + escapeHtml(compactConfigLabels[config]) + "</span></th>";
configs.forEach(function (config, configIndex) {
const benchmarkLabel = configIndex === 0 ? '<span class="benchmark-name">' + escapeHtml(benchmarkName) + "</span>" : "";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor (DRY): configIndex === 0 is tested three times in this loop (lines 310, 311, 316). Hoisting const isGroupStart = configIndex === 0; at the top of the callback and reusing it would remove the repetition and make the grouping intent self-documenting.

@zhouguangyuan0718
zhouguangyuan0718 merged commit 7c82eac into xgo-dev:main Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant