bpo-45020: Freeze the modules imported during startup. by ericsnowcurrently · Pull Request #28107 · python/cpython · GitHub

[I'm going to split this PR up.]

Doing this provides some significant performance gains for runtime startup. This change also adds a command-line flag (-X frozen_modules=[on|off]) that allows users to opt out of (or into) using frozen modules. When run in a development environment it defaults to opting out, to avoid contributors wasting time trying to figure out why their changes aren't getting used.

Note that I plan on updating the tests for the frozen modules so they run against the frozen and unfrozen modules, like we do with importlib. We'll also be looking at making frozen modules more like source modules in a separate PR.

There are only a handful of things left to do here before this could be merged:

get the "check_generated_files" job passing
deal with frozen modules on the WIndows builds
sort out PyConfig.stdlib_dir on Windows
ensure default "-X frozen_modules=on" for PGO builds
problem in test_4_daemon_threads in test_threading (timing out with "-X frozen_modules=on")
drop Python/frozen_modules/MANIFEST?

https://bugs.python.org/issue45020

Another reason not to check the generated .h files into git is that when the marshal format changes in any way, each of these files is included in the diff as changed, which makes the diff look overwhelming (this one, for example, has 151 files!).

Mostly nits; the most disturbing part is that it doesn't seem to work on Windows at all (it always believes it's installed).

This is a little vague on the exact syntax to use on the command line. Do I write

python -X frozen_modules=on

and

python -X frozen_modules=off

???

I'll clarify.

I've fixed this.

This seems like an unrelated refactoring snuck in? I had expected just one line adding int use_frozen_modules;

Same for the following few files.

This should be fixed.

Do you understand when to write $(srcdir)/ before a path and when not? It's definitely needed in shell commands, but it seems it's not used for dependencies. For example I see:

Modules/_math.o: Modules/_math.c Modules/_math.h

and none of the variables defining lists of .o files use it (e.g. MODULE_OBJS).

But usually when .h files are mentioned they do add $(srcdir), e.g. PEGEN_HEADERS or PYTHON_HEADERS.

Is this just not used any more, or does "make" only need it in certain cases?

I'll clean this up.

I've fixed this.

FWIW, I'm not sure what "make" requires so I'll follow the existing patterns. There are cases where we do use $(srcdir) with dependencies but only where the rule is also defined with $(srcdir) or there is no corresponding rule. Neither applies so I'll drop the $srcdir)/ prefix.

Using some printf() calls it looks like on Windows stdlib is NULL when run out of the development tree and we always return true here.

Yeah, the Windows builds aren't right yet.

…Py_IsInstalled()).

…ro).

os.path is an arbitrary dotted module name. I would rather switch.
Change is in two pieces because github would not do it in one.

Suggested change

      
                    dialog = self.Dummy_ModuleName('os.path')
          
                    dialog = self.Dummy_ModuleName('idlelib.idle')

I've made this change. Thanks!

Suggested change

      
                    with import_helper.CleanImport('os', 'os.path'):
          
                        import os.path
          
                        filename = dialog.entry_ok()
          
                    self.assertEqual(dialog.entry_error['text'], '')
          
                    self.assertTrue(filename.endswith('path.py'))
          
                    self.assertTrue(dialog.entry_ok().endswith('idle.py'))
          
                    self.assertEqual(dialog.entry_error['text'], '')

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Reviewed up to the latest commit, except I was daunted by the many changes in marshal.c and I will review those later.

What stdout content is being captured?

The frozen test module (hello.py) prints out Hello world!.

Can you update the class docstring to mention that it also disabled frozen modules unless usefrozen=True is passed?

Could we fix this problem in marshal itself? Then everyone (notably all regular, non-frozen PYC files) would benefit.

I've opened https://bugs.python.org/issue45186 to cover this separately.

Reviewed up to the latest commit, except I was daunted by the many changes in marshal.c and I will review those later.

Yeah, sorry about that. I can roll things back if you think it's too much. FWIW, it might be easier to review the 3 marshal-related changes separately:

