build with PGO on x86_64 ubuntu and windows #678
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #678 +/- ##
=======================================
Coverage 93.63% 93.63%
=======================================
Files 99 99
Lines 13913 13913
Branches 25 25
=======================================
Hits 13028 13028
Misses 879 879
Partials 6 6 Continue to review full report in Codecov by Sentry.
|
davidhewitt
force-pushed
the
dh/pgo
branch
3 times, most recently
from
2db5fa0 to
39cd650
Compare
CodSpeed Performance ReportMerging #678 will improve performances by 37.33%Comparing Summary
Benchmarks breakdown
|
davidhewitt
force-pushed
the
dh/pgo
branch
7 times, most recently
from
d378f19 to
a82f934
Compare
adriangb
approved these changes
Jun 21, 2023
| name: pypi_files | ||
| path: dist | ||
|
|
||
| build-pgo: |
There was a problem hiding this comment.
Do we need to do two builds?
There was a problem hiding this comment.
The problems I have been encountering with the merged step:
- (very hard in a merged step) we need to pass the correct PGO data per interpreter build as a rust flag. I think
maturin/maturin-actioncurrently assumes flags are same for all interpreters. - (fixable in a merged step) we need to run
pytest(or other PGO sampling) with the correct interpreter for the build.
I have started with this split approach to verify the end-to-end approach works, and limited just to what I assume is the most commonly used platform. It would be good to merge back into a single step and expand to more OS versions. Think I need upstream support in maturin to achieve that.
davidhewitt
force-pushed
the
dh/pgo
branch
5 times, most recently
from
6dd88d3 to
8ef5de6
Compare
davidhewitt
force-pushed
the
dh/pgo
branch
3 times, most recently
from
5c6a97a to
8af2a71
Compare
|
This is now tidied up and uses |
davidhewitt
force-pushed
the
dh/pgo
branch
2 times, most recently
from
84efead to
467e6d7
Compare
davidhewitt
force-pushed
the
dh/pgo
branch
5 times, most recently
from
c09a25e to
391840b
Compare
davidhewitt
force-pushed
the
dh/pgo
branch
2 times, most recently
from
ccd4017 to
af731d5
Compare
davidhewitt
changed the title
davidhewitt
force-pushed
the
dh/pgo
branch
2 times, most recently
from
e40c472 to
383f497
Compare
|
please review |
|
would be great in future if we can have a |
|
I will open an upstream issue on |
4 tasks
This was referenced Jul 4, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a brief experiment to see what happens if we use the test suite to gather profiling data, to feed back into the build for PGO.
If benchmarks look promising, this branch needs extending to also build the uploaded wheels using PGO in the same way.
Selected Reviewer: @adriangb