JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.31k stars 5.45k forks source link

For-Loop Type Instability in the Base Library #55063

Open kylincaster opened 1 month ago

kylincaster commented 1 month ago

I have found some for-loop code in the Base library that is not type-stable. For example, in Array.jl, the append! function at line 1319 with append!(a::AbstractVector, iter...) = (for v in iter; append!(a, v); end; return a) definition uses a for-loop rather than map or mapreduce to handle arguments with different types.

Here is an example with performance benchmarks for the type instability.

julia> x = (1, 1:3, 1, 1:3, 1, 1:3, 1, 1:3, 1, 1:3);

julia> function testFor(iters)
    _len = 0
    for v in iters
        _len += Base.length(v)
    end
    _len
end;

julia> testMap(iters) = mapreduce(Base.length, +, iters; init = 0);

julia> @benchmark testFor($x)
BenchmarkTools.Trial: 10000 samples with 792 evaluations.
 Range (min … max):  161.869 ns … 154.836 μs  ┊ GC (min … max):  0.00% … 99.69%
 Time  (median):     208.333 ns               ┊ GC (median):     0.00%
 Time  (mean ± σ):   274.003 ns ±   1.553 μs  ┊ GC (mean ± σ):  11.25% ± 11.02%

  162 ns        Histogram: log(frequency) by time        936 ns <

 Memory estimate: 1.28 KiB, allocs estimate: 14.

julia> @benchmark testMap($x)
BenchmarkTools.Trial: 10000 samples with 1000 evaluations.
 Range (min … max):  1.700 ns … 29.900 ns  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     1.800 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   1.864 ns ±  0.357 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

  1.7 ns       Histogram: log(frequency) by time      2.7 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

It appears that the for-loop code is only suitable for arguments with the same types:

julia> y = (1.3, 1.3, 1.3, 1.3, 1.3, 1.3, 1.3, 1.3, 1.3);

julia> @benchmark testFor($y)
BenchmarkTools.Trial: 10000 samples with 1000 evaluations.
 Range (min … max):  1.500 ns … 49.200 ns  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     1.600 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   1.617 ns ±  0.534 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

  1.5 ns       Histogram: log(frequency) by time      2.4 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

julia> @benchmark testMap($y)
BenchmarkTools.Trial: 10000 samples with 1000 evaluations.
 Range (min … max):  0.900 ns … 24.400 ns  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     0.900 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   1.064 ns ±  0.447 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

  0.9 ns         Histogram: frequency by time         1.5 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

Should we consider changing all similar code in the Base library to improve performance?

Thank you for your time and consideration.

nsajko commented 1 month ago

Yeah, this should call foreach instead of using a for loop. Mind creating a PR?

https://github.com/JuliaLang/julia/blob/23dabef1910604f8eb6669dfb3ee2324c0d00f61/base/array.jl#L1319