bpo-28411: Remove "modules" field from Py_InterpreterState. by ericsnowcurrently · Pull Request #1638 · python/cpython

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

I suggest to name _PyModule_CreateInitialized().

Good point.

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

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

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

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

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

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

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

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 1c472f7Compare 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 19388d4Compare 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:masterSep 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

Uh oh!

Uh oh!

Conversation

ericsnowcurrently commented May 17, 2017• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented May 17, 2017

Uh oh!

ericsnowcurrently commented May 17, 2017

Uh oh!

vstinner 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!

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!

ericsnowcurrently commented Sep 4, 2017

Uh oh!

bedevere-bot commented Sep 4, 2017

Uh oh!

pitrou commented Sep 4, 2017

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

hroncok commented May 11, 2018

Uh oh!

vstinner commented May 12, 2018

Uh oh!

hroncok commented May 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

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