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

ericsnowcurrently · 2021-08-31T23:46:19Z

[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

gvanrossum · 2021-09-01T19:02:30Z

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!).

gvanrossum

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

gvanrossum · 2021-09-01T19:04:57Z

Doc/using/cmdline.rst

@@ -480,6 +480,14 @@ Miscellaneous options
     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


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.

gvanrossum · 2021-09-01T19:06:43Z

Include/cpython/initconfig.h

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


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.

gvanrossum · 2021-09-01T19:15:09Z

Makefile.pre.in

@@ -765,6 +765,671 @@ Python/frozen_modules/zipimport.h: $(srcdir)/Programs/_freeze_module $(srcdir)/L
 		$(srcdir)/Lib/zipimport.py \
 		$(srcdir)/Python/frozen_modules/zipimport.h

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


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.

gvanrossum · 2021-09-01T21:04:28Z

Python/pathconfig.c

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


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

terryjreedy · 2021-09-11T02:01:02Z

Lib/idlelib/idle_test/test_query.py

@@ -137,8 +137,11 @@ def test_good_module_name(self):
        self.assertTrue(dialog.entry_ok().endswith('__init__.py'))
        self.assertEqual(dialog.entry_error['text'], '')
        dialog = self.Dummy_ModuleName('os.path')


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!

terryjreedy · 2021-09-11T02:03:23Z

Lib/idlelib/idle_test/test_query.py

+        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'))


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'], '')

bedevere-bot · 2021-09-11T02:03:38Z

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

gvanrossum

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

gvanrossum · 2021-09-11T03:40:51Z

Lib/ctypes/test/test_values.py

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


What stdout content is being captured?

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

gvanrossum · 2021-09-11T03:52:39Z

Lib/test/support/import_helper.py

@@ -185,7 +185,7 @@ class CleanImport(object):
            importlib.import_module("foo") # new reference
    """

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


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

Misc/NEWS.d/next/Core and Builtins/2021-08-31-17-44-51.bpo-45020.ZPI_3L.rst

gvanrossum · 2021-09-11T04:32:05Z

Programs/_freeze_module.c

+    // 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;
+    }
+


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.

ericsnowcurrently · 2021-09-13T15:50:38Z

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.

gvanrossum · 2021-09-13T15:52:51Z

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

ericsnowcurrently · 2021-09-13T15:58:41Z

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.

ericsnowcurrently · 2021-09-13T22:10:44Z

I'm splitting this PR up into 3:

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

ericsnowcurrently · 2021-09-15T16:23:45Z

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

ericsnowcurrently requested review from brettcannon, encukou, ncoghlan, rhettinger, warsaw and a team 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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ericsnowcurrently commented Aug 31, 2021 •

edited

Loading

gvanrossum commented Sep 1, 2021

gvanrossum left a comment

gvanrossum Sep 1, 2021

ericsnowcurrently Sep 3, 2021

ericsnowcurrently Sep 7, 2021

gvanrossum Sep 1, 2021

ericsnowcurrently Sep 7, 2021

gvanrossum Sep 1, 2021

ericsnowcurrently Sep 3, 2021

ericsnowcurrently Sep 8, 2021

gvanrossum Sep 1, 2021

ericsnowcurrently Sep 3, 2021

terryjreedy Sep 11, 2021

ericsnowcurrently Sep 11, 2021

terryjreedy Sep 11, 2021

bedevere-bot commented Sep 11, 2021

gvanrossum left a comment

gvanrossum Sep 11, 2021

ericsnowcurrently Sep 13, 2021

gvanrossum Sep 11, 2021

gvanrossum Sep 11, 2021

ericsnowcurrently Sep 13, 2021

ericsnowcurrently commented Sep 13, 2021

gvanrossum commented Sep 13, 2021

ericsnowcurrently commented Sep 13, 2021 •

edited

Loading

ericsnowcurrently commented Sep 13, 2021 •

edited

Loading

ericsnowcurrently commented Sep 15, 2021

	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)

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

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

Conversation

ericsnowcurrently commented Aug 31, 2021 • edited Loading

gvanrossum commented Sep 1, 2021

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 11, 2021

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericsnowcurrently commented Sep 13, 2021

gvanrossum commented Sep 13, 2021

ericsnowcurrently commented Sep 13, 2021 • edited Loading

ericsnowcurrently commented Sep 13, 2021 • edited Loading

ericsnowcurrently commented Sep 15, 2021

ericsnowcurrently commented Aug 31, 2021 •

edited

Loading

ericsnowcurrently commented Sep 13, 2021 •

edited

Loading

ericsnowcurrently commented Sep 13, 2021 •

edited

Loading