Overhaul build system rules related to profiling #104523
Labels
3.13
bugs and security fixes
build
The build process and cross-build
type-feature
A feature request or enhancement
Comments
indygreg
added a commit
to indygreg/cpython
that referenced
this issue
May 16, 2023
Various rules were only ever invoked once and had minimal bodies. I don't see a benefit to the indirection. So this commit inlines rules to simplify the PGO logic. skip news
indygreg
added a commit
to indygreg/cpython
that referenced
this issue
May 16, 2023
Various rules were only ever invoked once and had minimal bodies. I don't see a benefit to the indirection. So this commit inlines rules to simplify the PGO logic. skip news (trivial, non-user-visible change)
indygreg
added a commit
to indygreg/cpython
that referenced
this issue
May 16, 2023
This commit overhauls the make-based build system's rules for building optimized binaries. Along the way it fixes a myriad of bugs and shortcomings with the prior approach. The old way of producing optimized binaries had various limitations: * `make [all]` would do work when PGO was enabled because the phony `profile-opt` rule was non-empty. This prevented no-op PGO builds from working at all. This meant workflows like `make; make install` either incurred extra work or failed due to race conditions. * Same thing for BOLT, as its `bolt-opt` rule was also non-empty and always ran during `make [all]`. * BOLT could not be run multiple times without a full rebuild because `llvm-bolt` can't instrument binaries that have already received BOLT optimizations. * It was difficult to run BOLT on its own because of how various make targets and their dependencies were structured. * I found the old way that configure and make communicated the default targets to be confusing and hard to understand. There are essentially 2 major changes going on in this commit: 1. A rework of the high-level make targets for performing a build and how they are defined. 2. A rework of all the make logic related to profile-based optimization (read: PGO and BOLT). Build Target Rework =================== Before, we essentially had `build_all`, `profile-opt`, `bolt-opt` and `build_wasm` as our 3 targets for performing a build. `all` would alias to one of these, as appropriate. And there was another definition for which _simple_ make target to evaluate for non-optimized builds. This was likely `build_all` or `all`. In the rework, we introduce 2 new high-level targets: * `build-plain` - Perform a build without optimizations. * `build-optimized` - Perform a build with optimizations. `build-plain` is aliased to `build_all` in all configurations except WASM, where it is `build_wasm`. `build-optimized` by default is aliased to a target that prints an error message when optimizations aren't enabled. If PGO or BOLT are enabled, it is aliased to their respective target. `build-optimized` is the logical successor to `profile-opt`. I felt it best to delete `profile-opt` completely, as the new `build-*` high-level targets feel more friendly to use. But if people lament its loss, we can add a `profile-opt: build-optimized` to achieve almost the same result. Profiled-Based Optimization Rework ================================== Most of the make logic related to profile-based optimization (read: PGO and BOLT) has been touched in this change. A major issue with the old way of doing things was we used phony, always-executed make rules. This is a bad practice in make because it undermines no-op builds. Another issue is that the separation between the rules and what order they ran in wasn't always clear. Both PGO and BOLT consist of the same 4 phase solution: instrument, run, analyze, and apply. However, these steps weren't clearly expressed in the make logic. This is especially true for BOLT, which only had 1 make rule. Another issue with BOLT is that it was really easy to get things into a bad state. e.g. if you applied BOLT to `pythonX.Y` you could not run BOLT again unless you rebuilt `pythonX.Y` from source. In the new world, we have separate `profile-<tool>-<stage>-stamp` rules defining the 4 distinct `instrument`, `run`, `analyze`, and `apply` stages for both PGO and BOLT. Each of these stages is tracked by a _stamp_ semaphore file so progress can be captured. This should all be pretty straightforward. There is some minimal complexity here to handle BOLT's optional dependency on PGO, as BOLT either depends on `build_all` or `profile-pgo-apply-stamp`. As part of the refactor to BOLT we also preserve the original input binary before BOLT is applied. This original file is restored if BOLT runs again. This greatly simplifies repeated BOLT invocations, as make doesn't perform needless work. However, this is all best effort, as it is possible for some make target evaluations to still get things in a bad state. Other Remarks ============= This change effectively reverts pythongh-103574. The readelf based mechanism inserted by that change was effectively working around shortcomings in the make DAG. This change addresses those shortcomings so the readelf integration is no longer required. If this change perturbs any bugs, they are likely around cleaning behavior. The cleaning rules are a bit complicated and not clearly documented. And I'm unsure which targets CPython developers often iterate on. It is highly possible that state cleanup of PGO and/or BOLT files isn't as robust as it needs to be. I explicitly deleted some calls to PGO cleanup because those calls prevented no-op `make [all]` from working. It is certainly possible something somewhere (release automation?) relied on these files being deleted when they no longer are. We still have targets to purge profile files and it should be trivial to add these to appropriate make rules. What I'm trying to say is there are likely subtle workflow regressions with this refactor. But in my mind it is 3 steps forward 1 step back. It should be pretty straightforward to fix any regressions once people complain.
erlend-aasland
pushed a commit
that referenced
this issue
May 16, 2023
Inline profiling rules where the existing indirection was unneeded.
carljm
added a commit
to carljm/cpython
that referenced
this issue
May 16, 2023
* main: pythonGH-104510: Fix refleaks in `_io` base types (python#104516) pythongh-104539: Fix indentation error in logging.config.rst (python#104545) pythongh-104050: Don't star-import 'types' in Argument Clinic (python#104543) pythongh-104050: Add basic typing to CConverter in clinic.py (python#104538) pythongh-64595: Fix write file logic in Argument Clinic (python#104507) pythongh-104523: Inline minimal PGO rules (python#104524) pythongh-103861: Fix Zip64 extensions not being properly applied in some cases (python#103863) pythongh-69152: add method get_proxy_response_headers to HTTPConnection class (python#104248) pythongh-103763: Implement PEP 695 (python#103764) pythongh-104461: Run tkinter test_configure_screen on X11 only (pythonGH-104462) pythongh-104469: Convert _testcapi/watchers.c to use Argument Clinic (python#104503) pythongh-104482: Fix error handling bugs in ast.c (python#104483) pythongh-104341: Adjust tstate_must_exit() to Respect Interpreter Finalization (pythongh-104437) pythonGH-102613: Fix recursion error from `pathlib.Path.glob()` (pythonGH-104373)
indygreg
added a commit
to indygreg/cpython
that referenced
this issue
May 17, 2023
Before we had a separate rule for each source/object file. We can use `$(foreach $(eval $(call ...)))` to dynamically derive rules to avoid the repetition.
indygreg
added a commit
to indygreg/cpython
that referenced
this issue
May 18, 2023
Before we had a separate rule for each source/object file. We can use `$(foreach $(eval $(call ...)))` to dynamically derive rules to avoid the repetition. skip news (behavior preserving build system change)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Feature or enhancement
I have a stack of patches in #101093 to overhaul build system rules related to PGO and BOLT. This issue tracks the merging of them.
Roughly:
I'm scope bloating myself to fix various bugs in PGO/BOLT as part of this refactor. For example, the build system today has a rather crude method of determining if a binary is PGO/BOLT instrumented or needs optimization. This set of changes will fix that so that proper dependencies in the make DAG capture the state of things.
Read the commit messages for the commits in #101093 to get a preview of what's to come.
Linked PRs
The text was updated successfully, but these errors were encountered: