Skip to content

Weekly benchmarks#1758

Draft
AnshulPatil2005 wants to merge 24 commits into
elalish:masterfrom
AnshulPatil2005:weekly-benchmark
Draft

Weekly benchmarks#1758
AnshulPatil2005 wants to merge 24 commits into
elalish:masterfrom
AnshulPatil2005:weekly-benchmark

Conversation

@AnshulPatil2005

Copy link
Copy Markdown
Contributor

Adds weekly benchmark workflow and reporting structure.

ref-#1738

@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

I am currently working on running the Ember benchmarks in the workflow first. Once that is done, I will move on to adding the other tests.

The structure is similar to the previous benchmark guard CI, with a few changes for the Ember cases.

@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.62%. Comparing base (895bfc2) to head (75ece90).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1758   +/-   ##
=======================================
  Coverage   94.62%   94.62%           
=======================================
  Files          44       44           
  Lines        9441     9441           
=======================================
  Hits         8934     8934           
  Misses        507      507           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pca006132

Copy link
Copy Markdown
Collaborator

did you try it on your fork? say, once a day, to see if it works?

@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

did you try it on your fork? say, once a day, to see if it works?

I tested it manually on my fork and the workflow passed. I also added a temporary schedule on the fork default branch to test the scheduled trigger I set it at a very short time(10 mins), but it did not emit any runs so i increased the time so see if it would work that way.

i checked regarding this and i found on github's docs that scheduled workflows are best-effort and can be delayed or dropped under load, I will leave it enabled for longer and confirm once the scheduled run appears.

but this seems to be very unreliable, what if it dropped the weekly run ?

@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

Update: the scheduled workflow is working on my fork now. I changed the temporary schedule to a 12-hour interval, and it has produced 3 scheduled Ember benchmark runs so far.

@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

I have run the workflow several times on my fork now(using schedule).

The weekly benchmark currently lands on different runners, so the absolute timings vary between runs. I will compile the results from these runs to estimate how much difference this is causing.

Comment thread scripts/run_weekly_benchmarks.py Outdated
Comment thread scripts/run_weekly_benchmarks.py
Comment thread scripts/run_weekly_benchmarks.py Outdated
Comment thread scripts/parse_weekly_benchmarks.py Outdated
Comment thread scripts/parse_weekly_benchmarks.py Outdated
Comment thread scripts/parse_weekly_benchmarks.py Outdated
Comment thread scripts/report_weekly_benchmarks.sh Outdated
@AnshulPatil2005

AnshulPatil2005 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

xlx_processed.xlsx
image

image image

MacOS(named "arm" here) seems to be more noisier due to the OS scheduler moving the benchmark thread to an E-core(efficiency core rather than performance core) for even one repeat, that repeat is 3–4× slower.

@pca006132 @elalish

@elalish

elalish commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Nice plots, though the labels could use a little work. If "arm" is MacOS, maybe just call it that? And what are EPYC####? And what are C###? Are those particular test models?

@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

Nice plots, though the labels could use a little work. If "arm" is MacOS, maybe just call it that? And what are EPYC####? And what are C###? Are those particular test models?

Yes, I should have mentioned the label meanings. This was discussed in the previous meeting, so I missed adding the context here.

EPYC#### refers to the GitHub-hosted Linux runner CPU models that were assigned during CI runs. arm is the macOS 15 ARM runner; I should rename that label to macOS 15 to make it clearer.

I am also avoiding macos-latest for the weekly benchmark because the macOS version can change over time and add extra noise.

And yes, C667 means Ember benchmark case 667.

Let me know any improvements or confusion is there, I will change it accordingly

@pca006132

Copy link
Copy Markdown
Collaborator

@AnshulPatil2005 what about the macos timing anomaly for the largest sphere in perfTest? Any idea about that?

@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

@AnshulPatil2005 what about the macos timing anomaly for the largest sphere in perfTest? Any idea about that?

Root cause is the macOS-15 runners run on Apple Silicon (M1/M2/M3). The OS moves threads from P-cores (~3.5GHz) to E-cores (~2.0GHz) at any point during execution. A 33-second computation gives the scheduler many opportunities to do this so a mid-run migration to E-cores can cost 10–15 seconds.

I think we could try pinning the process to P-cores only if thats possible in macOS

@pca006132

Copy link
Copy Markdown
Collaborator

Interesting. How did you check that?

@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

Interesting. How did you check that?

ok so, i found this
P-cores run at ~4,512 MHz; E-cores run at ~1,050 MHz for background tasks which is a 4.3× frequency ratio

and the Observed worst-case ratio for nTri=512: 4.60ms ÷ 1.10ms = 4.16× which is very similar to that ratio