The first one re-arranges code to make the second and third commits cleaner. None of the changes in marshal.c should have any effect on the behavior of the marshal module and only change the behavior of _freeze_module.c.

I would much rather review separate PRs than separate commits on a single PR. Review and iteration can be much more focused that way.

Regarding the "address sanitizer" CI failure, this is similar to the earlier failure with the macOS job. When running with frozen modules, test_4_daemon_threads in test_threading is timing out (but only on a non-debug build). This implies some correlation between finalization of frozen modules and of daemon threads, leading to a deadlock or infinite loop. I'm going to take a look at the runtime finalization code to track this down.

[UPDATE] Never mind. The test relies on os.__file__ and runs in a loop. So it's not an issue with runtime finalization.

I'm splitting this PR up into 3:

There will be other PRs but these three cover the fundamental changes.

Pretty much everything from this PR has been merged through those other PRs.

ericsnowcurrently requested review from a team, brettcannon, encukou, ncoghlan, rhettinger and warsaw as code owners August 31, 2021 23:46

the-knights-who-say-ni added the CLA signed label Aug 31, 2021

bedevere-bot added the awaiting core review label Aug 31, 2021

ericsnowcurrently requested a review from gvanrossum August 31, 2021 23:46

ericsnowcurrently mentioned this pull request Aug 31, 2021

Freeze/link all stdlib modules imported during startup. faster-cpython/ideas#82

Closed

23 of 46 tasks

ericsnowcurrently marked this pull request as draft September 1, 2021 17:17

gvanrossum reviewed Sep 1, 2021

View reviewed changes

ericsnowcurrently force-pushed the frozen-startup-modules branch from a381ccf to cdd9784 Compare September 3, 2021 01:30

ericsnowcurrently added 16 commits September 2, 2021 19:32

Fix freeze_module() in freeze_modules.py.

a8bd3fc

Add FrozenSource and FrozenModule to freeze_modules.py.

6555ae4

Leave all non-required frozen modules uncommitted.

3a63f92

Leave *all* frozen modules uncommitted.

0587328

Generate the list of frozen modules used by test_ctypes.

9881456

Freeze the stdlib modules loaded during startup.

3df3e23

Add the -X frozen_modules CLI option and PyConfig.use_frozen_modules.

bfd733c

Ignore frozen modules depending on "-X frozen_modules".

ca952fc

Explicitly control PyConfig.use_frozen_modules during tests.

b4ca34c

Remember the stdlib dir during startup (adding PyConfig.stdlib_dir).

ef5f3f7

Add _Py_GetMainConfig().

4b630b8

Let _Py_GetStdlibDir() fall back to a config.

dd1a498

Add _PyConfig_InitImportConfig().

af07489

Identify whether or not the executable is running installed (adding _…

4e4baeb

…Py_IsInstalled()).

Default to "off" if in development.

344a5ec

Remember if built with --enable-optimizations (adding the _Py_OPT mac…

8710d80

…ro).

ericsnowcurrently added 3 commits September 10, 2021 10:42

Drop -_Py_GetStdlibDir() and PyConfig.stdlib_dir.

358ab69

Default to "-X frozen_modules=off" if built with --with-debug.

1fc1198

Fix test_embed.

37eb1f8

ericsnowcurrently marked this pull request as ready for review September 11, 2021 00:29

ericsnowcurrently requested review from terryjreedy and tiran as code owners September 11, 2021 00:29

ericsnowcurrently added 4 commits September 10, 2021 19:02

Clean up the marshal code.

2e05771

Add WFILE.refs.nonref.

82b4f85

Add _PyMarshal_WriteForFreezing().

097fc40

Fix the ASAN job.

a333fa3

ericsnowcurrently force-pushed the frozen-startup-modules branch from 5c669d8 to a333fa3 Compare September 11, 2021 01:04

terryjreedy requested changes Sep 11, 2021

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Sep 11, 2021

ericsnowcurrently added 2 commits September 10, 2021 20:53

Fix the Windows builds.

8abf4a0

