Skip to content

Fix CoreMark benchmark comparison validity - #1

Draft
arthurianresolve wants to merge 1 commit into
suthat:ci-benchmark-noisefrom
arthurianresolve:fix/pr-124
Draft

Fix CoreMark benchmark comparison validity#1
arthurianresolve wants to merge 1 commit into
suthat:ci-benchmark-noisefrom
arthurianresolve:fix/pr-124

Conversation

@arthurianresolve

Copy link
Copy Markdown

Summary

  • Build the PR and main implementations with the same CoreMark harness and workload.
  • Fail when either side cannot produce the pinned fixed-clock workload instead of silently skipping instruction counts.
  • Remove the unsupported widest-range-as-noise classification and the unexplained 1% warning.
  • Describe Callgrind counts and timed-score ranges without overstating what they prove.

Why

This is a focused follow-up to nasa#124. Its baseline currently predates COREMARK_FIXED_CLOCK, so the PR cannot exercise its own two-sided Callgrind comparison. It also treats the widest range from three timed samples as a regression threshold, although that range is not a confidence bound.

Saving the PR harness before checking out main holds the measurement apparatus constant while varying the implementation under test. Both sides must then produce the known 10.000 fixed-clock score before instruction counts are accepted.

Validation

  • Actionlint 1.7.12
  • Workflow YAML parsing
  • Embedded Bash and JavaScript syntax checks
  • cargo fmt --all -- --check
  • cargo check -p spacewasm_std --benches
  • cargo clippy -p spacewasm_std --benches -- -Dwarnings
  • git diff --check
  • PR build and current main rebuilt with the saved harness both produced CoreMark Score: 10.000 with four clock_ms calls

Scope

Two workflow files only; no Rust production code changes.

AI usage disclosure

OpenAI Codex was used to analyze PR nasa#124, implement the changes in .github/workflows/ci.yml and .github/workflows/comment.yml, and run the validation listed above. The resulting diff is submitted for author and maintainer review.

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