Allow types other than Int in randstring
#54402
Conversation
Fixes JuliaLang#54397 What remains is that the effectively allowed input still needs to be convertible to `Int`. So things like `typemax(Int128)` won't work, and neither will negative values.
| @@ -71,13 +71,14 @@ let b = UInt8['0':'9';'A':'Z';'a':'z'] | |||
| global randstring | |||
|
|
|||
| function randstring(r::AbstractRNG, chars=b, n::Integer=8) | |||
| _n = convert(Int, n) | |||
There was a problem hiding this comment.
I am not sure it is worth doing in this specific instance but presumably structuring it like:
f(x::Integer) = f(convert(Int, n))
f(x::Int) = ...would reduce compilation time since you would not compile the main function body for multiple integer types.
There was a problem hiding this comment.
For cases where lots of different types are passed to randstring, that's certainly the case, yeah. I'm not sure how common that would be though, there's lots of better ways to control the length of a generated string other than through the type.
There was a problem hiding this comment.
I think @KristofferC is just referring to the amount of code that is generated by using function barriers or:
https://docs.julialang.org/en/v1/manual/performance-tips/#kernel-functions
There was a problem hiding this comment.
Yes, I'm referring to the likelihood of hitting that & the additional compilation overhead being a significant slowdown. If this is a bottleneck, I'd first recommend switching to a different scheme for generating the length (with just Int) than optimize the compilation overhead.
There was a problem hiding this comment.
Right, and the problem is that this PR would allow someone to have a type unstable call site with various Integer types for length, rather than error as it currently does.
There was a problem hiding this comment.
Why would that inherently be a problem? We don't prevent type instabilities in user code in other places either.
There was a problem hiding this comment.
I think Krostoffer's convert approach is a good idea. Yes, it's ok to just allow a different integer type flow through but in this case Int covers the entire range of reasonable values and converting avoids additional compilation.
There was a problem hiding this comment.
There is no other integer type flowing through though; the code in this PR already calls _convert(Int, ..), just directly instead of in a function barrier. This function is a total of 11 lines long, I seriously doubt that this leads to a bottleneck in either compilation time or binary size. At that size, I'm willing to bet that the inlining pass costs more than potential dual compilation if anyone calls this with an UInt8..
There was a problem hiding this comment.
Ok, but every little bit counts. Why not just change it?
There was a problem hiding this comment.
Because I agree with the comment by Kristoffer that it's probably not worth doing in this case. Is it a problem that I agree with the reviewer? As it stands, this just seems like premature optimization of a hypothetical to me.
|
It is a problem because as this PR is currently written you will generate a
lot of redundant code. You should accept Kristoffer's suggestion for the
signatures as it will improve the quality of this PR.
…On Thu, May 9, 2024, 02:07 Sukera ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In stdlib/Random/src/misc.jl
<#54402 (comment)>:
> @@ -71,13 +71,14 @@ let b = UInt8['0':'9';'A':'Z';'a':'z']
global randstring
function randstring(r::AbstractRNG, chars=b, n::Integer=8)
+ _n = convert(Int, n)
Why would that inherently be a problem? We don't prevent type
instabilities in user code in other places either.
—
Reply to this email directly, view it on GitHub
<#54402 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAP5MDAUQXLRZEXJIYX554LZBMHALAVCNFSM6AAAAABHMSEKMSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANBXGIYTEMJQHA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Since Kristoffer mentioned that it's probably not worth it in this PR, I've chosen to keep the code simpler to follow without having to disentangle niche dispatch/compilation optimizations. |
|
Does this need anything else (cc @KristofferC)? |
Fixes #54397
What remains is that the effectively allowed input still needs to be convertible to
Intand non-negative. So things liketypemax(Int128)won't work.