GeezerVison

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

GeezerVison is showing github.com — GH-130090: build time analysis by chris-eibl · Pull Request #131005 · python/cpython

Conversation

chris-eibl commented Mar 9, 2025 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

This is how I generated the tables in #130090.

What to do with it?

Don't know, whether we want anything of that branch. Most probably just close.

Sorry, something went wrong.

Uh oh!

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

zanieb reacted with heart emoji

All reactions

❤️ 1 reaction

chris-eibl and others added 21 commits

February 9, 2025 16:30


          minimal changes needed for PGO with clang-cl

859c89f


          like for MSVC, don't use link time optimization

a529c39

for _freeze_module in case of clang-cl to speed up the build


          Do not build _freeze_module twice in case of PGO

26fb51f

Speeds up both MSVC and clang-cl builds.

Should most probably done in a separate PR and issue, though.


          remove /arch:AVX for blake2module.c again

6e2cb69

I've previously gotten compile errors from clang, because the needed
intrinsics were not available without that option.

Cannot reproduce anymore. Most probably, because I've upgraded to
Visual Studio 17.13.0 Preview 5.0, which now ships with clang 19.1.1
instead of 18.1.8 and they've done that for compatibility with MSVC?

Anyway, let's keep the PR small :)


          Revert "Do not build _freeze_module twice in case of PGO"

7b46aeb

This reverts commit 26fb51f.

Shall be done in a separate PR.


          always clean the profile directory when doing a new clang PGO build

ced66bd

This better matches the behaviour of build.bat in case of MSVC PGO builds.


          Merge remote-tracking branch 'origin/main' into clang-pgo

1aae65d


          Merge branch 'main' into clang-pgo

52fd5ab


          blurb it

c2ecb57


          Adding myself to ACKS

ad72df5


          extract pyproject-clangcl.props

74ec74e


          Merge branch 'python:main' into clang-pgo

79ce418


          move MergeClangProfileData to pyproject-clangcl.props

8c8aa79

and make it a target with inputs and outputs


          rename RequirePGCFiles to RequireProfileData

2a9da27

because the name is too MSVC specific


          Apply suggestions from code review

ea4de96

Co-authored-by: Steve Dower <[email protected]>


          Use -Wno-profile-instr-unprofiled in the PGUpdate case

3346b9d


          introduce CLANG_PROFILE_PATH

9db1a29


          add build time analysis

0e6f95f


          Interestingly, for debug builds I now need this, too.

0bd337d

python#130040 (comment)


          fix for older clangs

981210b


          use -flto=thin

e1dfc39

This was referenced Mar 9, 2025

Do not build the freeze module twice in case of Windows PGO builds #130419

Closed

gh-130090: Support PGO for clang-cl #129907

Merged

chris-eibl commented

Mar 9, 2025

View reviewed changes

Comment thread

Include/internal/pycore_debug_offsets.h

		_GENERATE_DEBUG_SECTION_LINUX(name)

		#if defined(MS_WINDOWS)
		#if defined(MS_WINDOWS)&& !defined(__clang__)

chris-eibl Mar 9, 2025

Copy link

Copy Markdown

Member Author

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.

Interestingly, for debug builds I now need this, too. Never needed before, see #130040 (comment). Will anyway come with that PR...

Sorry, something went wrong.

Uh oh!

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

All reactions

chris-eibl commented

Mar 9, 2025

View reviewed changes

Comment thread

PCbuild/pythoncore.vcxproj

		<ClCompile Include="..\Modules\blake2module.c">
		<PreprocessorDefinitions Condition="'$(Platform)' == 'x64'">HACL_CAN_COMPILE_SIMD128;%(PreprocessorDefinitions)</PreprocessorDefinitions>
		<PreprocessorDefinitions Condition="'$(Platform)' == 'x64'">HACL_CAN_COMPILE_SIMD256;%(PreprocessorDefinitions)</PreprocessorDefinitions>
		<AdditionalOptions Condition="'$(Platform)' == 'x64' and '$(LLVMToolsVersion)' < '19'">/arch:AVX</AdditionalOptions>

chris-eibl Mar 9, 2025

Copy link

Copy Markdown

Member Author

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.

Only as an interim taken from #130447 until we vendor the latest hacl-star that abstracts the AVX intrinsics (#130960)

Sorry, something went wrong.

Uh oh!

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

All reactions

chris-eibl commented

Mar 9, 2025

View reviewed changes

Comment thread

PCbuild/analyse_build_times.py

		@@ -0,0 +1,264 @@
		import argparse

chris-eibl Mar 9, 2025

Copy link

Copy Markdown

Member Author

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.

Not a beauty but it creates the tables for me :)

Sorry, something went wrong.

Uh oh!

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

All reactions

chris-eibl commented

Mar 9, 2025

View reviewed changes

Comment thread

PCbuild/pyproject.props

		</Task>
		</UsingTask>

		<Target Name="BeginTimeStamp" BeforeTargets="PrepareForBuild">

chris-eibl Mar 9, 2025

Copy link

Copy Markdown

Member Author

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.

Only needed for detailed timings per project. Could be guarded behind a Condition="'$(PrintBuildTimeStamps)' == 'true'", in case this shall be merged.

Sorry, something went wrong.

Uh oh!

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

All reactions

chris-eibl commented

Mar 9, 2025

View reviewed changes

Comment thread

PCbuild/analyse_build_times.py

		from datetime import datetime, date, time

		# Verstrichene Zeit 00:00:00.74
		msbuild_time_str = "Verstrichene Zeit"

chris-eibl Mar 9, 2025

Copy link

Copy Markdown

Member Author

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.

That's the thing I dislike most: msbuild does this localized :(

Sorry, something went wrong.

Uh oh!

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

All reactions


          remove debug print

9879c49

chris-eibl changed the title ~~use -flto=thin for clang-cl on Windows~~ GH-130090: build time analysis

Mar 10, 2025

bedevere-app Bot mentioned this pull request

Mar 10, 2025

Support PGO for clang-cl #130090

Closed

chris-eibl commented Mar 22, 2025

Copy link

Copy Markdown

Member Author

Closing, since this served its purpose. Build time analysis is done, in case someone else ever wants to do something like that:
PCbuild/analyse_build_times.py and BeginTimeStamp/EndTimeStamp is the only stuff needed ...

All reactions

Sorry, something went wrong.

Uh oh!

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

chris-eibl closed this

Mar 22, 2025

chris-eibl deleted the analyse_build_times branch

March 22, 2025 17:53

Reviewers

ericsnowcurrently

Awaiting requested review from ericsnowcurrently

ericsnowcurrently will be requested when the pull request is marked ready for review

ericsnowcurrently 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.

1 participant

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.

GeezerVison

Conversation

chris-eibl commented Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What to do with it?

Uh oh!

chris-eibl Mar 9, 2025

Choose a reason for hiding this comment

Uh oh!

chris-eibl Mar 9, 2025

Choose a reason for hiding this comment

Uh oh!

chris-eibl Mar 9, 2025

Choose a reason for hiding this comment

Uh oh!

chris-eibl Mar 9, 2025

Choose a reason for hiding this comment

Uh oh!

chris-eibl Mar 9, 2025

Choose a reason for hiding this comment

Uh oh!

chris-eibl commented Mar 22, 2025

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

chris-eibl commented Mar 9, 2025 •

edited

Loading

Uh oh!

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