Skip to content

Ensure fair benchmarks #183

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.

Sign up for GitHub

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

Jump to bottom
Closed
Peter554 wants to merge 12 commits into from
Closed

Ensure fair benchmarks #183

Peter554 wants to merge 12 commits into from

Conversation

@Peter554
Copy link
Collaborator

@Peter554 Peter554 commented Feb 7, 2025

No description provided.

seddonym and others added 12 commits February 6, 2025 13:44
@seddonym
Implement Graph struct
47487dc 
This is the main graph implemented in Rust - but it isn't plumbed into
Python yet.
@seddonym
Make integration test use new API
7d441fb
@seddonym
Use a Rust-backed Graph
04043e5 
This adds a wrapper around the Rust-graph in lib.rs which handles the
interface between Python and Rust. At the same time, it changes the
Python ImportGraph so it uses Rust instead.
@seddonym
Remove unused code
f24d24c
@seddonym
Use faster hash library
392b401 
This is faster, but not cryptographically safe. That shouldn't matter for our
purposes.
@Peter554
Do not set number of rounds for local benchmark
ee84fab 
pytest-benchmark will actually determine a suitable number
of rounds+iterations automatically (typically much more than 3!).
With only 3 rounds the results are quite noisy.
@Peter554
Reimplement import graph
04c32b3
@seddonym
Merge pull request python-grimp#175 from Peter554/peter.rust-graph
cb3e1fa 
Rust graph (Peter)
@seddonym
Cache modules on Python graph
c31771a 
Accessing graph.modules is more expensive than it looks.
@Peter554
Remove contains_module from public API
e4e7b39
@seddonym
Merge pull request python-grimp#181 from Peter554/remove-contains-mod…
f6ce566 
…ule-from-public-api

Remove contains_module from public API
@Peter554
Ensure fair benchmarks
65038f0
@Peter554 Peter554 changed the base branch from master to rust-graph February 7, 2025 13:58
@codspeed-hq
Copy link

codspeed-hq bot commented Feb 7, 2025

CodSpeed Performance Report

Merging #183 will degrade performances by 99.6%

Comparing Peter554:ensure-fair-benchmarks (65038f0) with rust-graph (f6ce566)

Summary

❌ 2 regressions
✅ 14 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
❌ test_graph_contains_module 137.4 µs 34,564.2 µs -99.6%
❌ test_iterate_over_modules_in_graph 3.8 ms 36.7 ms -89.71%

@Peter554
Copy link
Collaborator Author

Peter554 commented Feb 7, 2025

Closing since we merged #184 which covers this

@Peter554 Peter554 closed this Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Peter554 @seddonym