Skip to content

[subinterpreters] Improve Interpreter Isolation for Uses of PyArg_Parser #95909

Description

@ericsnowcurrently

(This is a follow-up to gh-90928.)

Argument clinic adds a static local PyArg_Parser for each function it generates. Each of those variables is initialized at runtime the first time it is used (even though, technically, it could be done statically). That means there's a (very unlikely) race on initializing each of those variables, at least once interpreters stop sharing the GIL. Furthermore, the PyArg_Parser.kwtuple field holds a PyObject *, which means we're leaking objects between interpreters (albeit completely immutable ones), and there still can be a race on refcounts if we don't do something about it. (For core cases, incl. builtin modules, we statically initialize kwtuple, but not for extension modules.)

For the sake of isolation (and of a per-interpreter GIL) we should store PyArg_Parser.kwtuple on PyInterpreterState in the cases where we aren't statically initializing it. For per-interpreter GIL we should also add a global lock around initializing each static PyArg_Parser.

Activity

  1. kumaraditya303 commented on Aug 13, 2022

    @kumaraditya303
    Contributor

    We don't really need to make kwtuple per interpreter as it is both not needed and will make kwnames parsing slower as #95910 need a dict lookup to find the kwtuple.

    Here's my plan:

    To avoid a performance regression we want to avoid locking for the general case when the parser is already initialized and no dict lookups. Acquiring and releasing a lock is very slow compared to an atomic read as the latter it is implemented in the hardware. kwtuple is only used for parsing and is never exposed to the function so there is no reference counting contention across threads.

    • Make _PyArg_Parser.initialized an atomic integer and return early in parser_init if it is 1. No locking is needed here and no dict lookups. General case.
    • Use a global mutex when a parser is initialized to maintain thread safe initialization of the parser.

    With this we can avoid locking and releasing the global mutex for the general case and retain performance.

  2. added a commit that references this issue on Aug 16, 2022
  3. ericsnowcurrently commented on Aug 16, 2022

    @ericsnowcurrently
    MemberAuthor

    Thanks for the PR, @kumaraditya303.

  4. Repository owner moved this from In Progress to Done in Fancy CPython Boardon Aug 16, 2022
  5. Repository owner moved this from In Progress to Done in Subinterpreterson Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

type-featureA feature request or enhancement

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions