Skip to content

Conversation

blegat
Copy link
Member

@blegat blegat commented Sep 23, 2023

Alternative fix to #2285

The for loop is quite weird as you expect the list of arguments to be a tuple of mixed argument types.
I think there is a win of not doing the recursion and falling back to a for loop at some point but I think it's at least more readable to use the standard afold for this.

This shows a big win on @odow's benchmark in #2284 (comment)

julia> import MathOptInterface as MOI

julia> using BenchmarkTools

julia> function my_operate(::typeof(+), ::Type{T}, f, g, h, args...) where {T<:Number}
           ret = MOI.Utilities.operate(+, T, f, g)
           ret = MOI.Utilities.operate!(+, T, ret, h)
           for a in args
               ret = MOI.Utilities.operate!(+, T, ret, a)
           end
           return ret
       end
my_operate (generic function with 1 method)

julia> f = (
           MOI.VariableIndex(1),
           1 * MOI.VariableIndex(1) + 2,
           3 * MOI.VariableIndex(1) * MOI.VariableIndex(1) + 4,
       )
(MOI.VariableIndex(1), (2) + (1) MOI.VariableIndex(1), (4) + 3.0 MOI.VariableIndex(1)²)

julia> x = f[[2, 1, 2, 1, 2, 3]]
((2) + (1) MOI.VariableIndex(1), MOI.VariableIndex(1), (2) + (1) MOI.VariableIndex(1), MOI.VariableIndex(1), (2) + (1) MOI.VariableIndex(1), (4) + 3.0 MOI.VariableIndex(1)²)

julia> foo(x) = MOI.Utilities.operate(+, Int, x...)
foo (generic function with 1 method)

julia> bar(x) = my_operate(+, Int, x...)
bar (generic function with 1 method)

julia> @benchmark foo($x)
BenchmarkTools.Trial: 10000 samples with 996 evaluations.
 Range (min  max):  25.117 ns   1.026 μs  ┊ GC (min  max):  0.00%  95.46%
 Time  (median):     28.674 ns              ┊ GC (median):     0.00%
 Time  (mean ± σ):   35.176 ns ± 56.915 ns  ┊ GC (mean ± σ):  10.90% ±  6.54%

  ▁ ▆█▇▅▅▅▃▂▂▁▁▁                                              ▂
  █▇█████████████████████▇▇▇▆▇▆▆▆▅▇▇▇█▅▄▆▇▇▅▅▅▄▄▅▄▄▄▃▃▄▄▆▇▆▇█ █
  25.1 ns      Histogram: log(frequency) by time      74.8 ns <

 Memory estimate: 224 bytes, allocs estimate: 2.

julia> @benchmark bar($x)
BenchmarkTools.Trial: 10000 samples with 206 evaluations.
 Range (min  max):  379.612 ns   11.897 μs  ┊ GC (min  max): 0.00%  95.58%
 Time  (median):     419.449 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   466.205 ns ± 502.626 ns  ┊ GC (mean ± σ):  5.41% ±  4.83%

     ▃▄▆▇▇█▆▅▄▄▃▂▂▂▁   ▁▁▁▁ ▁▁▁▁▂▁▁▁▁▁                          ▂
  ▇▇███████████████████████████████████▇█▇█▇▇▆▇▆▆▆▇▇▆▇████▇▇▆▅▆ █
  380 ns        Histogram: log(frequency) by time        673 ns <

 Memory estimate: 608 bytes, allocs estimate: 9.

@odow odow changed the title Use afoldl to avoid StackOverflow [Utilities] use foldl to avoid StackOverflow Sep 24, 2023
@odow
Copy link
Member

odow commented Sep 24, 2023

I don't think your benchmark is correct because there was a bug in your method. The 25.117 ns is too small to be realistic.

Here's what I get now:

julia> import MathOptInterface as MOI

julia> using BenchmarkTools

julia> function my_operate(::typeof(+), ::Type{T}, f, g, h, args...) where {T<:Number}
           ret = MOI.Utilities.operate(+, T, f, g)
           ret = MOI.Utilities.operate!(+, T, ret, h)
           for a in args
               ret = MOI.Utilities.operate!(+, T, ret, a)
           end
           return ret
       end
my_operate (generic function with 1 method)

julia> f = (
           MOI.VariableIndex(1),
           1 * MOI.VariableIndex(1) + 2,
           3 * MOI.VariableIndex(1) * MOI.VariableIndex(1) + 4,
       )
(MOI.VariableIndex(1), (2) + (1) MOI.VariableIndex(1), (4) + 3.0 MOI.VariableIndex(1)²)

julia> x = f[[2, 1, 2, 1, 2, 3]]
((2) + (1) MOI.VariableIndex(1), MOI.VariableIndex(1), (2) + (1) MOI.VariableIndex(1), MOI.VariableIndex(1), (2) + (1) MOI.VariableIndex(1), (4) + 3.0 MOI.VariableIndex(1)²)

julia> foo(x) = MOI.Utilities.operate(+, Int, x...)
foo (generic function with 1 method)

julia> bar(x) = my_operate(+, Int, x...)
bar (generic function with 1 method)

julia> @benchmark foo($x)
BenchmarkTools.Trial: 10000 samples with 641 evaluations.
 Range (min  max):  191.588 ns    6.407 μs  ┊ GC (min  max):  0.00%  96.14%
 Time  (median):     207.365 ns               ┊ GC (median):     0.00%
 Time  (mean ± σ):   251.995 ns ± 436.067 ns  ┊ GC (mean ± σ):  14.51% ±  8.04%

  ▇▆▆█▅▂▁ ▁   ▁▁▁▁                                              ▁
  ████████████████▇▇█▇█▇▇█▇▆▇▆▅▅▅▆▆▆▆▅▆▆▄▅▅▄▅▆▄▅▅▃▄▄▄▄▄▅▄▅▃▄▄▄▄ █
  192 ns        Histogram: log(frequency) by time        491 ns <

 Memory estimate: 576 bytes, allocs estimate: 7.

julia> @benchmark bar($x)
BenchmarkTools.Trial: 10000 samples with 155 evaluations.
 Range (min  max):  669.084 ns   25.458 μs  ┊ GC (min  max): 0.00%  96.29%
 Time  (median):     697.110 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   774.301 ns ± 948.249 ns  ┊ GC (mean ± σ):  5.08% ±  4.06%

  ▄█▇▅▃▁          ▁                                             ▁
  ██████▆▇███▆▇▅▇▇███▇▆▅▆▅▆▆▅▄▆▅▅▆▇▆▅▅▅▅▅▅▅▃▄▄▄▄▄▆▄▄▄▄▄▅▄▄▄▅▅▅▄ █
  669 ns        Histogram: log(frequency) by time       1.56 μs <

 Memory estimate: 608 bytes, allocs estimate: 9.

@odow odow mentioned this pull request Sep 23, 2023
5 tasks
@odow odow merged commit ee780ba into master Sep 24, 2023
@odow odow deleted the bl/afoldl branch September 24, 2023 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants