[issue33144] random._randbelow optimization

Change by Serhiy Storchaka : -- resolution: -> fixed stage: patch review -> resolved status: open -> closed ___ Python tracker ___ _

[issue33144] random._randbelow optimization

Serhiy Storchaka added the comment: New changeset ec1622d56c80d15740f7f8459c9a79fd55f5d3c7 by Serhiy Storchaka in branch 'master': bpo-33144: Fix choosing random.Random._randbelow implementation. (GH-6563) https://github.com/python/cpython/commit/ec1622d56c80d15740f7f8459c9a79fd55f5d3c7

[issue33144] random._randbelow optimization

Change by Raymond Hettinger : -- assignee: rhettinger -> serhiy.storchaka ___ Python tracker ___ ___ Python-bugs-list mailing list Un

[issue33144] random._randbelow optimization

Serhiy Storchaka added the comment: PR 6291 didn't work properly with case 1. Rand2 uses getrandbits() since it is overridden in the parent despites the fact that random() is defined later. PR 6563 fixes this. It walks classes in method resolution order and finds the first class that defines

[issue33144] random._randbelow optimization

Change by Serhiy Storchaka : -- pull_requests: +6258 ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://m

[issue33144] random._randbelow optimization

Change by Raymond Hettinger : -- resolution: -> fixed stage: patch review -> resolved status: open -> closed ___ Python tracker ___

[issue33144] random._randbelow optimization

Raymond Hettinger added the comment: Possibly, the switch from type checks to identity checks could be considered a bugfix that could be backported. I've always had a lingering worry about that part of the code. -- ___ Python tracker

[issue33144] random._randbelow optimization

Raymond Hettinger added the comment: New changeset ba3a87aca37cec5b1ee32cf68f4a254fa0bb2bec by Raymond Hettinger (Wolfgang Maier) in branch 'master': bpo-33144: random.Random and subclasses: split _randbelow implementation (GH-6291) https://github.com/python/cpython/commit/ba3a87aca37cec5b1ee

[issue33144] random._randbelow optimization

Wolfgang Maier added the comment: ok, I've created issue 33203 to deal with raising ValueError in _randbelow consistently. -- ___ Python tracker ___ __

[issue33144] random._randbelow optimization

Wolfgang Maier added the comment: In addition, I took the opportunity to fix a bug in the original _randbelow in that it would only raise the advertised ValueError on n=0 in the getrandbits-dependent branch, but ZeroDivisionError in the pure random branch. --

[issue33144] random._randbelow optimization

Wolfgang Maier added the comment: So, the PR implements the behaviour suggested by Serhiy as his cases 1 and 2. Case 2 changes *existing* behaviour because before it was sufficient to have a user-defined getrandbits anywhere in the inheritance tree, while with the PR it has to be more recent (

[issue33144] random._randbelow optimization

Change by Wolfgang Maier : -- pull_requests: +6015 stage: -> patch review ___ Python tracker ___ ___ Python-bugs-list mailing list U

[issue33144] random._randbelow optimization

Wolfgang Maier added the comment: Thanks, Raymond. I'll do that once I've addressed Serhiy's points. -- ___ Python tracker ___ ___ P

[issue33144] random._randbelow optimization

Raymond Hettinger added the comment: Wolfgang, can you submit this as a PR. -- ___ Python tracker ___ ___ Python-bugs-list mailing l

[issue33144] random._randbelow optimization

Wolfgang Maier added the comment: Serhiy: > I like the idea in general, but have comments about the implementation. > > __init_subclass__ should take **kwargs and pass it to > super().__init_subclass__(). type(cls.random) is not the same as > type(self.random). I would use the condition `cls

[issue33144] random._randbelow optimization

Serhiy Storchaka added the comment: I think this is excellent application of __init_subclass__. It is common to patch an instance method in __init__, but this can create a reference loop if patch it by other instance method. In this case the choice doesn't depend on arguments of __init__, and

[issue33144] random._randbelow optimization

Tim Peters added the comment: I'm the wrong guy to ask about that. Since I worked at Zope Corp, my natural inclination is to monkey-patch everything - but knowing full well that will offend everyone else ;-) That said, this optimization seems straightforward to me: two distinct method impl

[issue33144] random._randbelow optimization

Raymond Hettinger added the comment: > the optimized `_randbelow()` also avoids populating its locals > with 5 unused formal arguments Yes, that clean-up would be nice as well :-) Any thoughts on having __init__ set a flag versus using __init__subclass__ to backpatch the subclass? To me, th

[issue33144] random._randbelow optimization

Tim Peters added the comment: I don't see anything objectionable about the class optimizing the implementation of a private method. I'll note that there's a speed benefit beyond just removing the two type checks in the common case: the optimized `_randbelow()` also avoids populating its loc

[issue33144] random._randbelow optimization

Raymond Hettinger added the comment: > it could also be done at instantiation time (the attached patch > uses __init_subclass__ for that purpose FWIW, a 10-25% speedup is only possible because the remaining code is already somewhat fast. All that is being proposed is removing couple of lines

[issue33144] random._randbelow optimization

New submission from Wolfgang Maier : Given that the random module goes a long way to ensure optimal performance, I was wondering why the check for a match between the random and getrandbits methods is performed per call of Random._randbelow, when it could also be done at instantiation time (th