Description

arhadthedev

opened on Jul 2, 2022

Issue body actions

Currently the function is parsed with the following behemoth:

if (!PyArg_ParseTuple(
            args, "OOpO!OOiiiiiiiiii" _Py_PARSE_PID "OOOiOp:fork_exec",
            &process_args, &executable_list,
            &close_fds, &PyTuple_Type, &py_fds_to_keep,
            &cwd_obj, &env_list,
            &p2cread, &p2cwrite, &c2pread, &c2pwrite,
            &errread, &errwrite, &errpipe_read, &errpipe_write,
            &restore_signals, &call_setsid, &pgid_to_set,
            &gid_object, &groups_list, &uid_object, &child_umask,
            &preexec_fn, &allow_vfork))
        return NULL;

Conversion will:

• hide this from a realm of manual and error-prone labor into a precise and checked world of automation
• allow to use faster methods like METH_FASTCALL+_PyArg_CheckPositional.

Linked PRs

• gh-94518: [_posixsubprocess] Replace variable validity flags with reserved values #94687
• gh-94518: Port 23-argument _posixsubprocess.fork_exec to Argument Clinic #94519
• gh-94518: Rename group* to extra_group* to avoid confusion #101054

Activity

1. arhadthedev

   added

   type-featureA feature request or enhancementA feature request or enhancement

   on Jul 2, 2022
2. gpshead commented on Jul 3, 2022

   

   gpshead

   on Jul 3, 2022

   Member

   More actions

   Indeed, one reason I've left it a behemoth in the past is that it only has 2-3 call-sites and gained parameters slowly over time. Cleaning this monster up will be good.

   One minor concern with argument clinic on this is the C stack space use increase due to the intermediate function calling the implementation with everything as C arguments on the stack. BUT... there is some other long overdue _posixsubprocess refactoring to be done that could alleviate stack use elsewhere (not to be done as part of this issue) in 3.12 that could make up for this change by reducing a similar amount of C stack use. (the internal C call to the child fork exec function which is also a giant messy pile of C args)

   Also, the argument clinic generated code is effectively a tail call to the _impl. Ideally compilers see that in optimized builds and effectively inline/merge the two functions instead of wasting time separating them with a calling convention sucking up stack. (No guarantees on that)

   If we went an alternate route and constructed a structure with all of the values instead of a giant list of arguments (a namedtuple could make it an easy transition), the C stack overhead from argument clinic would be gone. But so would the nice value type checking boilerplate that argument clinic handles for us. Unless we created our own internal to _posixsubprocess type instead of namedtuple and used argument clinic on its construction and data fill-in APIs instead. That idea could just be overcomplicated. I'll ponder it.

   In the meantime, thanks for the PR. I'll get to a review on that soon.

   Reacted by Erlend E. Aasland and Oleg Iarygin
3. gpshead

   added

   3.12only security fixesonly security fixes

   on Jul 3, 2022
4. AlexWaygood

   added

   topic-argument-clinic

   on Jul 3, 2022
5. gpshead

   added a commit that references this issue on Jan 14, 2023

   gh-94518: [_posixsubprocess] Replace variable validity flags with res…

   124af17
6. gpshead

   added a commit that references this issue on Jan 26, 2023

   gh-94518: Rename `group*` to `extra_group*` to avoid confusion (#101054)

   73245d0
7. mdboom

   added a commit that references this issue on Jan 31, 2023

   pythongh-94518: Rename `group*` to `extra_group*` to avoid confusion (p…

   add6967
8. gpshead

   added a commit that references this issue on Apr 24, 2023

   gh-94518: Port 23-argument `_posixsubprocess.fork_exec` to Argument C…

   dfc5c41
9. gpshead commented on Apr 24, 2023

   

   gpshead

   on Apr 24, 2023

   Member

   More actions

   thanks! :)

   Reacted by Oleg Iarygin
10. gpshead

    closed this as completedon Apr 24, 2023
11. carljm

    added 2 commits that reference this issue on Apr 24, 2023

    Merge branch 'main' into superopt

    19b8025

    Merge branch 'superopt' into superopt_spec

    1972ca4

Issue actions