Skip to content

Conversation

@sl-solution
Copy link

For the Beta distribution, the compiler cannot infer the type of the output, so this causes unnecessary memory allocation. Defining the sampler at the argument level of the function solve the problem.

For the Beta distribution, the compiler cannot infer the type of the output, so this causes unnecessary memory allocation. Defining the sampler at the argument level of the function solve the problem.
@codecov-io
Copy link

codecov-io commented Feb 14, 2021

Codecov Report

Merging #1281 (e9bd2cf) into master (0b1ecad) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1281      +/-   ##
==========================================
- Coverage   81.34%   81.28%   -0.07%     
==========================================
  Files         117      115       -2     
  Lines        6563     6529      -34     
==========================================
- Hits         5339     5307      -32     
+ Misses       1224     1222       -2     
Impacted Files Coverage Δ
src/univariates.jl 60.80% <100.00%> (-5.87%) ⬇️
src/common.jl 67.56% <0.00%> (-0.86%) ⬇️
src/multivariates.jl 70.66% <0.00%> (-0.77%) ⬇️
src/matrixvariates.jl 76.81% <0.00%> (-0.66%) ⬇️
src/quantilealgs.jl 80.95% <0.00%> (-0.23%) ⬇️
src/utils.jl 90.47% <0.00%> (-0.23%) ⬇️
src/samplers.jl
src/Distributions.jl
src/matrix/wishart.jl 88.67% <0.00%> (+0.82%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b1ecad...e9bd2cf. Read the comment docs.

@andreasnoack
Copy link
Member

I'm not sure why we should expose this as an argument. Could you provide more information about the issue that you are fixing here?

@sl-solution
Copy link
Author

Consider the following example:

`using Random, Distributions
x = zeros(1000)
@time rand!(Beta(0.1,1.0),x)
@time rand!(Beta(0.1,1.0),x)

0.000091 seconds (1.49 k allocations: 23.328 KiB)`

The 23.328 KiB allocations is undesirable. @code_typed rand!(Distributions.GLOBAL_RNG,sampler(Beta(0.1,1.0)),x) reveals that the compiler cannot infer the type of rand(rng, smp). By adding smp as an argument of the function, the inference of the type is done before the run of the actual body of the function.
I thought this should be the minimal way to fix this and avoid any side effect.

@andreasnoack
Copy link
Member

I see. I think it's better to change the loop to an in-place map like

map!(t -> rand(rng, smp), A, eachindex(A))

That gives a function barrier as well.

@sl-solution
Copy link
Author

Agree, however, I guess (generally) optimising for loops (e.g. exploiting AVX) is easier than map! (?)

@sl-solution
Copy link
Author

It seems that using loop require 2/3 of allocation compared to map!.

function fun(n, m)
           res = zeros(n)
           t_hold = zeros(m)
           for i in 1:n
               res[i] = mean(rand!(Beta(.1,1.0),t_hold))
           end
           res
       end
# loop
@benchmark fun(1000,100)
BenchmarkTools.Trial: 
  memory estimate:  102.56 KiB
  allocs estimate:  2002
  --------------
  minimum time:     4.500 ms (0.00% GC)
  median time:      4.773 ms (0.00% GC)
  mean time:        4.789 ms (0.12% GC)
  maximum time:     8.130 ms (40.31% GC)
  --------------
  samples:          1044
  evals/sample:     1

# map!
BenchmarkTools.Trial: 
  memory estimate:  149.44 KiB
  allocs estimate:  3002
  --------------
  minimum time:     4.607 ms (0.00% GC)
  median time:      4.897 ms (0.00% GC)
  mean time:        4.920 ms (0.11% GC)
  maximum time:     7.068 ms (27.09% GC)
  --------------
  samples:          1016
  evals/sample:     1

@devmotion
Copy link
Member

Can't we just introduce a proper function barrier instead of the somewhat weird keyword argument that one is not supposed to use anyway?

@devmotion
Copy link
Member

I opened a PR that introduces a function barrier without adding any keyword argument to rand!: #1350

@andreasnoack
Copy link
Member

Superseded by #1350

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.

4 participants