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.

Sign up for GitHub

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

Performance regression of scalar randn() between Julia 1.4 and 1.5 #37234

Closed
wants to merge 3 commits into from
Closed

Performance regression of scalar randn() between Julia 1.4 and 1.5 #37234

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
45ec398
definition of randn for scalars reverted to 1.4
lungben Aug 27, 2020
c1f0061
Added comments why this change is done
lungben Aug 28, 2020
08104ed
Update normal.jl
KristofferC Sep 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion stdlib/Random/src/normal.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,26 @@ julia> randn(rng, ComplexF32, (2, 3))
0.611224+1.56403im 0.355204-0.365563im 0.0905552+1.31012im
```
"""
@inline randn(rng::AbstractRNG=default_rng()) = _randn(rng, rand(rng, UInt52Raw()))
@inline function randn(rng::AbstractRNG=default_rng())
#=
When defining
Copy link
Member

Choose a reason for hiding this comment

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

Trailing whitespace here.

`@inline randn(rng::AbstractRNG=default_rng()) = _randn(rng, rand(rng, UInt52Raw()))`
the function call to `_randn` is currently not inlined, resulting in slightly worse
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering whether it's really _randn which fails to be inlined... Looking rapidly at @code_typed randn(MersenneTwister()) doesn't show a call to _randn if I'm not mistaken. How did you conclude that randn_ is not inlined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deduced it from the @code_llvm (but I might be wrong here - I do not have much experience with these topics):
In the Julia 1.5 version, there is the following call:
%30 = call double @j__randn_3665(%jl_value_t* nonnull %0, i64 %29) #0
In the Julia 1.4 code, this call is missing, but there are ca. 60 additional lines of LLVM code. I suppose this is the inlined function, but am not sure about it.

Copy link
Member

Choose a reason for hiding this comment

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

Since _randn is defined as calling randn, the inliner decides to stop. This is reflected in code_llvm, but due to the particular way that code_typed works and is cached, it won't be represented there.

Copy link
Member

Choose a reason for hiding this comment

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

To fix this, I would suggest moving this code duplication into randn_unlikely instead, so that's the only recursive function and not all these others

Copy link
Member

Choose a reason for hiding this comment

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

randn_unlikely is the noinline slow path though. If we moved the code we'd have to inline everything?

performance for scalar random normal numbers than repeating the code of `_randn`
inside the following function.
=#
@inbounds begin
r = rand(rng, UInt52Raw())

Copy link
Member

Choose a reason for hiding this comment

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

and here

# the following code is identical to the one in `_randn(rng::AbstractRNG, r::UInt64)`
r &= 0x000fffffffffffff
rabs = Int64(r>>1) # One bit for the sign
idx = rabs & 0xFF
x = ifelse(r % Bool, -rabs, rabs)*wi[idx+1]
rabs < ki[idx+1] && return x # 99.3% of the time we return here 1st try
return randn_unlikely(rng, idx, rabs, x)
end
end

@inline function _randn(rng::AbstractRNG, r::UInt64)
@inbounds begin
Expand Down