Skip to content

Comments

Run benchmarks with --profile profiling#5927

Merged
ibraheemdev merged 1 commit intomainfrom
ibraheem/bench-profiling
Sep 10, 2024
Merged

Run benchmarks with --profile profiling#5927
ibraheemdev merged 1 commit intomainfrom
ibraheem/bench-profiling

Conversation

@ibraheemdev
Copy link
Member

@ibraheemdev ibraheemdev commented Aug 8, 2024

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

ibraheemdev commented Aug 8, 2024

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
@ibraheemdev ibraheemdev force-pushed the ibraheem/bench-profiling branch from 1fc293b to 317306f Compare September 10, 2024 18:05
@codspeed-hq
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

ibraheemdev commented Sep 10, 2024

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

BurntSushi
BurntSushi approved these changes Sep 10, 2024
View reviewed changes
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
@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
Revert "Run benchmarks with --profile profiling (#5927)"
5a3b871 
This reverts commit 4f03d20.
@zanieb zanieb mentioned this pull request Sep 10, 2024
Merged
@ibraheemdev ibraheemdev mentioned this pull request Sep 10, 2024
Merged
zanieb pushed a commit that referenced this pull request Sep 10, 2024
@ibraheemdev
Revert Cargo.lock changes (#7276)
cc3259b 
## Summary

Reverts accidental changes in #5927.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BurntSushi BurntSushi BurntSushi approved these changes

@charliermarsh charliermarsh charliermarsh approved these changes

Assignees

No one assigned

Labels

internal A refactor or improvement that is not user-facing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ibraheemdev @BurntSushi @charliermarsh