Use a simpler change for test_idle.

1005321

gvanrossum reviewed Sep 11, 2021

View reviewed changes

ericsnowcurrently marked this pull request as draft September 13, 2021 16:41

Fix test_4_daemon_threads.

7d62747

ericsnowcurrently mentioned this pull request Sep 13, 2021

bpo-45019: Do some cleanup related to frozen modules. #28319

Merged

This was referenced Sep 13, 2021

bpo-45020: Add -X frozen_modules=[on|off] to explicitly control use of frozen modules. #28320

Merged

bpo-45020: Freeze some of the modules imported during startup. #28335

Merged

ericsnowcurrently closed this Sep 15, 2021

ericsnowcurrently deleted the frozen-startup-modules branch September 15, 2021 16:23

                    objects and pyc files are desired as well as supressing the extra visual
                    location indicators when the interpreter displays tracebacks. See also
                    :envvar:`PYTHONNODEBUGRANGES`.
+                 * ``-X frozen_modules`` determines whether or not frozen modules are

+                  // To avoid unnecessary duplication during marshaling, all "complex"
+                  // objects get stored in a lookup table that allows them to be
+                  // referenced by later entries using an integer ID.  However, this
+                  // optimization is not applied if the refcount is exactly 1.
+                  //
+                  // The problem is that any objects cached during runtime
+                  // initialization will get re-used in the code object during
+                  // compilation.  This means some objects will have a refcount > 1,
+                  // even though there is only one instance within the code object.
+                  // This is problematic it this happens inconsistently, such as only
+                  // in non-debug builds (e.g. init_filters() in Python/_warnings.c);
+                  // the generated marshal data we are freezing will be different even
+                  // though the Python code hasn't changed.
+                  //
+                  // We address this by giving marshal the set of objects with
+                  // refcount > 1 that actually only get used once in the code object.
+                  // This mostly consists of interned strings and small integers.
+                  PyObject *refs_before = get_ref_counts();
+                  if (refs_before == NULL) {
+                      return NULL;
+                  }

+                  int user_site_directory;
+                  wchar_t *check_hash_pycs_mode;
+                  bool use_frozen_modules;

               		$(srcdir)/Lib/zipimport.py \
               		$(srcdir)/Python/frozen_modules/zipimport.h
+              Python/frozen_modules/abc.h: $(srcdir)/Programs/_freeze_module $(srcdir)/Lib/abc.py

+                      }
+                  }
+                  /* We fall back to assuming this is an installed Python. */
+                  return true;

	dialog = self.Dummy_ModuleName('os.path')
	dialog = self.Dummy_ModuleName('idlelib.idle')

		with import_helper.frozen_modules(), captured_stdout():
		spec = importlib.util.find_spec(modname)

                   """
-                  def __init__(self, *module_names):
+                  def __init__(self, *module_names, usefrozen=False):

bpo-45020: Freeze the modules imported during startup. by ericsnowcurrently · Pull Request #28107 · python/cpython · GitHub

Repository navigation

bpo-45020: Freeze the modules imported during startup. - #28107

bpo-45020: Freeze the modules imported during startup.#28107ericsnowcurrently wants to merge 61 commits intopython:mainpython/cpython:mainfrom ericsnowcurrently:frozen-startup-modulesericsnowcurrently/cpython:frozen-startup-modulesCopy head branch name to clipboard

Conversation

ericsnowcurrently commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Sep 1, 2021

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 11, 2021

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently commented Sep 13, 2021

Uh oh!

gvanrossum commented Sep 13, 2021

Uh oh!

ericsnowcurrently commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently commented Sep 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bpo-45020: Freeze the modules imported during startup.#28107
ericsnowcurrently wants to merge 61 commits into
python:mainfrom
ericsnowcurrently:frozen-startup-modules

ericsnowcurrently commented Aug 31, 2021 •
edited
Loading

ericsnowcurrently commented Sep 13, 2021 •
edited
Loading

ericsnowcurrently commented Sep 13, 2021 •
edited
Loading