Skip to content

Conversation

@nsajko
Copy link
Member

@nsajko nsajko commented Mar 3, 2025

One less method.

@nsajko nsajko added the fold sum, maximum, reduce, foldl, etc. label Mar 3, 2025
@nsajko nsajko added merge me PR is reviewed. Merge when all tests are passing and removed merge me PR is reviewed. Merge when all tests are passing labels Mar 4, 2025
@nsajko nsajko marked this pull request as draft March 4, 2025 20:58
@nsajko nsajko marked this pull request as ready for review March 4, 2025 22:23
@nsajko nsajko added the merge me PR is reviewed. Merge when all tests are passing label Mar 4, 2025
@jishnub jishnub merged commit 3d62893 into JuliaLang:master Mar 5, 2025
8 checks passed
@jishnub jishnub removed the merge me PR is reviewed. Merge when all tests are passing label Mar 5, 2025
@nsajko nsajko deleted the Base_unify_prod_Tuple_Vararg_Int_with_a_single_method branch March 5, 2025 21:29
Zentrik referenced this pull request Mar 25, 2025
This is required to avoid introducing a name / binding in the Module
without a corresponding definition.

Resolves #57808
@Zentrik
Copy link
Member

Zentrik commented Mar 25, 2025

This has caused quite a few performance regressions in the array indexing benchmarks in BaseBenchmarks. See for example the following regressions taken from https://github.com/JuliaCI/NanosoldierReports/blob/master/benchmark/by_date/2025-03/21/report.md and https://tealquaternion.camdvr.org/compare.html?start=9294132415cfdf2198b64d03f0fd58f8a4d3a258&end=b8dc57e59db11658cb57424e4e12773f572a6517&stat=min-wall-time&name=array.index

Benchmark % Change
array.index.ind2sub 927.68%
array.index.(mapr_access, SubArray{Float32, 2, BaseBenchmarks.ArrayBenchmarks.ArrayLS{Float32, 3}, Tuple{Int64, Base.Slice{Base.OneTo{Int64}}, Base.Slice{Base.OneTo{Int64}}}, false}) 514.02%
array.index.(mapr_access, SubArray{Float32, 2, Array{Float32, 3}, Tuple{Int64, Base.Slice{Base.OneTo{Int64}}, Base.Slice{Base.OneTo{Int64}}}, true}) 481.90%
array.index.(sumlinear_view, Base.ReinterpretArray{Float32, 2, Int32, Matrix{Int32}, false}) 449.60%
array.index.(sumeach_view, Base.ReinterpretArray{Float32, 2, Int32, Matrix{Int32}, false}) 414.18%
array.index.(mapr_access, SubArray{Int32, 2, Array{Int32, 3}, Tuple{Int64, Base.Slice{Base.OneTo{Int64}}, Base.Slice{Base.OneTo{Int64}}}, true}) 317.28%
array.index.(mapr_access, SubArray{Int32, 2, BaseBenchmarks.ArrayBenchmarks.ArrayLS{Int32, 3}, Tuple{Int64, Base.Slice{Base.OneTo{Int64}}, Base.Slice{Base.OneTo{Int64}}}, false}) 295.44%
array.index.(sumvector_view, Base.ReinterpretArray{BaseBenchmarks.ArrayBenchmarks.PairVals{Float32}, 2, Float64, Matrix{Float64}, false}) 193.03%
array.index.(sumlinear_view, Base.ReinterpretArray{BaseBenchmarks.ArrayBenchmarks.PairVals{Int32}, 2, Int64, Matrix{Int64}, false}) 172.68%
array.index.(sumeach_view, Base.ReinterpretArray{BaseBenchmarks.ArrayBenchmarks.PairVals{Int32}, 2, Int64, Matrix{Int64}, false}) 168.50%
array.index.(sumvector_view, Base.ReinterpretArray{BaseBenchmarks.ArrayBenchmarks.PairVals{Int32}, 2, Int64, Matrix{Int64}, false}) 156.48%
array.index.(sumvector_view, Base.ReinterpretArray{Float32, 2, Int32, Matrix{Int32}, false}) 147.77%

topolarity added a commit to topolarity/julia that referenced this pull request Mar 25, 2025
@nsajko
Copy link
Member Author

nsajko commented Mar 26, 2025

Observations regarding the regression:

  • code_llvm and code_native looks fine and mostly unchanged for prod
  • the inlining cost for prod is now different because of the loop, as shown by Base.print_statement_costs
  • profiling run(BaseBenchmarks.SUITE[["array", "index", "ind2sub"]]) doesn't show either prod or * in the flame graph (perhaps they're inlined after all?)
  • adding @inline to prod doesn't help

KristofferC pushed a commit that referenced this pull request Mar 26, 2025
…)" (#57894)

This reverts commit 3d62893.

See
#57628 (comment)
for array indexing regressions
@nsajko nsajko added the reverted This PR has since been reverted label Jun 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fold sum, maximum, reduce, foldl, etc. reverted This PR has since been reverted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants