Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-108716: Turn off deep-freezing of modules. #108722

Merged
merged 5 commits into from Sep 8, 2023

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Aug 31, 2023

Performance impact is neutral, maybe a little positive, but noisy

Startup might be a bit slower: no-site is 2% faster, but normal startup 5% slower.

However, without deepfreeze it is much easier to implement things like: faster-cpython/ideas#566 or #99555 which should more than compensate.

@brandtbucher
Copy link
Member

We may want to benchmark this on the other machines too, but in general I'm excited by this!

@markshannon
Copy link
Member Author

@vstinner
Copy link
Member

vstinner commented Sep 6, 2023

FYI I merged my PR #108741 to fix a race condition in make regen-all in the main branch. Backports to 3.11 and 3.12 are on-going.

@vstinner
Copy link
Member

vstinner commented Sep 6, 2023

Windows 0% slower, Mac 1% faster

Oh right, for the comparison to the main branch, I see:

Windows:

  • python_startup_no_site: 16.4 ms => 16.2 ms: 1.01x faster
  • python_startup: 19.3 ms => 19.8 ms: 1.02x slower

macOS:

  • python_startup_no_site: 9.40 ms => 9.15 ms: 1.03x faster
  • python_startup: 11.5 ms => 11.8 ms: 1.02x slower

To be honest, it's surprising that disabling an optimization supposed to make Python startup actually... has no effect (or might be faster, but that can be noise of the benchmark?).

@vstinner
Copy link
Member

vstinner commented Sep 6, 2023

What I can say is that it has a nice impact on build performance! :-)

Comparison main => this PR:

  • gcc -O0 (pydebug): 0m20,152s => 0m13,200s
  • gcc -O3 (release): 0m39,349s => 0m35,819s

Python configured with ./configure --with-pydebug or ./configure (without LTO, without PGO). I ran time make -j14 to measure build time on my laptop with 12 threads (6 CPU cores). Quick-and-dirty non scientific benchmark, I only ran make once to measure :-)

Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@markshannon markshannon merged commit 15d4c9f into python:main Sep 8, 2023
24 checks passed
@vstinner
Copy link
Member

vstinner commented Sep 8, 2023

It's unclear to me if Python/deepfreeze/deepfreeze.c is still used or not. When I build Python with make, it's not built. But make-regen creates the file. Is it intended?

# Dependencies which can add and/or remove _Py_ID() identifiers:
# - deepfreeze.c
# - "make clinic"
.PHONY: regen-global-objects
regen-global-objects: $(srcdir)/Tools/build/generate_global_objects.py $(DEEPFREEZE_C) clinic
	$(PYTHON_FOR_REGEN) $(srcdir)/Tools/build/generate_global_objects.py

@markshannon
Copy link
Member Author

Python/deepfreeze/deepfreeze.c shouldn't be used any more. I don't want to remove the tools yet, but we probably should remove the build target.

@vstinner
Copy link
Member

It seems like the Windows build system was not updated. deepfreeze.c is still generated on Windows, no?

This rule in PCbuild/_freeze_module.vcxproj:

  <Target Name="_RebuildDeepFrozen"
          AfterTargets="_RebuildFrozen"
          DependsOnTargets="FindPythonForBuild"
          Condition="$(Configuration) != 'PGUpdate'">
    <!-- BEGIN deepfreeze rule -->
        <Exec Command='$(PythonForBuild) "$(PySourcePath)Tools\build\deepfreeze.py" ^
                 "$(PySourcePath)Python\frozen_modules\importlib._bootstrap.h:importlib._bootstrap" ^
                 "$(PySourcePath)Python\frozen_modules\importlib._bootstrap_external.h:importlib._bootstrap_external" ^
                 "$(PySourcePath)Python\frozen_modules\zipimport.h:zipimport" ^
                 "$(PySourcePath)Python\frozen_modules\abc.h:abc" ^
                 "$(PySourcePath)Python\frozen_modules\codecs.h:codecs" ^
                 "$(PySourcePath)Python\frozen_modules\io.h:io" ^
                 "$(PySourcePath)Python\frozen_modules\_collections_abc.h:_collections_abc" ^
                 "$(PySourcePath)Python\frozen_modules\_sitebuiltins.h:_sitebuiltins" ^
                 "$(PySourcePath)Python\frozen_modules\genericpath.h:genericpath" ^
                 "$(PySourcePath)Python\frozen_modules\ntpath.h:ntpath" ^
                 "$(PySourcePath)Python\frozen_modules\posixpath.h:posixpath" ^
                 "$(PySourcePath)Python\frozen_modules\os.h:os" ^
                 "$(PySourcePath)Python\frozen_modules\site.h:site" ^
                 "$(PySourcePath)Python\frozen_modules\stat.h:stat" ^
                 "$(PySourcePath)Python\frozen_modules\importlib.util.h:importlib.util" ^
                 "$(PySourcePath)Python\frozen_modules\importlib.machinery.h:importlib.machinery" ^
                 "$(PySourcePath)Python\frozen_modules\runpy.h:runpy" ^
                 "$(PySourcePath)Python\frozen_modules\__hello__.h:__hello__" ^
                 "$(PySourcePath)Python\frozen_modules\__phello__.h:__phello__" ^
                 "$(PySourcePath)Python\frozen_modules\__phello__.ham.h:__phello__.ham" ^
                 "$(PySourcePath)Python\frozen_modules\__phello__.ham.eggs.h:__phello__.ham.eggs" ^
                 "$(PySourcePath)Python\frozen_modules\__phello__.spam.h:__phello__.spam" ^
                 "$(PySourcePath)Python\frozen_modules\frozen_only.h:frozen_only" ^
                 "-o" "$(PySourcePath)Python\deepfreeze\deepfreeze.c"'/>
    <!-- END deepfreeze rule -->
  </Target>

If you want to keep the infra for now, maybe this rule should just be commented with a reference to gh-108716?

@markshannon markshannon deleted the no-deep-freeze-3 branch September 26, 2023 12:56
@gvanrossum
Copy link
Member

gvanrossum commented Mar 16, 2024

This PR probably should also have updated Python/deepfreeze/README.txt to indicate that it is (currently) no longer used. It should also have updated various sections in Makefile.pre.in (including some comments) that still reference deepfreeze -- those are just distractions at this point (until we decide to actually deep-freeze some other objects, which doesn't look like it's going to happen for 3.13 at least). EDIT: See gh-116919

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants