search

avrae / d20

A fast, powerful, and extensible dice engine for D&D, d20 systems, and any other system that needs dice!
MIT License
122 stars 27 forks source link

Use `random.randrange(10) * 10` for percentile dice #6

Closed rndr closed 3 years ago

rndr commented 3 years ago

Summary

In the following benchmark random.randrange(10) * 10 seems to be about 30% faster in python3.8 on my machine:

import timeit

if __name__ == '__main__':
    for line in [
        'randrange(0, 100, 10)',
        'randrange(10) * 10',
        'randint(0, 9) * 10',
    ]:
        print(line, min(timeit.repeat(line, 'from random import randrange, randint')))

Checklist

PR Type

posita commented 3 years ago

Apologies for butting in with my $0.02 (which is probably worth what you paid for it), but I'm curious if this is performance bottleneck in practice. I can indeed confirm an improvement locally:

In [1]: from random import randrange, randint

In [2]: %timeit randrange(0, 100, 10)
955 ns ± 12.5 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [3]: %timeit randrange(10) * 10
588 ns ± 14.2 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [4]: %timeit randint(0, 9) * 10
920 ns ± 15.2 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

On my machine, this would be shaving less-than-half-a-µsec on d% generation. My guess is that the parser takes orders of magnitude more time than this, so I wonder how compelling this is as a performance improvement.

I could see a readability argument being made that randrange(10) * 10 better highlights that this generates random values among [0, 10, 20, …, 90]. I think that's debatable, though.

rndr commented 3 years ago

I have no doubt that the performance win is extremely small. The d% is also probably not used very often too (at least in Avrae and compared to dNUMBER), so it's improvement is probably not gonna bring much. I just though it might be of interest/concern from looking at the comment on the dNUMBER generation line:

n = Literal(random.randrange(self.size) + 1) # 200ns faster than randint(1, self._size)

I think at this point i'll close this down.

posita commented 3 years ago

Yeah, I noticed that comment too. 😜 Sorry to butt in!

zhudotexe commented 3 years ago

Whoops, I completely missed the PR until today's comment notifications - LGTM if you'd like to reopen it (for future PRs, the best way to make sure I don't miss it is by assigning me as a reviewer)

rndr commented 3 years ago

I deleted the branch and repo fork quiet a while back so github doesn't give me the option to reopen the PR (even if i make a new fork). I'm good with you copy-pasting the 1 line change and committing it into master if you wanna do that.