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

Implement guardrails for Fibonacci benchmark utilities #995

Merged
merged 4 commits into from
Dec 21, 2023
Merged

Conversation

arthurpaulino
Copy link
Contributor

@arthurpaulino arthurpaulino commented Dec 21, 2023

This PR implements an automatic computation of the linear and angular coefficients for the iteration count formula and uses that to assert that the hardcoded constants are correct. These assertions run in the beginning of the benchmark routine.

It also removes examples/fibonacci.rs as its main purpose was to validate the constants that we hardcoded. The function to extract the Fibonacci result was factored out to benches/common/fib.rs and is used for the tests mentioned above.

Closes #990

@arthurpaulino arthurpaulino requested review from a team as code owners December 21, 2023 14:49
@arthurpaulino arthurpaulino force-pushed the std-fib branch 2 times, most recently from 2d5fdd7 to cbb9984 Compare December 21, 2023 16:04
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

I really don't want to make fibonacci utilities part of the public API of Lurk.

If this was just for benches, a better approach here is to open another crate in our workspace (which already has several) , make it non-publishable, and make this crate a dev-dependency. Unfortunately, this would not work for examples.

One approach we could envision is to have an unpublished application crate in the workspace that contains essentially the code of the example (and example is morally a public external module anyway), and that crate could have a benchmark. This would work because both bench and example are supposed to only call into the lurk public functions, which is something they can do from an external crate.

This will require some CI support (/cc @samuelburnham for visibility) and has no downside I can see besides being quite a bit of work: perhaps not our first priority atm, to save a few lines of duplication.

Copy link
Contributor Author

@arthurpaulino arthurpaulino left a comment

Choose a reason for hiding this comment

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

Some details I'm not very happy about the latest commit.
Also, it needs the changes so CI doesn't break, since this moves benches/fibonacci.rs to lurk-fibonacci/benches/throughput.rs

@samuelburnham I need your help on this one

lurk-fibonacci/Cargo.toml Outdated Show resolved Hide resolved
lurk-fibonacci/benches/throughput.rs Outdated Show resolved Hide resolved
lurk-fibonacci/benches/throughput.rs Outdated Show resolved Hide resolved
@huitseeker
Copy link
Contributor

Some details I'm not very happy about the latest commit.

Right so in the grand scheme of things the question is: was this worth it? If we had a grand benchmark such that say the Lurk function is hard to maintain in two places, maybe. In our case, maybe the status quo is enough?

@arthurpaulino
Copy link
Contributor Author

maybe the status quo is enough?

  • The status quo doesn't have the tests test_coeffs and test_fib_io_matches which materialize @porcuquine's request
  • The status quo also doesn't centralize the common functions

For the sake of correctness and simplicity, I'd rather revert the latest commit and have src/fib.rs. At least until we migrate away from it for throughput benchmarks

@arthurpaulino arthurpaulino force-pushed the std-fib branch 2 times, most recently from 80d3baa to bb6d6e3 Compare December 21, 2023 22:28
@arthurpaulino arthurpaulino changed the title Centralize Fibonacci utility functions Implement guardrails for Fibonacci benchmark utilities Dec 21, 2023
samuelburnham
samuelburnham previously approved these changes Dec 21, 2023
benches/common/fib.rs Show resolved Hide resolved
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Excellent, thank you so much!

@arthurpaulino arthurpaulino added this pull request to the merge queue Dec 21, 2023
github-actions bot pushed a commit that referenced this pull request Dec 21, 2023
Merged via the queue into main with commit 6af8086 Dec 21, 2023
12 checks passed
@arthurpaulino arthurpaulino deleted the std-fib branch December 21, 2023 23:15
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.

Fix fibonacci iteration count formula
4 participants