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

bpo-35094: Improved algorithms for random.sample #10192

Closed
wants to merge 3 commits into from

Conversation

ciphergoth
Copy link

@ciphergoth ciphergoth commented Oct 28, 2018

Current algorithms for random.sample allocate considerable auxiliary memory to track what's been used so far; with this pull request we sample in a maximally memory efficient way. Peformance is similar or in some cases faster. See also https://github.com/ciphergoth/sansreplace

https://bugs.python.org/issue35094

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

Lib/random.py Outdated
if is_set or k*2 >= n:
for i, item in enumerate(population):
r = randbelow(i + 1)
if r < k:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can reduce one indentation level with an early continue:

if r >= k:
   continue
if i < k:
    result[i] = result[r]
result[r] = item

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed - thanks!

@ciphergoth
Copy link
Author

BTW CLA signed at 10:52 Pacific time this morning, just waiting for the systems to catch up!

@rhettinger rhettinger closed this Oct 28, 2018
@rhettinger
Copy link
Contributor

rhettinger commented Aug 24, 2019

For future reference, here are my notes from evaluating the PR:

Call count summary

k Baseline Patched
10 10 20
20 20 40
100 100 200
200 200 400
1,000 1,000 2,000
2,000 2,000 4,000
5,001 5,001 10,000
7,500 5,001 10,000
8,500 8,500 10,000
9,500 9,500 10,000

Test Code

from unittest.mock import Mock
from random import Random
import sys

prng = Random()
prng._randbelow = Mock(wraps=prng._randbelow)

n, k = map(int, sys.argv[1:3])
prng.sample(range(n), k)
rbc = prng._randbelow.call_count
print(f'randbelow_calls={rbc} <-- n={n} k={k}')

Results with the PR Applied

$ python3.8 randombelow_counter.py 10000 10
randbelow_calls=20 <-- n=10000 k=10
$ python3.8 randombelow_counter.py 10000 20
randbelow_calls=40 <-- n=10000 k=20
$ python3.8 randombelow_counter.py 10000 100
randbelow_calls=200 <-- n=10000 k=100
$ python3.8 randombelow_counter.py 10000 200
randbelow_calls=400 <-- n=10000 k=200
$ python3.8 randombelow_counter.py 10000 1000
randbelow_calls=2000 <-- n=10000 k=1000
$ python3.8 randombelow_counter.py 10000 2000
randbelow_calls=4000 <-- n=10000 k=2000
$ python3.8 randombelow_counter.py 10000 5001
randbelow_calls=10000 <-- n=10000 k=5001
$ python3.8 randombelow_counter.py 10000 7500
randbelow_calls=10000 <-- n=10000 k=7500
$ python3.8 randombelow_counter.py 10000 8500
randbelow_calls=10000 <-- n=10000 k=8500
$ python3.8 randombelow_counter.py 10000 9500
randbelow_calls=10000 <-- n=10000 k=9500

Results for the Baseline Version

$ python3.8 randombelow_counter.py 10000 10
randbelow_calls=10 <-- n=10000 k=10
$ python3.8 randombelow_counter.py 10000 20
randbelow_calls=20 <-- n=10000 k=20
$ python3.8 randombelow_counter.py 10000 100
randbelow_calls=100 <-- n=10000 k=100
$ python3.8 randombelow_counter.py 10000 200
randbelow_calls=201 <-- n=10000 k=200
$ python3.8 randombelow_counter.py 10000 1000
randbelow_calls=1055 <-- n=10000 k=1000
$ python3.8 randombelow_counter.py 10000 2000
randbelow_calls=2000 <-- n=10000 k=2000
$ python3.8 randombelow_counter.py 10000 5001
randbelow_calls=5001 <-- n=10000 k=5001
$ python3.8 randombelow_counter.py 10000 7500
randbelow_calls=7500 <-- n=10000 k=7500
$ python3.8 randombelow_counter.py 10000 8500
randbelow_calls=8500 <-- n=10000 k=8500
$ python3.8 randombelow_counter.py 10000 9500
randbelow_calls=9500 <-- n=10000 k=9500

@rhettinger
Copy link
Contributor

Also, here at the comparative timings with and without the patch:

With the PR Applied

$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=10)'
20000 loops, best of 11: 14.9 usec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=20)'
10000 loops, best of 11: 26.7 usec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=100)'
2000 loops, best of 11: 122 usec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=200)'
1000 loops, best of 11: 243 usec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=1_000)'
200 loops, best of 11: 1.27 msec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=2_000)'
100 loops, best of 11: 2.58 msec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=5_001)'
50 loops, best of 11: 5.56 msec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=7500)'
50 loops, best of 11: 5.7 msec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=8500)'
50 loops, best of 11: 5.78 msec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=9500)'
50 loops, best of 11: 5.83 msec per loop

Baseline

$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=10)'
20000 loops, best of 11: 10.6 usec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=20)'
20000 loops, best of 11: 17.9 usec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=100)'
5000 loops, best of 11: 74.9 usec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=200)'
2000 loops, best of 11: 144 usec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=1_000)'
500 loops, best of 11: 738 usec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=2_000)'
200 loops, best of 11: 1.63 msec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=5_001)'
100 loops, best of 11: 3.38 msec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=7500)'
50 loops, best of 11: 4.9 msec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=8500)'
50 loops, best of 11: 5.52 msec per loop
$ ./python.exe -m timeit -r11 -s 'from random import sample' 'sample(range(10_000), k=9500)'

@ciphergoth
Copy link
Author

Thanks for following up on this! Surprised by the numbers you're seeing - I'll try reproducing your tests and investigate further. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants