Skip to content

cache the Range for gen_ascii_chars#171

Closed
TheIronBorn wants to merge 1 commit into
rust-random:masterfrom
TheIronBorn:patch-1
Closed

cache the Range for gen_ascii_chars#171
TheIronBorn wants to merge 1 commit into
rust-random:masterfrom
TheIronBorn:patch-1

Conversation

@TheIronBorn

@TheIronBorn TheIronBorn commented Sep 15, 2017

Copy link
Copy Markdown
Contributor

could even use unsafe fn get_unchecked.

  • passes all tests

could even use `unsafe fn get_unchecked`.
@TheIronBorn TheIronBorn changed the title cache the range for gen_ascii_chars cache the Range for gen_ascii_chars Sep 15, 2017
@dhardy

dhardy commented Sep 16, 2017

Copy link
Copy Markdown
Member

Just an optimisation? This may get removed anyway (check my branch/PR).

@TheIronBorn

Copy link
Copy Markdown
Contributor Author

Will this function/feature be moved elsewhere? If not, I think archiving a more optimal version somewhere is still a good idea.

@dhardy

dhardy commented Sep 22, 2017

Copy link
Copy Markdown
Member

Sorry, @TheIronBorn. I'm not quite sure what will happen; I have an experimental refactor of quite a lot of rand, but obviously can't promise it will get merged.

You might be able to apply the same optimisation here. Note that TODO comment and the note in the refactor RFC; some more tweaks to this (e.g. other character classes) are possible (please open a PR or comment on the RFC if you have suggestions):

AsciiWordChar is currently an oddity, used in many tests but with no hard requirements on form or function. This could be renamed and augmented in line with Regex character classes.

@pitdicker

Copy link
Copy Markdown
Contributor

In dhardy#69 I have an other solution that does not need caching, and is about 4,5 times faster than the current version. And no unsafe indexing necessary: the range check needed to ensure a uniform range doubles as a bounds check.

@TheIronBorn Sorry for working on the same thing.

@dhardy

dhardy commented Dec 12, 2017

Copy link
Copy Markdown
Member

Great, lets go for the second version then.

@dhardy dhardy closed this Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants