bpo-33144: random.Random and subclasses: split _randbelow implementation #6291

wm75 · 2018-03-28T14:49:05Z

Splits the getrandbits-dependent and -independent branches of
random.Random._randbelow into separate methods and selects the
implementation to be used by Random and its subclasses at class
creation time for increased performance.

https://bugs.python.org/issue33144

serhiy-storchaka · 2018-03-28T17:07:46Z

Lib/random.py

+    def __init_subclass__(cls, **kwargs):
+        # Only call self.getrandbits if the original random() builtin method
+        # has not been overridden or if a new getrandbits() was supplied.
+        if type(cls.__dict__.get('getrandbits')) is _FunctionType:


Why not just 'getrandbits' in cls.__dict__?

to preserve the existing behavior as much as possible; if getrandbits gets overridden with just an attribute instead of a method, the fallback mechanism stays activated

We already change the behavior by allowing getrandbits() to win even if random() is overridden in a parent. I think it would be better to allow overriding getrandbits() not only with Python functions.

meaning that when you override it with something that isn't callable _randbelow will fail with an exception; it doesn't do that now, but I agree it may actually be the right thing to do because such a situation would probably only indicate a mistake.

serhiy-storchaka · 2018-03-28T17:10:05Z

Lib/random.py

+            # so we can only use random() from here.
+            cls._randbelow = cls._randbelow_without_getrandbits
+        else:
+            cls._randbelow = getattr(cls, cls._randbelow.__name__)


Is this needed?

This is for the exotic case that a subclass overrides one of the private _randbelow implementations without providing random or getrandbits methods. Even though that would be considered a hack, inheritance mechanisms should still work and _randbelow_with_getrandbits or _randbelow_without_getrandbits get replaced. The test with Subclass5 is for this situation.

It deserves a comment in the source though. I'll add it.

_randbelow is an implementation detail. It isn't designed for overriding. The code will be simpler if don't support this hack.

I still think that one extra line (I'm not really counting the else because if/elif without else looks questionable anyway) is worth it. Hack or not, our class-level manipulation of things should not make inheritance unpredictable.

Serhiy is correct in pointing out the _randbelow() is an implementation detail. We really don't want people using (overriding) this non public api.

Let's not feature-creep this optimization patch. Ideally, it should be the smallest possible change that moves algorithmic decision to class instantiation time.

Also, any possible "bug fix" to change the exception raised should be in a separate PR. Unless we've guaranteed a particular exception (which is not usually the case), the C code is allowed to differ in small ways from the Python code. That isn't to say that it shouldn't be changes, but that it is a separate discussion.

ok: separate PR #6338 and corresponding https://bugs.python.org/issue33203 for the ValueError fix. Once that's settled I'll rebase this PR.

Speaking of not "feature-creeping" this simple optimization: would you prefer this patch then to retain the old behaviour that as soon as getrandbits was overridden once in a subclass, overriding random later again did not have an effect anymore on _randbelow (see Serhiy's Case 2 in https://bugs.python.org/msg314534 )?

serhiy-storchaka · 2018-03-28T17:27:45Z

Lib/test/test_random.py

+                random.Random.__init__(self)
+        Subclass(newarg=1)
+
+    def test_overriding_random(self):


This test uses implementation details. It is possible to test this behavior using only public API. Make random() and getrandbits() in subclasses logging their calls, and check what methods were used when call randrange(). This will make the test valid even if the implementation be changed.

Good idea! I'll look into it.

But maybe we need to keep a simple white-box test for random.Random._randbelow and a subclass that don't override random or getrandbits, because there is no way to test this as a black box. It is worth to mark this test as CPython-only.

Sounds reasonable: I'm going to work on all the changes tomorrow, thanks.

rhettinger

Please add a blurb entry and resolve the conflict caused by the separate patch for the ZeroDivisionError.

bedevere-bot · 2018-04-05T18:25:02Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Splits the getrandbits-dependent and -independent branches of random.Random._randbelow into separate methods and selects the implementation to be used by Random and its subclasses at class creation time for increased performance.

wm75 · 2018-04-10T13:12:35Z

I have made the requested changes; please review again.

bedevere-bot · 2018-04-10T13:12:37Z

Thanks for making the requested changes!

@rhettinger: please review the changes made to this pull request.

rhettinger · 2018-04-17T14:37:20Z

Lib/random.py

@@ -221,22 +242,23 @@ def randint(self, a, b):

        return self.randrange(a, b+1)

-    def _randbelow(self, n, int=int, maxsize=1<<BPF, type=type,
-                   Method=_MethodType, BuiltinMethod=_BuiltinMethodType):
+    def _randbelow_with_getrandbits(self, n):


Why not just call this _randbelow and only patch the without getrandbits case?

You need to keep a reference to the "with getrandbits" implementation to be able to return to using it in the case:

class Rand1(Random): def random(self): ... # _randbelow should use random() class Rand2(Rand1): def getrandbits(self): ... # _randbelow should use getrandbits() now again

Okay, I see what you're trying to do.

serhiy-storchaka · 2018-04-17T15:23:06Z

Lib/random.py

+        ranges.
+        """
+
+        if (cls.random is _random.Random.random) or (


I would write this as:

if 'getrandbits' in cls.__dict__: cls._randbelow = cls._randbelow_with_getrandbits elif 'random' in cls.__dict__: cls._randbelow = cls._randbelow_without_getrandbits #else inherits from the parent

serhiy-storchaka · 2018-04-17T15:25:30Z

Lib/test/test_random.py

+                return super().random()
+
+            def getrandbits(self, n):
+                logging.getLogger('getrandbits').info('used getrandbits')


Using logging for testing looks very... strange. You could just set a nonlocal variable.

wm75 requested a review from rhettinger as a code owner March 28, 2018 14:49

the-knights-who-say-ni added the CLA signed label Mar 28, 2018

bedevere-bot added the awaiting review label Mar 28, 2018

serhiy-storchaka reviewed Mar 28, 2018

View reviewed changes

serhiy-storchaka added the performance Performance or resource usage label Mar 28, 2018

wm75 mentioned this pull request Apr 5, 2018

bpo-33228: Use Random.choices in tempfile #6383

Closed

rhettinger requested changes Apr 5, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Apr 5, 2018

wm75 force-pushed the random-improve branch from 2f77a64 to cc1a10c Compare April 9, 2018 16:30

Wolfgang Maier added 3 commits April 9, 2018 18:41

Fix rebase artefact

270fec1

Simplify subclassing logic and improve tests

fba740b

Add NEWS blurb

83aab0d

bedevere-bot removed the awaiting changes label Apr 10, 2018

bedevere-bot added the awaiting change review label Apr 10, 2018

rhettinger reviewed Apr 17, 2018

View reviewed changes

rhettinger approved these changes Apr 17, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Apr 17, 2018

rhettinger merged commit ba3a87a into python:master Apr 17, 2018

bedevere-bot removed the awaiting merge label Apr 17, 2018

serhiy-storchaka reviewed Apr 17, 2018

View reviewed changes

Uh oh!

bpo-33144: random.Random and subclasses: split _randbelow implementation #6291

bpo-33144: random.Random and subclasses: split _randbelow implementation #6291

Uh oh!

Conversation

wm75 commented Mar 28, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wm75 Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhettinger left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Apr 5, 2018

Uh oh!

wm75 commented Apr 10, 2018

Uh oh!

bedevere-bot commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wm75 commented Mar 28, 2018 •

edited by bedevere-bot

Loading

wm75 Mar 28, 2018 •

edited

Loading