bpo-28411: Remove "modules" field from Py_InterpreterState. #1638

ericsnowcurrently · 2017-05-17T21:16:42Z

https://bugs.python.org/issue28411

mention-bot · 2017-05-17T21:16:50Z

@ericsnowcurrently, thanks for your PR! By analyzing the history of the files in this pull request, we identified @brettcannon, @serhiy-storchaka and @tim-one to be potential reviewers.

ericsnowcurrently · 2017-05-17T22:08:52Z

@ncoghlan

vstinner

I like the overall change. I don't expect any major performance slowdown.

I just have a few suggestions on the implementation.

vstinner · 2017-05-17T22:46:07Z

Include/modsupport.h

@@ -194,6 +194,10 @@ PyAPI_FUNC(int) PyModule_ExecDef(PyObject *module, PyModuleDef *def);

 PyAPI_FUNC(PyObject *) PyModule_Create2(struct PyModuleDef*,
                                     int apiver);
+#ifndef Py_LIMITED_API
+PyAPI_FUNC(PyObject *) _PyModule_Create2(struct PyModuleDef*,


I suggest to name _PyModule_CreateInitialized().

Good point.

vstinner · 2017-05-17T22:52:17Z

Python/import.c

+   example, see issue #4236 and PyModule_Create2(). */
+
+void
+_PyImport_EnsureInitialized(PyInterpreterState *interp)


I'm not a big fan of functions calling Py_FatalError(). I suggest to return an integer and call Py_FatalError() in the caller. Rename the function to _PyImport_IsInitialized().

Or... maybe use PyDict_GetItemWithError()? :-) I'm lost in all these "Get" functions.

Good point.

Consolidating direct usage of sys.modules ended up being more work than expected. However, I think it's worth it.

vstinner · 2017-05-17T22:54:41Z

Python/import.c

