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

Make random module PEP-384 compatible #82256

Closed
DinoV opened this issue Sep 9, 2019 · 9 comments
Closed

Make random module PEP-384 compatible #82256

DinoV opened this issue Sep 9, 2019 · 9 comments
Assignees
Labels
3.9 only security fixes extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@DinoV
Copy link
Contributor

DinoV commented Sep 9, 2019

BPO 38075
Nosy @Yhg1s, @rhettinger, @vstinner, @tiran, @DinoV, @ericsnowcurrently, @serhiy-storchaka
PRs
  • bpo-38075: Port _randommodule.c to PEP-384 #15798
  • bpo-38075: Fix random_seed(): use PyObject_CallOneArg() #18897
  • 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/DinoV'
    closed_at = <Date 2019-09-16.08:07:56.459>
    created_at = <Date 2019-09-09.15:57:58.797>
    labels = ['extension-modules', 'type-feature', '3.9']
    title = 'Make random module PEP-384 compatible'
    updated_at = <Date 2020-03-10.14:15:35.260>
    user = 'https://github.com/DinoV'

    bugs.python.org fields:

    activity = <Date 2020-03-10.14:15:35.260>
    actor = 'vstinner'
    assignee = 'dino.viehland'
    closed = True
    closed_date = <Date 2019-09-16.08:07:56.459>
    closer = 'vstinner'
    components = ['Extension Modules']
    creation = <Date 2019-09-09.15:57:58.797>
    creator = 'dino.viehland'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 38075
    keywords = ['patch']
    message_count = 9.0
    messages = ['351509', '351548', '351574', '351627', '352276', '352472', '352528', '363821', '363823']
    nosy_count = 7.0
    nosy_names = ['twouters', 'rhettinger', 'vstinner', 'christian.heimes', 'dino.viehland', 'eric.snow', 'serhiy.storchaka']
    pr_nums = ['15798', '18897']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue38075'
    versions = ['Python 3.9']

    @DinoV
    Copy link
    Contributor Author

    DinoV commented Sep 9, 2019

    Make random module PEP-384 compatible

    @DinoV DinoV added the 3.9 only security fixes label Sep 9, 2019
    @DinoV DinoV self-assigned this Sep 9, 2019
    @DinoV DinoV added the extension-modules C modules in the Modules dir label Sep 9, 2019
    @rhettinger
    Copy link
    Contributor

    I'm curious what this does for us. _randommodule.c isn't public. Internally, we get to use our full ABI, not just the stable public ABI.

    I'm unclear on why this needs to change at all. Is code like this deemed broken in some way?

    @serhiy-storchaka
    Copy link
    Member

    I have the same questions. The new code looks more complex and cumbersome. What is the benefit of all these changes?

    I see you have created many similar issues. Would be nice to discuss first on the Python-Dev list what they do and why they are needed.

    @tiran tiran closed this as completed Sep 10, 2019
    @tiran tiran added the type-feature A feature request or enhancement label Sep 10, 2019
    @tiran tiran reopened this Sep 10, 2019
    @DinoV
    Copy link
    Contributor Author

    DinoV commented Sep 10, 2019

    There's several different reasons why these changes are beneficial. It'll help make the modules more compatible with PEP-554 in the future, enable more code re-use across Python VMs, reduce code maintenance going forward, and may also help with performance oriented changes in the future.

    On the PEP-554 front it is necessary to remove the static state that is used in the modules as we can't have sharing across the interpreters. The modules that are loaded into the different interpreter will need to not share any state on so all of their state needs to be per-instance state (there's also just an argument that static global state is just kind of ugly).

    On the re-use across VMs front it'll mean that alternative VMs will only need to stick to the stable API and well be able to use these modules as-is without having to re-implement them. It may not even be strictly necessary that these modules stick to 100% of the stable API if the places where they go outside dramatically help the implementation (for example PR 15805 uses _PyBytesWriter still).

    Sticking to the stable API will also naturally make it somewhat easier to make changes that impact the non-stable API. By it's very nature the stable API is going to change less (theoretically never) and therefore these modules will be less impacted by updates which are attempting to improve performance on the core. One example of that is the recent vector call support where many of the types needed to be updated (although trivially).

    So the final, and probably most speculative area, is the possibility of unlocking performance in the core runtime per Victor's stable API work: https://pythoncapi.readthedocs.io/. By having less dependencies on the core implementation details it should be easier to evolve those going forward an making it easier to unlock future performance gains in the core.

    @Yhg1s
    Copy link
    Member

    Yhg1s commented Sep 13, 2019

    New changeset 04f0bbf by T. Wouters (Dino Viehland) in branch 'master':
    bpo-38075: Port _randommodule.c to PEP-384 (GH-15798)
    04f0bbf

    @vstinner
    Copy link
    Member

    bpo-38075: Port _randommodule.c to PEP-384 (GH-15798)

    It seems like this change introduced a reference leak: bpo-38176.

    @vstinner
    Copy link
    Member

    commit 09dc2c6
    Author: Dino Viehland <dinoviehland@gmail.com>
    Date: Sun Sep 15 15:51:44 2019 +0100

    Fix missing dec ref (bpo-16158)
    
    diff --git a/Modules/_randommodule.c b/Modules/_randommodule.c
    index 8b0a0244bf..1ea2bf28ab 100644
    --- a/Modules/_randommodule.c
    +++ b/Modules/_randommodule.c
    @@ -572,6 +572,7 @@ static int
     _random_clear(PyObject *module)
     {
         Py_CLEAR(_randomstate(module)->Random_Type);
    +    Py_CLEAR(_randomstate(module)->Long___abs__);
         return 0;
     }

    @vstinner
    Copy link
    Member

    Commit 04f0bbf introduced a regression: bpo-39918.

    I proposed PR 18897 to fix it.

    @vstinner
    Copy link
    Member

    New changeset 00d7cd8 by Victor Stinner in branch 'master':
    bpo-38075: Fix random_seed(): use PyObject_CallOneArg() (GH-18897)
    00d7cd8

    @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.9 only security fixes extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants