classification
Title: Make random module PEP-384 compatible
Type: enhancement Stage: resolved
Components: Extension Modules Versions: Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: dino.viehland Nosy List: christian.heimes, dino.viehland, eric.snow, rhettinger, serhiy.storchaka, twouters, vstinner
Priority: normal Keywords: patch

Created on 2019-09-09 15:57 by dino.viehland, last changed 2020-03-10 14:15 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 15798 merged dino.viehland, 2019-09-09 15:58
PR 18897 merged vstinner, 2020-03-10 11:50
Messages (9)
msg351509 - (view) Author: Dino Viehland (dino.viehland) * (Python committer) Date: 2019-09-09 15:57
Make random module PEP-384 compatible
msg351548 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-09-09 19:18
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?
msg351574 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-09-10 06:08
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.
msg351627 - (view) Author: Dino Viehland (dino.viehland) * (Python committer) Date: 2019-09-10 12:31
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.
msg352276 - (view) Author: Thomas Wouters (twouters) * (Python committer) Date: 2019-09-13 10:12
New changeset 04f0bbfbedf8d2bb69b012f853de6648b1a9f27f by T. Wouters (Dino Viehland) in branch 'master':
bpo-38075: Port _randommodule.c to PEP-384 (GH-15798)
https://github.com/python/cpython/commit/04f0bbfbedf8d2bb69b012f853de6648b1a9f27f
msg352472 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-15 09:53
> bpo-38075: Port _randommodule.c to PEP-384 (GH-15798)

It seems like this change introduced a reference leak: bpo-38176.
msg352528 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-16 08:07
commit 09dc2c672f937cbe53300cb680fca1f9c78ff976
Author: Dino Viehland <dinoviehland@gmail.com>
Date:   Sun Sep 15 15:51:44 2019 +0100

    Fix missing dec ref (#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;
 }
msg363821 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-03-10 11:58
Commit 04f0bbfbedf8d2bb69b012f853de6648b1a9f27f introduced a regression: bpo-39918.

I proposed PR 18897 to fix it.
msg363823 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-03-10 14:15
New changeset 00d7cd8ab8db2c1e1f591ade828f88a1a973d70f by Victor Stinner in branch 'master':
bpo-38075: Fix random_seed(): use PyObject_CallOneArg() (GH-18897)
https://github.com/python/cpython/commit/00d7cd8ab8db2c1e1f591ade828f88a1a973d70f
History
Date User Action Args
2020-03-10 14:15:35vstinnersetmessages: + msg363823
2020-03-10 11:58:18vstinnersetmessages: + msg363821
2020-03-10 11:50:29vstinnersetpull_requests: + pull_request18255
2019-09-16 08:07:56vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg352528

stage: patch review -> resolved
2019-09-15 09:53:06vstinnersetnosy: + vstinner
messages: + msg352472
2019-09-13 10:12:30twouterssetnosy: + twouters
messages: + msg352276
2019-09-10 12:31:49dino.viehlandsetmessages: + msg351627
2019-09-10 11:35:19christian.heimessetstatus: closed -> open
nosy: + christian.heimes

resolution: fixed -> (no value)
stage: resolved -> patch review
2019-09-10 11:22:47christian.heimessetstatus: open -> closed
type: enhancement
resolution: fixed
stage: patch review -> resolved
2019-09-10 06:08:06serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg351574
2019-09-09 19:18:27rhettingersetnosy: + rhettinger
messages: + msg351548
2019-09-09 15:58:16dino.viehlandsetkeywords: + patch
stage: patch review
pull_requests: + pull_request15450
2019-09-09 15:57:58dino.viehlandcreate