Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase the benchmark warmup time to avoid warmup artifact #355

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JacobEvelyn
Copy link
Member

This commit increases the warmup time used in our benchmark so that two benchmarked gems with similar code (e.g. local MemoWise vs. the version pulled from GitHub) will not have their benchmark results depend on whichever happens to run second (which the Ruby interpreter has had more time to optimize). This issue was not seen in CI but was reported by one user running benchmarks locally.

Before merging:

  • Copy the table printed at the end of the latest benchmark results into the README.md and update this PR
  • If this change merits an update to CHANGELOG.md, add an entry following Keep a Changelog guidelines with semantic versioning

This commit increases the warmup time used in our benchmark so that two
benchmarked gems with similar code (e.g. local `MemoWise` vs. the
version pulled from GitHub) will not have their benchmark results depend
on whichever happens to run second (which the Ruby interpreter has had
more time to optimize). This issue was not seen in CI but was reported
by one user running benchmarks locally.
Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (ef52a1a) to head (a43489d).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #355   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          190       190           
  Branches        90        90           
=========================================
  Hits           190       190           

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

@tagliala
Copy link
Contributor

tagliala commented Sep 26, 2024

Does not look to help at all on GitHub actions

2.7

https://github.com/panorama-ed/memo_wise/actions/runs/11040286867/job/30667918311?pr=355#step:11:359

Method arguments memo_wise-github-main (1.10.0)
() (none) 1.00x
(a) 1.00x
(a, b) 1.03x
(a:) 1.00x
(a:, b:) 1.02x
(a, b:) 1.00x
(a, *args) 1.01x
(a:, **kwargs) 1.00x
(a, *args, b:, **kwargs) 1.00x

3.3

https://github.com/panorama-ed/memo_wise/actions/runs/11040286867/job/30667919858?pr=355#step:11:362

Method arguments memo_wise-github-main (1.10.0)
() (none) 1.00x
(a) 1.06x
(a, b) 1.00x
(a:) 0.93x
(a:, b:) 0.99x
(a, b:) 1.00x
(a, *args) 0.97x
(a:, **kwargs) 1.00x
(a, *args, b:, **kwargs) 0.98x

A previous run without changes to production code lead these results for 3.3:

https://github.com/panorama-ed/memo_wise/actions/runs/10908331323/job/30274052702#step:11:253

Method arguments memo_wise-github-main (1.10.0)
() (none) 1.00x
(a) 1.06x
(a, b) 1.00x
(a:) 0.93x
(a:, b:) 0.99x
(a, b:) 1.00x
(a, *args) 0.97x
(a:, **kwargs) 1.00x
(a, *args, b:, **kwargs) 0.98x

@pboling
Copy link
Contributor

pboling commented Oct 1, 2024

The fact that it is not helped at all on GHA may be due to resource constraints. The memory & CPU limitations will reduce the amount of optimizations that can be front-loaded.

I don't think that is avoidable except to run one benchmark per action, and collate the results after... :(

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