-        goto error;
+    else {
+        PyObject *modules = PyImport_GetModuleDict();
+        if (PyDict_GetItem(modules, package) == NULL) {


Maybe add a private helper function to PyDict_GetItem(PyImport_GetModuleDict)? I ask because in another function PyMapping_GetItemString() is used. I don't know which one is correct :-)

pitrou · 2017-05-22T11:04:23Z

Doc/c-api/import.rst

@@ -204,6 +204,11 @@ Importing Modules
   Return the dictionary used for the module administration (a.k.a.
   ``sys.modules``).  Note that this is a per-interpreter variable.

+.. c:function:: PyObject* PyImport_GetModule(PyObject *name)
+
+   Return the already imported module with the given name.


What happens if the module is not found?

Seems this function returns a borrowed reference. This should be specially emphasized.

It shouldn't be returning a borrowed reference.

The only code that uses it (_pickle_Unpickler_find_class_impl) is written as PyImport_GetModule returning a borrowed reference.

That was an omission on my part. I should have moved the Py_DECREF(module) line down.

You should document that the function returns NULL if the module is not already imported, but raise an exception and return NULL on exception. I don't think that it's obvious from the current doc.

pitrou · 2017-05-22T11:07:44Z

Include/pystate.h

@@ -28,7 +28,6 @@ typedef struct _is {
    struct _is *next;
    struct _ts *tstate_head;

-    PyObject *modules;


I don't think we can remove a field in the middle of the struct, as that would break the stable ABI (perhaps @zooba can confirm). Instead probably replace it with void *_unused or something.

This structure is not a part of the stable ABI.

serhiy-storchaka · 2017-05-22T11:45:10Z

Python/import.c

+        return NULL;
+    }
+    if (PyDict_Check(modules))
+        return PyDict_GetItemWithError(modules, name);


This returns a borrowed reference.

Unlike PyDict_GetItem() and PyDict_GetItemString(), PyDict_GetItemWithError() does not return a borrowed reference.

I don't think so.

Ah, the documentation does not indicate it returns a borrowed reference. However, looking at the implementation indicates it is a borrowed reference. Thanks for catching that. I'll make a separate PR to fix the docs.

serhiy-storchaka · 2017-05-22T11:45:44Z

Python/import.c

+    PyObject *m = PyObject_GetItem(modules, name);
+    if (PyErr_Occurred() && !PyMapping_HasKey(modules, name))
+        PyErr_Clear();
+    return m;


But this returns an owned reference.

Is there a need to support non-dicts?

Yes, part of the point here is to support arbitrary mappings (anything you might assign to sys.modules).

serhiy-storchaka · 2017-05-22T11:50:21Z

Python/import.c

+PyImport_GetModule(PyObject *name)
+{
+    _Py_IDENTIFIER(modules);
+    PyObject *modules = _PySys_GetObjectId(&PyId_modules);


Why not use just PyImport_GetModuleDict()?

I was following some precedent. However, there isn't a strong argument either way. I'm fine with changing it to PyImport_GetModuleDict().

ericsnowcurrently · 2017-09-04T20:20:49Z

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-09-04T20:20:51Z

Nobody expects the Spanish Inquisition!

@Haypo, @serhiy-storchaka: please review the changes made to this pull request.

pitrou · 2017-09-04T20:22:17Z

Actually, I did.

vstinner

LGTM. Maybe just mention the change in https://docs.python.org/dev/whatsnew/3.7.html#porting-to-python-3-7

* 'master' of https://github.com/python/cpython: (32 commits) Conceptually, roots is a set. Also searching it as a set is a tiny bit faster (python#3338) bpo-31343: Include sys/sysmacros.h (python#3318) bpo-30102: Call OPENSSL_add_all_algorithms_noconf (python#3112) Prevent a few make suspicious warnings. (python#3341) Include additional changes to support blurbified NEWS (python#3340) Simplify NEWS entry to prevent suspicious warnings. (python#3339) bpo-31347: _PyObject_FastCall_Prepend: do not call memcpy if args might not be null (python#3329) Revert "bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. (python#1908)" (python#3337) bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. (python#1908) Fix terminology in comment and add more design rationale. (python#3335) Add comment to explain the implications of not sorting keywords (python#3331) bpo-31170: Update libexpat from 2.2.3 to 2.2.4 (python#3315) bpo-28411: Remove "modules" field from Py_InterpreterState. (python#1638) random_triangular: sqrt() is more accurate than **0.5 (python#3317) Travis: use ccache (python#3307) remove IRIX support (closes bpo-31341) (python#3310) Code clean-up. Remove unnecessary pre-increment before the loop starts. (python#3312) Regen Moduls/clinic/_ssl.c.h (pythonGH-3320) bpo-30502: Fix handling of long oids in ssl. (python#2909) Cache externals, depending on changes to PCbuild (python#3308) ...

) sys.modules is the one true source.

…ython#1638)" This reverts commit 86b7afd.

…3565) PR #1638, for bpo-28411, causes problems in some (very) edge cases. Until that gets sorted out, we're reverting the merge. PR #3506, a fix on top of #1638, is also getting reverted.

A bunch of code currently uses PyInterpreterState.modules directly instead of PyImport_GetModuleDict(). This complicates efforts to make changes relative to sys.modules. This patch switches to using PyImport_GetModuleDict() uniformly. Also, a number of related uses of sys.modules are updated for uniformity for the same reason. Note that this code was already reviewed and merged as part of python#1638. I reverted that and am now splitting it up into more focused parts.

hroncok · 2018-05-11T22:05:30Z

Maybe just mention the change in https://docs.python.org/dev/whatsnew/3.7.html#porting-to-python-3-7

What this ever put there? I've juts been bit by the changed API of _PyImport_FixupBuiltin (in gdb).

https://bugzilla.redhat.com/show_bug.cgi?id=1577396

vstinner · 2018-05-12T02:14:39Z

@hroncok: Would you mind to open a new issue at bugs.python.org? This PR has been closed since last September, so people will unlikely notice your comment :-(

hroncok · 2018-05-12T08:40:42Z

Sure, i was just checking. Here it is: https://bugs.python.org/issue33470

the-knights-who-say-ni added the CLA signed label May 17, 2017

vstinner requested changes May 17, 2017

View reviewed changes

ericsnowcurrently force-pushed the remove-modules-from-interpreter-state branch from b073e48 to 1c472f7 Compare May 20, 2017 06:42

pitrou reviewed May 22, 2017

View reviewed changes

serhiy-storchaka self-requested a review May 22, 2017 11:19

serhiy-storchaka requested changes May 22, 2017

View reviewed changes

ericsnowcurrently force-pushed the remove-modules-from-interpreter-state branch 3 times, most recently from b7d3895 to 19388d4 Compare May 25, 2017 18:31

ericsnowcurrently added 18 commits September 4, 2017 09:40

Add _PyImport_GetModuleDict().

70f6e19

Import the sys module before any other imports.

10f7443

Pass the modules dict to _PyImport_FindBuiltin().

5ea0e3b

Replace usage of interp->modules with PyImport_GetModuleDict().

29780ec

Add _PyImport_EnsureInitialized().

e6de215

Add _PyModule_Create2().

29f6b7c

Use _PyModule_Create2() for key builtin modules.

b77d73f

Check sys.modules in _PyImport_EnsureInitialized().

8275633

Drop PyInterpreterState.modules.

7c62114

Pass the modules dict to _PyImport_FixupBuiltin().

428d223

Pass PyInterpreterState to _PyImport_GetModuleDict().

833b00b

Drop _PyImport_GetModuleDict().

e0e51d2

_PyModule_Create2 -> _PyModule_CreateInitialized

88aadeb

_PyImport_EnsureInitialized() -> _PyImport_IsInitialized()

79320bd

Add _PyImport_GetModule*.

6a0215c

Allow sys.modules to be any mapping.

dabd187

Add _PyImport_SetModule*.

58d9be7

Add PyImport_GetModule().

e3b0f0c

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 4, 2017

Switch the code order back.

6544fa2

vstinner approved these changes Sep 4, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Sep 4, 2017

Add a whatsnew entry for porting.

8e86c53

ericsnowcurrently merged commit 86b7afd into python:master Sep 4, 2017

ericsnowcurrently deleted the remove-modules-from-interpreter-state branch September 4, 2017 23:54

GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this pull request Sep 10, 2017

bpo-28411: Remove "modules" field from Py_InterpreterState. (python#1638

fa0ff58

) sys.modules is the one true source.

ericsnowcurrently restored the remove-modules-from-interpreter-state branch September 14, 2017 06:04

ericsnowcurrently added a commit to ericsnowcurrently/cpython that referenced this pull request Sep 14, 2017

Revert "bpo-28411: Remove "modules" field from Py_InterpreterState. (p…

9e05a48

…ython#1638)" This reverts commit 86b7afd.

ericsnowcurrently mentioned this pull request Sep 14, 2017

bpo-31404: Revert "remove modules from Py_InterpreterState (#1638)" #3565

Merged

ericsnowcurrently mentioned this pull request Sep 14, 2017

bpo-28411: Isolate PyInterpreterState.modules #3575

Merged

This was referenced Sep 14, 2017

bpo-28411: Support other mappings in PyInterpreterState.modules. #3593

Merged

bpo-28411: Remove PyInterpreterState.modules. #3606

Closed

Mariatta removed the awaiting merge label Oct 8, 2017

ericsnowcurrently mentioned this pull request Apr 10, 2022

[subinterpreters] Eliminate PyInterpreterState.modules. #72597

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-28411: Remove "modules" field from Py_InterpreterState. #1638

bpo-28411: Remove "modules" field from Py_InterpreterState. #1638

ericsnowcurrently commented May 17, 2017 •

edited by bedevere-bot

Loading

mention-bot commented May 17, 2017

ericsnowcurrently commented May 17, 2017

vstinner left a comment

vstinner May 17, 2017

ericsnowcurrently May 17, 2017

vstinner May 17, 2017

ericsnowcurrently May 18, 2017

ericsnowcurrently May 18, 2017

vstinner May 17, 2017

pitrou May 22, 2017

serhiy-storchaka May 22, 2017

ericsnowcurrently May 22, 2017

serhiy-storchaka May 23, 2017

ericsnowcurrently May 25, 2017

vstinner Sep 4, 2017

pitrou May 22, 2017

serhiy-storchaka May 22, 2017

serhiy-storchaka May 22, 2017

ericsnowcurrently May 22, 2017

serhiy-storchaka May 23, 2017

ericsnowcurrently May 25, 2017

serhiy-storchaka May 22, 2017

ericsnowcurrently May 22, 2017

serhiy-storchaka May 22, 2017

ericsnowcurrently May 23, 2017

ericsnowcurrently commented Sep 4, 2017

bedevere-bot commented Sep 4, 2017

pitrou commented Sep 4, 2017

vstinner left a comment

hroncok commented May 11, 2018

vstinner commented May 12, 2018

hroncok commented May 12, 2018

bpo-28411: Remove "modules" field from Py_InterpreterState. #1638

bpo-28411: Remove "modules" field from Py_InterpreterState. #1638

Conversation

ericsnowcurrently commented May 17, 2017 • edited by bedevere-bot Loading

mention-bot commented May 17, 2017

ericsnowcurrently commented May 17, 2017

vstinner 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

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

ericsnowcurrently commented Sep 4, 2017

bedevere-bot commented Sep 4, 2017

pitrou commented Sep 4, 2017

vstinner left a comment

Choose a reason for hiding this comment

hroncok commented May 11, 2018

vstinner commented May 12, 2018

hroncok commented May 12, 2018

ericsnowcurrently commented May 17, 2017 •

edited by bedevere-bot

Loading