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

Add an id field to PyInterpreterState. #73288

Closed
ericsnowcurrently opened this issue Dec 29, 2016 · 32 comments
Closed

Add an id field to PyInterpreterState. #73288

ericsnowcurrently opened this issue Dec 29, 2016 · 32 comments
Assignees
Labels
3.7 interpreter-core type-feature

Comments

@ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented Dec 29, 2016

BPO 29102
Nosy @brettcannon, @ncoghlan, @vstinner, @ericsnowcurrently, @serhiy-storchaka, @zooba
PRs
  • #1639
  • Files
  • interpreter-id.diff
  • interpreter-id-2.diff
  • interpreter-id-3.diff
  • interpreter-id-4.diff
  • 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 = 'https://github.com/ericsnowcurrently'
    closed_at = <Date 2017-05-25.00:22:01.977>
    created_at = <Date 2016-12-29.06:53:42.384>
    labels = ['interpreter-core', 'type-feature', '3.7']
    title = 'Add an id field to PyInterpreterState.'
    updated_at = <Date 2017-05-25.00:22:01.976>
    user = 'https://github.com/ericsnowcurrently'

    bugs.python.org fields:

    activity = <Date 2017-05-25.00:22:01.976>
    actor = 'eric.snow'
    assignee = 'eric.snow'
    closed = True
    closed_date = <Date 2017-05-25.00:22:01.977>
    closer = 'eric.snow'
    components = ['Interpreter Core']
    creation = <Date 2016-12-29.06:53:42.384>
    creator = 'eric.snow'
    dependencies = []
    files = ['46070', '46094', '46100', '46105']
    hgrepos = []
    issue_num = 29102
    keywords = ['patch']
    message_count = 32.0
    messages = ['284229', '284230', '284232', '284233', '284273', '284276', '284278', '284280', '284283', '284290', '284328', '284329', '284332', '284335', '284350', '284353', '284359', '284366', '284374', '284378', '284380', '284395', '284406', '293899', '294116', '294212', '294232', '294312', '294315', '294370', '294371', '294417']
    nosy_count = 7.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'vstinner', 'grahamd', 'eric.snow', 'serhiy.storchaka', 'steve.dower']
    pr_nums = ['1639']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue29102'
    versions = ['Python 3.7']

    @ericsnowcurrently
    Copy link
    Member Author

    @ericsnowcurrently ericsnowcurrently commented Dec 29, 2016

    Currently there isn't any way to uniquely identify an interpreter. This patch adds a new "id" field to the PyInterpreterState struct. The ID for every new interpreter is set to the value of an increasing global counter. That means that the ID is unique within the process.

    IIRC, the availability of unique ID would help tools that make use of subinterpreters, like mod_wsgi. It is also necessary for any effort to expose interpreters in Python-level code (which is the subject of other ongoing work).

    The patch also adds:

    unsigned long PyInterpreterState_GetID(PyInterpreterState *interp)

    Note that, without a Python-level interpreters module, testing this change is limited to extending the existing test code in test_capi.

    @ericsnowcurrently ericsnowcurrently self-assigned this Dec 29, 2016
    @ericsnowcurrently ericsnowcurrently added interpreter-core type-feature labels Dec 29, 2016
    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 29, 2016

    Why not use just the pointer to PyInterpreterState itself?

    @ericsnowcurrently
    Copy link
    Member Author

    @ericsnowcurrently ericsnowcurrently commented Dec 29, 2016

    Pointers can get re-used, so they aren't temporally unique.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 29, 2016

    What is the use case of keeping the uniqueness after deleting an interpreter?

    @zooba
    Copy link
    Member

    @zooba zooba commented Dec 29, 2016

    Tracking purposes mainly, so someone outside the interpreter state can tell when it's no longer there. Making interpreter states weak-referencable would have a similar effect, and could very well use this id if we didn't need the callback.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 29, 2016

    If add an API for getting an unique ID of the interpreter state, is it needed to add an API for getting the interpreter state by ID?

    @ericsnowcurrently
    Copy link
    Member Author

    @ericsnowcurrently ericsnowcurrently commented Dec 29, 2016

    Three reasons come to mind:

    1. threads are identified by small integers
    2. long, random-looking IDs are not human-friendly, and subinterpreter IDs will be used like thread IDs are
    3. related to what Steve said, temporally unique IDs allow us to be confident about whether or not an interpreter has been destroyed (and how many interpreters there have been)

    Since PyInterpreterState is not a PyObject, using weakrefs to address the third point won't work, right?

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 29, 2016

    There is an issue with integer identifiers of threads. See bpo-25658 and https://mail.python.org/pipermail/python-ideas/2016-December/043983.html.

    @zooba
    Copy link
    Member

    @zooba zooba commented Dec 29, 2016

    That's an issue with TLS initialisation, not thread IDs. It's easily solved by defining an "uninitialized" value (e.g. 0) and an "invalid" value (e.g. -1).

    Interpreter states are in a linked list, so you can traverse the list to find one by ID.

    WRT weakrefs, we can't use them directly, but I suspect the higher-level API will need it. Possibly adding a callback on finalisation would fill both needs, but I like having a reliable ID - otherwise we'll probably end up with multiple different IDs managed indirectly via callbacks. (Perhaps a single callback for when any interpreter is finalized that passes the ID through? That should work well, since the ID is designed to outlive the interpreter itself, so it can be an asynchronous notification.)

    @ericsnowcurrently
    Copy link
    Member Author

    @ericsnowcurrently ericsnowcurrently commented Dec 29, 2016

    Interpreter states are in a linked list, so you

    can traverse the list to find one by ID.

    Exactly. At first I had added a PyInterpreterState_FindByID() or something
    like that. However, as you noted, I realized it wasn't necessary. :)

    WRT weakrefs, we can't use them directly, but I suspect the higher-level
    API will need it...

    Everything you said about weakrefs sounds good. We can discuss more when
    we get to that high-level API.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Dec 30, 2016

    +1 from me for the general idea.

    One subtlety with the draft implementation is that an Initialize/Finalize cycle doesn't reset the counter, which:

    1. Increases the chance of counter overflow (while admittedly still leaving it incredibly low)
    2. Means you still can't readily check whether the current interpreter is the main interpreter (i.e. the one created automatically in Py_Initialize)

    What do you think about resetting the counter back to 1 in Py_Initialize?

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 30, 2016

    What do you think about resetting the counter back to 1 in Py_Initialize?

    Wouldn't this break the main property of IDs, the uniqueness?

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Dec 30, 2016

    It depends on the scope of uniqueness we're after. threading._counter() (which is the small-integer-ID debugging counter for threading.Thread names) is a module global in the threading module, so an Initialize/Finalize cycle will reset it.

    If we wanted to track "Which Initialize/Finalize cycle is this?" *as well*, it would make more sense to me to have that as a separate "runtime" counter, such that the full coordinates of the current point of execution were:

    • runtime counter (How many times has Py_Initialize been called?)
    • interpreter counter (Which interpreter is currently active?)
    • thread name (Which thread is currently active?)

    I'll also note that in the threading module, the main thread is implicitly thread 0 (but named as MainThread) - Thread-1 is the first thread created via threading.Thread. So it may make sense to use a signed numeric ID, with 0 being the main interpreter, 1 being the first subinterpreter, and negative IDs being errors.

    @zooba
    Copy link
    Member

    @zooba zooba commented Dec 30, 2016

    Wouldn't this break the main property of IDs, the uniqueness?

    If we bump it up to a 64-bit ID then it'll be no worse than how we track all dict mutations.

    @ericsnowcurrently
    Copy link
    Member Author

    @ericsnowcurrently ericsnowcurrently commented Dec 30, 2016

    What do you think about resetting the counter back to 1 in Py_Initialize?

    Sounds good to me. When I was working on the patch I had the idea in the back of my mind that not resetting the counter would better support interpreter separation efforts in the future. However, after giving it some thought I don't think that's the case. So resetting it in Py_Initialize() is fine with me.

    I'll also note that in the threading module, the main thread is
    implicitly thread 0 (but named as MainThread) - Thread-1 is the first
    thread created via threading.Thread. So it may make sense to use a
    signed numeric ID, with 0 being the main interpreter, 1 being the first
    subinterpreter, and negative IDs being errors.

    I had considered that and went with an unsigned long. 0 is used for errors, and starting at 1, which effectively means the main interpreter is always 1. If we later run into overflow issues then we can sort that out at that point (e.g. by moving to a 64-bit int or even a Python int).

    I'll add comments to the patch regarding these points.

    @ericsnowcurrently
    Copy link
    Member Author

    @ericsnowcurrently ericsnowcurrently commented Dec 30, 2016

    Here's the updated patch.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Dec 31, 2016

    The concern I have with using an unsigned value as the interpreter ID is that it's applying the "NULL means an error" idiom or the "false means an error" idiom to a non-pointer and non-boolean return type, whereas the common conventions for integer return values are:

    • 0 = success in CLI return codes
    • non-negative = success in int-based C APIs

    If we were to use int_fast32_t for IDs instead, then any negative value can indicate an error, and the main interpreter could be given ID 0 to better align with the threading.Thread naming scheme.

    Whether we hit runtime error at 2 billion subinterpreters or 4 billion subinterpreters in a single process isn't likely to make much difference to anyone, but choosing an idiosyncratic error indicator will impact everyone that attempts to interact with the API.

    @zooba
    Copy link
    Member

    @zooba zooba commented Dec 31, 2016

    I fully expect subinterpreters to have a serious role in long running applications like web servers or other agents (e.g. cluster nodes), so I'd rather just bite the bullet and take 64-bits now so that we can completely neglect reuse issues. Otherwise we'll find ourselves adding infrastructure to hide the fact that you may see the same id twice.

    Another four bytes is a cheap way to avoid an entire abstraction layer.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Dec 31, 2016

    Yeah, I'm also fine with using int_fast64_t for the subinterpreter count.

    The only thing I'm really advocating for strongly on that front is that I think it makes sense to sacrifice the sign bit in the ID field as an error indicator that provides a more idiomatic C API.

    @ericsnowcurrently
    Copy link
    Member Author

    @ericsnowcurrently ericsnowcurrently commented Dec 31, 2016

    int_fast64_t it is then. :) I vacillated between the options and went with the bigger space. However, you're right that following convention is worth it.

    @ericsnowcurrently
    Copy link
    Member Author

    @ericsnowcurrently ericsnowcurrently commented Dec 31, 2016

    I've updated the patch to address Nick's review. Thanks!

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Dec 31, 2016

    I would prefer to not use "fast" C types because they are not well supported. For example, ctypes has ctypes.c_int64 but no ctypes.c_int_fast64.

    Previous work adding an unique identifier: PEP-509
    https://www.python.org/dev/peps/pep-0509/#integer-overflow

    @ericsnowcurrently
    Copy link
    Member Author

    @ericsnowcurrently ericsnowcurrently commented Dec 31, 2016

    Thanks for pointing that out, Victor. Given the precedent I switched to using int64_t. The patch actually uses PY_INT64_T, but I didn't see a reason to use int64_t directly. FWIW, there *are* a few places that use int_fast64_t, but they are rather specialized and I didn't want this patch to be a place where I had to deal with setting a more general precedent. :)

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented May 17, 2017

    What the status of this issue Eric? Do you still need interpreter ID?

    @ericsnowcurrently
    Copy link
    Member Author

    @ericsnowcurrently ericsnowcurrently commented May 21, 2017

    Yes, I still need it. :)

    @ericsnowcurrently
    Copy link
    Member Author

    @ericsnowcurrently ericsnowcurrently commented May 23, 2017

    New changeset e377416 by Eric Snow in branch 'master':
    bpo-29102: Add a unique ID to PyInterpreterState. (bpo-1639)
    e377416

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented May 23, 2017

    This change added a compiler warning.

    ./Programs/_testembed.c: In function ‘print_subinterp’:
    ./Programs/_testembed.c:31:22: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 2 has type ‘int64_t {aka long long int}’ [-Wformat=]
    printf("interp %lu <0x%" PRIXPTR ">, thread state <0x%" PRIXPTR ">: ",
    ^

    @ericsnowcurrently
    Copy link
    Member Author

    @ericsnowcurrently ericsnowcurrently commented May 24, 2017

    Thanks for pointing this out, Serhiy. I'll take a look in the morning.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented May 24, 2017

    Does someone know the PRxxx constant for int64_t?

    @ericsnowcurrently
    Copy link
    Member Author

    @ericsnowcurrently ericsnowcurrently commented May 24, 2017

    Apparently it is PRId64.

    @ericsnowcurrently
    Copy link
    Member Author

    @ericsnowcurrently ericsnowcurrently commented May 24, 2017

    (see bpo-30447)

    @ericsnowcurrently
    Copy link
    Member Author

    @ericsnowcurrently ericsnowcurrently commented May 25, 2017

    I've fixed the compiler warning via d1c3c13.

    @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
    3.7 interpreter-core type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants