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

make randstring 25% faster for ASCII #44264

Merged
merged 1 commit into from
May 9, 2022
Merged

Conversation

rfourquet
Copy link
Member

@rfourquet rfourquet commented Feb 19, 2022

This uses _string_n instead of StringVector in order to allocate less,
e.g.:

julia> @btime randstring() # master
  71.499 ns (2 allocations: 96 bytes)
"DZk2V5Bm"

julia> @btime randstring() # PR
  57.832 ns (1 allocation: 32 bytes)
"Hj5PINXU"

@rfourquet rfourquet added performance Must go faster domain:randomness Random number generation and the Random stdlib labels Feb 19, 2022
@rfourquet rfourquet changed the title make randstring 25% faster make randstring 25% faster for ASCII Feb 19, 2022
@KristofferC
Copy link
Sponsor Member

Could use a rebase it seems.

Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Random.UnsafeView perhaps really should have been deprecated a long time ago.

This uses `_string_n` instead of `StringVector` in order to allocate less,
e.g.:
```
julia> @Btime randstring() # master
  71.499 ns (2 allocations: 96 bytes)
"DZk2V5Bm"

julia> @Btime randstring() # PR
  57.832 ns (1 allocation: 32 bytes)
"Hj5PINXU"
```
@rfourquet
Copy link
Member Author

rfourquet commented Feb 26, 2022

Random.UnsafeView perhaps really should have been deprecated a long time ago.

There is an issue for that: #36718
But this PR is an example where it still proves to be useful, unless I'm missing another way. As mentioned in the issue though, we could definitely try to make UnsafeView hold a reference to the parent and see if performance doesn't get worse (although that would mean compiling more methods, for each parent type, instead of for each eltype).

Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

This looks good to me. I think that qualms about UnsafeView are valid but don't block this PR.

@KristofferC KristofferC merged commit ff45fdc into master May 9, 2022
@KristofferC KristofferC deleted the rf/faster-randstring branch May 9, 2022 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:randomness Random number generation and the Random stdlib performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants