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

Run benchmarks with --profile profiling #5927

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Conversation

ibraheemdev
Copy link
Member

Summary

The CodSpeed flamegraphs are currently useless after #5745.

@ibraheemdev ibraheemdev added the internal A refactor or improvement that is not user-facing label Aug 8, 2024
@ibraheemdev
Copy link
Member Author

Looks like cargo-codspeed doesn't support this flag.

@ibraheemdev ibraheemdev marked this pull request as draft August 8, 2024 20:07
@ibraheemdev ibraheemdev force-pushed the ibraheem/bench-profiling branch 2 times, most recently from 582a0bf to 1fc293b Compare September 10, 2024 18:01
Copy link

codspeed-hq bot commented Sep 10, 2024

CodSpeed Performance Report

Merging #5927 will degrade performances by 13.79%

Comparing ibraheem/bench-profiling (317306f) with main (cfa9299)

Summary

❌ 4 regressions
✅ 10 untouched benchmarks

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

Benchmarks breakdown

Benchmark main ibraheem/bench-profiling Change
build_platform_tags[burntsushi-archlinux] 1.1 ms 1.3 ms -11.97%
wheelname_parsing[flyte-long-compatible] 8.9 µs 10 µs -10.95%
wheelname_parsing[flyte-short-compatible] 5.5 µs 6.4 µs -13.79%
wheelname_parsing[flyte-short-incompatible] 5.5 µs 6.4 µs -13.59%

@ibraheemdev ibraheemdev marked this pull request as ready for review September 10, 2024 18:17
@ibraheemdev
Copy link
Member Author

The regressions are because the profiling profile does not enable LTO, which seems acceptable unless we want to add another profile?

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

I think it makes sense to merge with the regressions since we are changing how we measure perf with this PR.

@ibraheemdev ibraheemdev merged commit 4f03d20 into main Sep 10, 2024
57 of 58 checks passed
@ibraheemdev ibraheemdev deleted the ibraheem/bench-profiling branch September 10, 2024 18:25
zanieb added a commit that referenced this pull request Sep 10, 2024
@zanieb zanieb mentioned this pull request Sep 10, 2024
zanieb pushed a commit that referenced this pull request Sep 10, 2024
## Summary

Reverts accidental changes in #5927.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal A refactor or improvement that is not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants