bpo-28643: Record profile-opt build progress with stamp files#4223
Merged
Conversation
The profile-opt makefile target is expensive to build. Since the makefile does not contain complete dependency information for this target, much extra work can get done if the build is interrupted and re-started. Even running "make" a second time will result in a huge amount of redundant work. As a minimal fix (rather than removing recursive "make" and adding a proper dependency graph), split the profile-opt target into three parts: - build with profile generation enabled (profile-gen-stamp) - run task to generate profile information (profile-run-stamp) - build optimized Python using above information (profile-opt) Use the "stamp" files profile-gen-stamp and profile-run-stamp to record completion of the first two steps. The first will be cleaned by the "clobber" target, the second by "profile-removal". Other minor changes: - remove the "build_all_use_profile" target. I don't expect callers of the makefile to use this target so that should be safe. - remove "$(MAKE) clean" at the start of the profile-gen-stamp target. I assume that a profiled build normally starts from a clean source or build tree and so cleaning should not be required. Having the clean there causes an interruption of the profile-gen result in everthing getting built from scratch. - remove execution of "profile-removal" at end of "profile-opt". I don't see any reason to not to keep the profile information, given the cost to generate it. Removing the stamp files will force a re-build.
gpshead
approved these changes
Nov 1, 2017
gpshead
left a comment
gpshead
left a comment
Member
There was a problem hiding this comment.
Overall LGTM, thank you for doing this. It should make repeated profile-opt builds easier to deal with.
there is a chance that someone could build with an outdated profile if they haven't run make clobber or make distclean - but that is rarely an issue. Important profile-opt builds likely to be included in a release anywhere should always be done with either a fresh build tree or at least after a make distclean.
| $(MAKE) profile-removal | ||
| # This is an expensive target to build and it does not have proper | ||
| # makefile dependancy information. So, we create a "stamp" file | ||
| # to record that it has been preformed and does not have to get |
Member
There was a problem hiding this comment.
I'd edit this to read
# to record its completion to avoid rerunning it unnecessarily.
gpshead
reviewed
Nov 1, 2017
|
|
||
| # Compile with profile generation | ||
| # | ||
| # Run "make clean" and remove stamp file to force re-build of the profile |
Member
There was a problem hiding this comment.
these should read "make clobber"
Add a profile-clean-stamp target. We must make sure that Python is built from a clean tree with -fprofile-generate before running the profile task.
embray
pushed a commit
to embray/cpython
that referenced
this pull request
Nov 9, 2017
…#4223) * bpo-28643: Record profile-opt build progress with stamp files The profile-opt makefile target is expensive to build. Since the makefile does not contain complete dependency information for this target, much extra work can get done if the build is interrupted and re-started. Even running "make" a second time will result in a huge amount of redundant work. As a minimal fix (rather than removing recursive "make" and adding a proper dependency graph), split the profile-opt target into parts: - ensure tree is clean (profile-clean-stamp) - build with profile generation enabled (profile-gen-stamp) - run task to generate profile information (profile-run-stamp) - build optimized Python using above information (profile-opt) We use "stamp" files to record completion of the steps. Running "make clean" will not remove the profile-run-stamp file. Other minor changes: - remove the "build_all_use_profile" target. I don't expect callers of the makefile to use this target so that should be safe. - remove execution of "profile-removal" at end of "profile-opt". I don't see any reason to not to keep the profile information, given the cost to generate it. Removing the "profile-run-stamp" file will force re-generation of it.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
The profile-opt makefile target is expensive to build. Since the
makefile does not contain complete dependency information for this
target, much extra work can get done if the build is interrupted and
re-started. Even running "make" a second time will result in a huge
amount of redundant work.
As a minimal fix (rather than removing recursive "make" and adding a
proper dependency graph), split the profile-opt target into three parts:
Use the "stamp" files profile-gen-stamp and profile-run-stamp to
record completion of the first two steps. The first will be cleaned by
the "clobber" target, the second by "profile-removal".
Other minor changes:
remove the "build_all_use_profile" target. I don't expect callers
of the makefile to use this target so that should be safe.
remove "$(MAKE) clean" at the start of the profile-gen-stamp target.
I assume that a profiled build normally starts from a clean source
or build tree and so cleaning should not be required. Having the
clean there causes an interruption of the profile-gen result in
everthing getting built from scratch.
remove execution of "profile-removal" at end of "profile-opt". I
don't see any reason to not to keep the profile information, given
the cost to generate it. Removing the stamp files will force a
re-build.
https://bugs.python.org/issue28643