For nTri=8388608: Observed ratio: 44.16s ÷ 20.17s = 2.19× this doesnt match the 4.3x because The likely reason: a 30+ second computation is long enough that the OS migrates the thread back to a P-core mid-run thus we are noticing less effect for larger size.

But the effect is clear for the smallest sphere

@pca006132

Copy link
Copy Markdown
Collaborator

oh btw, maybe you can convert those STLs to binary STLs, so it will not mess up the line count.

AnshulPatil2005 and others added 5 commits July 11, 2026 19:54
- merge stderr into stdout in run_ember_case so man_bench diagnostic
  output is not silently dropped
- remove Assembly/Triangulation/Simplification/Sorting from
  INDEPENDENT_PHASES; their Timer::Print calls were removed in PR elalish#1745
  and they always produced 0.0; only Intersect12 and Winding03 phases
  are still reported
- update build_ember_summary table to match the 4 remaining phases

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

@pca006132 @elalish Would it make sense to also run the benchmark workflow on release publish events?(version-wise data)

The idea is to reuse the same benchmark runner, but store release results separately in the same benchmark-data branch, for example under releases/<tag>/<run_id>/, while keeping weekly runs under weekly/.

then plot the data on the dashboard
rust has done similar thing in their perf dashboard

@pca006132

Copy link
Copy Markdown
Collaborator

I think we can mark the release in the UI, but the release triggered run should still add to the same dataset. E.g., weekly data but tag when we have releases.

@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

I think we can mark the release in the UI, but the release triggered run should still add to the same dataset. E.g., weekly data but tag when we have releases.

So when a release is published, we trigger the same benchmark workflow and store the result in the same weekly/ dataset, just like a scheduled run, but add release metadata such as release_tag: "v3.5.1".

Then the dashboard can mark that run as a release point in the same timeline.

Am I understanding this correctly?

@pca006132

Copy link
Copy Markdown
Collaborator

Yes, this is what I am thinking.

@AnshulPatil2005

Copy link
Copy Markdown
Contributor Author

Yes, this is what I am thinking.

Makes sense, I'll do that

Comment thread .github/workflows/manifold.yml Outdated
Comment thread .github/workflows/manifold.yml Outdated
Comment thread .github/workflows/manifold.yml Outdated
Comment thread .github/workflows/weekly_benchmark.yml Outdated
Comment thread .github/workflows/weekly_benchmark.yml Outdated
Comment thread dashboard/dashboard.js Outdated
Comment thread dashboard/index.html
Comment thread scripts/parse_weekly_benchmarks.py
Comment thread scripts/parse_weekly_benchmarks.py
Comment thread .gitignore Outdated
Not for merge - revert before this PR lands. Only relaxing the if: gate
so a manual run can validate the folded run: > block and sanitizer arg
passing reviewers asked about, without waiting for a scheduled/release run.
- Move build_sanitizer/test_sanitizer in weekly_benchmark.yml from
  macos-15 to ubuntu-24.04, matching the existing PR-triggered sanitizer
  lane in manifold.yml, since nothing about ASan/UBSan requires macOS
  here. Drops the compiler overrides and macOS-only setup steps.
- Download sanitizer build artifacts into build/ instead of the
  workspace root, so manifold_test's baked-in RPATH to libsamples can
  actually resolve. It was crashing on load with
  "Library not loaded: @rpath/libsamples.3.dylib" because the artifact
  download flattened the build/ directory away.
- Capture the sanitizer build's own cmake configure log and derive
  SANITIZER_SUMMARY_COMPILER from it at runtime instead of hardcoding
  the string, reusing system_metadata.py's existing helpers.
- Trim set -euo pipefail to set -u in the data-publish steps, since
  GitHub Actions' default bash invocation already applies -e/pipefail.
- Remove the duplicate weeklybenchmark-test job from manifold.yml in
  favor of testing weekly_benchmark.yml directly via workflow_dispatch.
…+ simplify perf summary formatting + clean up .gitignore
Comment thread .github/workflows/weekly_benchmark.yml
Comment thread scripts/parse_weekly_benchmarks.py
Comment thread scripts/ci/sanitizer_build.sh Outdated
Comment thread .github/workflows/weekly_benchmark.yml Outdated
if: github.event_name == 'release' || github.ref == format('refs/heads/{0}', github.event.repository.default_branch)
shell: bash
run: |
set -u

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

set -u makes the script error out immediately if it references a variable that was never set. It was added more for future typo avoidance rather than catching a current error.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am personally not particularly worried about this kind of typo, since it will show up in the diff. But well, it is fine to keep it as is. I just don't like the idea of putting set -u everywhere.

@@ -1,6 +1,10 @@
#!/usr/bin/env bash
set -euo pipefail

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

btw normally you can just set it in the shebang.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

wouldnt shebang line just becomes a plain comment to bash and never gets executed as flags ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if you invoke it as ./script.sh then it will invoke the shebang.

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.

3 participants