GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-101282: Apply BOLT optimisations to libpython for shared builds by indygreg · Pull Request #104709 · python/cpython

Conversation

@indygreg

@indygreg indygreg commented May 20, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

(This change is a quick and dirty way to merge some of the build system improvements I'm proposing in gh-101093 before the 3.12 feature freeze. I wanted to scope bloat myself to fix some longstanding deficiencies in the build system around profile-guided builds. But I'm getting soft resistance to the reviews so close to the freeze deadline and it is obvious that we need a simpler solution to hit the 3.12 deadline. While this change is quick and dirty, it attempts to not make things worse.)

Before this change, we only applied bolt to the main python binary. After this change, we apply bolt to libpython if it is configured. In shared library builds, most of the C code is in libpython so it is critical to apply bolt to libpython to realize bolt benefits.

This change also reworks how bolt instrumentation is applied. It effectively removes the readelf based logic added in gh-101525 and replaces it with a mechanism that saves a copy of the pre-bolt binary and restores that copy when necessary. This allows us to perform bolt optimizations without having to manually delete the output binary to force a new bolt run.

We also add a new make target for purging bolt files and hook it up to clean so bolt state is purged when appropriate.

.gitignore rules have been added to ignore files related to bolt.

Before and after this refactor, make will no-op after a previous run. Both versions should also share common make DAG deficiencies where targets fail to trigger as often as they need to or can trigger prematurely in certain scenarios. e.g. after this change you may need to rm profile-bolt-stamp to force a bolt run because there aren't appropriate non-phony targets for bolt's make target to depend on. Fixing this is a non-trivial amount of work that will likely have to wait until the 3.13 window.

To make it easier to iterate on custom BOLT settings, the flags to pass to instrumentation and application are now defined in configure and can be overridden by passing BOLT_INSTRUMENT_FLAGS and BOLT_APPLY_FLAGS.

  • Issue: Enhance the BOLT build process #101282

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@indygreg
indygreg requested review from corona10 and erlend-aasland as code owners May 20, 2023 23:12
@bedevere-bot bedevere-bot added the awaiting review label May 20, 2023
@bedevere-bot bedevere-bot mentioned this pull request May 20, 2023
Enhance the BOLT build process #101282
Closed
4 tasks done
@erlend-aasland
erlend-aasland requested review from mdboom and zware May 21, 2023 07:11
corona10
corona10 requested changes May 21, 2023
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update docs for BOLT_INSTRUMENT_FLAGS and BOLT_APPLY_FLAGS : https://docs.python.org/3.12/using/configure.html#performance-options

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-bot bedevere-bot removed the awaiting review label May 21, 2023
@bedevere-bot

bedevere-bot commented May 21, 2023

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added the awaiting changes label May 21, 2023
corona10
corona10 reviewed May 21, 2023
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@indygreg
pythongh-101282: rework the BOLT build process
555686d
(This change is a quick and dirty way to merge some of the build system
improvements I'm proposing in pythongh-101093 before the 3.12 feature freeze.
I wanted to scope bloat myself to fix some longstanding deficiencies in
the build system around profile-guided builds. But I'm getting soft
resistance to the reviews so close to the freeze deadline and it is
obvious that we need a simpler solution to hit the 3.12 deadline. While
this change is quick and dirty, it attempts to not make things worse.)

Before this change, we only applied bolt to the main python binary.
After this change, we apply bolt to libpython if it is configured. In
shared library builds, most of the C code is in libpython so it is
critical to apply bolt to libpython to realize bolt benefits.

This change also reworks how bolt instrumentation is applied. It
effectively removes the readelf based logic added in pythongh-101525 and
replaces it with a mechanism that saves a copy of the pre-bolt binary
and restores that copy when necessary. This allows us to perform
bolt optimizations without having to manually delete the output binary
to force a new bolt run.

We also add a new make target for purging bolt files and hook it up
to `clean` so bolt state is purged when appropriate.

`.gitignore` rules have been added to ignore files related to bolt.

Before and after this refactor, `make` will no-op after a previous run.
Both versions should also share common make DAG deficiencies where
targets fail to trigger as often as they need to or can trigger
prematurely in certain scenarios. e.g. after this change you may need
to `rm profile-bolt-stamp` to force a bolt run because there aren't
appropriate non-phony targets for bolt's make target to depend on.
Fixing this is a non-trivial amount of work that will likely have to
wait until the 3.13 window.

To make it easier to iterate on custom BOLT settings, the flags to
pass to instrumentation and application are now defined in configure
and can be overridden by passing `BOLT_INSTRUMENT_FLAGS` and
`BOLT_APPLY_FLAGS`.
@indygreg
indygreg force-pushed the rework-bolt branch from f7851cd to 555686d Compare May 21, 2023 20:14
@indygreg

indygreg commented May 21, 2023

Copy link
Copy Markdown
Contributor Author

Please update docs for BOLT_INSTRUMENT_FLAGS and BOLT_APPLY_FLAGS : https://docs.python.org/3.12/using/configure.html#performance-options

Done in latest push.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland added 2 commits May 21, 2023 23:43
@erlend-aasland
Fixup doc formatting
52a266e
@erlend-aasland
M4 style nits
3cf0f36
erlend-aasland
erlend-aasland reviewed May 21, 2023
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cleaned up the docs and AC code; hope you don't mind.

I left some questions. Regarding BOLT technical stuff, I lean on Dong-hee's review.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread .gitignore Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Makefile.pre.in Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Makefile.pre.in Resolved

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented May 21, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Done in latest push.

We appreciate if you don't force-push:

  • It does not play very nice with the GitHub UX (messes up CI runs, commit history, review comments, etc.)
  • We often collaborate on PRs; pulling in new changes using git merge --no-ff is more friendly to our workflow

(This is also mentioned in the devguide.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland
erlend-aasland reviewed May 21, 2023
View reviewed changes
Comment thread configure.ac Resolved

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
Remove extra backtick
972dcb2
@erlend-aasland
erlend-aasland requested a review from corona10 May 22, 2023 11:37
erlend-aasland
erlend-aasland approved these changes May 22, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels May 22, 2023
@erlend-aasland erlend-aasland changed the title gh-101282: rework the BOLT build process gh-101282: Apply BOLT optimisations to libpython for shared builds May 22, 2023
@erlend-aasland
erlend-aasland merged commit 5360cb3 into python:main May 22, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label May 22, 2023
@erlend-aasland

erlend-aasland commented May 22, 2023

Copy link
Copy Markdown
Contributor

Thanks, Greg and Dong-hee!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@zaniebot zaniebot mentioned this pull request Oct 22, 2026
gh-119696: Apply BOLT profiles to relocatable install binaries zanieb/cpython#56
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland approved these changes
@zware zware Awaiting requested review from zware
@mdboom mdboom Awaiting requested review from mdboom
@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@indygreg @bedevere-bot @erlend-aasland @corona10
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.