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

pull some import state out of the interpreter state #58820

Closed
ericsnowcurrently opened this issue Apr 18, 2012 · 11 comments
Closed

pull some import state out of the interpreter state #58820

ericsnowcurrently opened this issue Apr 18, 2012 · 11 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@ericsnowcurrently
Copy link
Member

BPO 14615
Nosy @loewis, @brettcannon, @ericsnowcurrently

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2012-04-19.01:33:05.070>
created_at = <Date 2012-04-18.16:32:41.758>
labels = ['interpreter-core']
title = 'pull some import state out of the interpreter state'
updated_at = <Date 2012-04-19.01:33:29.108>
user = 'https://github.com/ericsnowcurrently'

bugs.python.org fields:

activity = <Date 2012-04-19.01:33:29.108>
actor = 'eric.snow'
assignee = 'none'
closed = True
closed_date = <Date 2012-04-19.01:33:05.070>
closer = 'eric.snow'
components = ['Interpreter Core']
creation = <Date 2012-04-18.16:32:41.758>
creator = 'eric.snow'
dependencies = []
files = []
hgrepos = []
issue_num = 14615
keywords = []
message_count = 11.0
messages = ['158638', '158640', '158643', '158647', '158652', '158659', '158666', '158671', '158690', '158692', '158693']
nosy_count = 3.0
nosy_names = ['loewis', 'brett.cannon', 'eric.snow']
pr_nums = []
priority = 'normal'
resolution = 'rejected'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue14615'
versions = ['Python 3.3']

@ericsnowcurrently
Copy link
Member Author

Once the dust clears from the bpo-2377 and bpo-13959, we should consider what import-state-related members of PyInterpreterState (Include/pystate.h) can be removed. This is in the interest of simplifying the interpreter state.

The most straightforward candidate is 'modules_reloading' (since reload() will likely become pure python), but we'll have to make sure we do not introduce any race conditions.

Another candidate that could probably go away, regardless of the import work, is 'modules_by_index'. As far as I can see, there is only one use of interp->modules_by_index in the cpython code-base: PyState_FindModule() in Python/pystate.c. Likewise there is only one use of PyState_FindModule(): atexit_callfuncs() in Modules/atexitmodule.c.

Ultimately I'd love it if modules were also removed from the interpreter state, but doing that is not so cut-and-dry.

@ericsnowcurrently ericsnowcurrently added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Apr 18, 2012
@brettcannon
Copy link
Member

So PyState_FindModule() is an (undocumented) part of the public API, which means that it has to somehow be supported to keep ABI compatibility (unless I am reading PEP-384 incorrectly).

@ericsnowcurrently
Copy link
Member Author

Curse you, undocumented API that we have to support! It could still wrap a simple get() on sys.modules, roughly like this:

PyObject*
PyState_FindModule(struct PyModuleDef* m)
{
    PyObject* modules, module;
    modules = PyImport_GetModuleDict();
    if (modules == NULL)
        return NULL;
    module = PyDict_GetItemString(sys_modules, m->m_name);
    return module==Py_None ? NULL : module;
}

@brettcannon
Copy link
Member

Right, so the question is why wasn't that done in the first place? Who decided this modules_by_index concept was even worth it?

@loewis
Copy link
Mannequin

loewis mannequin commented Apr 18, 2012

Another candidate that could probably go away, regardless of the
import work, is 'modules_by_index'. As far as I can see, there is
only one use of interp->modules_by_index in the cpython code-base:
PyState_FindModule() in Python/pystate.c. Likewise there is only one
use of PyState_FindModule(): atexit_callfuncs() in
Modules/atexitmodule.c.

There will be certainly many more uses of PySteate_FindModule in the
future. I fail to see what is gained by this kind of change, and am
firmly -1 on removing variables arbitrarily.

@loewis loewis mannequin changed the title pull some import state out of the interpreter state pull some import state out of the interpreter state Apr 18, 2012
@ericsnowcurrently
Copy link
Member Author

