Always build PGO using RUSTFLAGS
#739
Conversation
There are many ways to pass flags to rustc: * `cargo rustc`, which only affects your crate and not its dependencies. * `RUSTFLAGS` environment variable, which affects dependencies as well. See https://stackoverflow.com/a/38040431 In this case I think `RUSTFLAGS` is a better choice.
.github/workflows/ci.yml
Outdated
|
|
||
| - name: merge pgo data | ||
| run: ${{ env.LLVM_PROFDATA }} merge -o ${{ github.workspace }}/merged.profdata ${{ github.workspace }}/profdata | ||
| run: rustup run stable llvm-profdata merge -o ${{ github.workspace }}/merged.profdata ${{ github.workspace }}/profdata |
There was a problem hiding this comment.
Odd, worked for me locally on a Linux server.
There was a problem hiding this comment.
Yeah llvm-profdata is in an unusual bin directory (see the way I figure it out just above). The rustup team might have knowledge of why that worked (and the best practices to add it to path).
CodSpeed Performance ReportMerging #739 will not alter performancesComparing Summary
|
There was a problem hiding this comment.
Thanks!
@messense what do you think of adding PGO support directly into either maturin or maturin-action?
I had to split out the build jobs in #678 because I need to pass different profiling data to each python build. In theory I think if maturin was given a command it could run to gather PGO data with each interpreter it should be able to:
- build all the initial wheels for profiling (current build
- for each interpreter in turn
a. install the profiling wheel into the interpreter's environment
b. run the PGO command to gather profiling data
c. build the final optimized wheel
This might add quite a lot of complexity, so it might be better to keep this out of maturin and just add to maturin-action.
The upside would be that it would become much easier for other projects to also use PGO, and also I think an integrated action could reuse Rust build products better than I do with these builds.
I'm in favor of adding PGO support to one of them.
IMO the upside of adding it in |
Change Summary
There are many ways to pass flags to rustc:
cargo rustc, which only affects your crate and not its dependencies.RUSTFLAGSenvironment variable, which affects dependencies as well.See https://stackoverflow.com/a/38040431
In this case I think
RUSTFLAGSis a better choice.Related issue number
Checklist
pydantic-core(except for expected changes)