Sorry, Martin, for not looking at PEP-3121 before. I was thinking modules_by_index was a lot older.

@ericsnowcurrently
Copy link
Member Author

Rather than being arbitrary, the motivation here is to limit amount of the import state that is specific to CPython. I apologize that that wasn't clear. The three import-related members of PyInterpreterState (modules, modules_reloading, and modules_by_index), are implementation-specific. Regarding each of them:

modules_by_index was explicitly designed for C extension modules, which are (mostly) CPython-specific. It's essentially a behind-the-scenes optimization. Martin's right that we should leave it alone.

modules_reloading is an artifact of the C import implementation, and (like modules_by_index) doesn't have extra bearing on the import state. However, if the importlib bootstrap renders it superfluous, why not yank it?

Finally, modules (the biggie) is accessible as sys.modules. importlib uses sys.modules. The C import implementation used interp->modules directly. [1] Most (all) of that usage is going away. Again, _if_ that is the case, why keep it around?

Granted, the C implementation takes advantage of interp->modules as an optimized path for getting a module (vs. the extra step of grabbing sys.modules directly), so perhaps the intention is to keep that fast path for C extension modules, etc. However, I'm naively skeptical of how much performance that buys you when all is said and done.

Just to be clear, I do _not_ want to make changes willy-nilly. (I've even grown more conservative in what discussion topics I bring up.) This issue has no urgency attached to it, in my mind. It is the result of an actionable conversation that I didn't want to lose track of. If anything here is doable for 3.3, great! If not, that's fine too. If people think it's a bad idea, I'll accept that gladly, but I think consideration of the idea is justifiable. I'm glad there are plenty of sensible minds around to keep Python on a good track. :)

[1] This causes a problem in a corner case (see bpo-12633), but it's easy to deal with.

@loewis
Copy link
Mannequin

loewis mannequin commented Apr 18, 2012

Rather than being arbitrary, the motivation here is to limit amount
of the import state that is specific to CPython.

What is gained by doing that?

Finally, modules (the biggie) is accessible as sys.modules.
importlib uses sys.modules. The C import implementation used
interp->modules directly. [1] Most (all) of that usage is going
away. Again, _if_ that is the case, why keep it around?

I think each of them should be considered on a case-by-case basis.
Feel free to submit a patch that eliminates ->modules. People may
find reasons not to do so when they see an actual patch.

I suggest to close this issue, and encourage people who have the
desire to eliminate certain state as individual patches.

Just to be clear, I do _not_ want to make changes willy-nilly. (I've
even grown more conservative in what discussion topics I bring up.)
This issue has no urgency attached to it, in my mind. It is the
result of an actionable conversation that I didn't want to lose track
of.

Then I think it doesn't belong in this bug tracker. I have five or
ten "grand plans" of things that should change in Python at some point;
putting them into a bug tracker is only confusing people, though, since
no implementation might be coming forward (for some of the things, I
have been pondering for the last eight years). For many of the things,
I ended up writing PEPs since they were significant changes.

So if this is one of your grand plans, feel free to mention it on
python-dev. Putting it on the bug tracker asks for specific action.
If I had to act on this issue, I'd outright reject it.

@brettcannon
Copy link
Member

Eric put this in the bug tracker because I asked him to; I'm getting more emails about stuff about importlib that it's a little hard to keep track of everything.

But originally this was about exposing what is left of the C-level APIs so it no longer was hidden. I'm fine with rescoping this bug to only looking at modules_reloading.

@brettcannon brettcannon changed the title pull some import state out of the interpreter state pull some import state out of the interpreter state Apr 19, 2012
@ericsnowcurrently
Copy link
Member Author

Thanks for your feedback, Martin. I'll go ahead and make a new issue for modules_reloading (and one for interp->modules when appropriate).

@ericsnowcurrently
Copy link
Member Author

for modules_reloading, bpo-14618

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)
Projects
None yet
Development

No branches or pull requests

2 participants