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.

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

Fixups for #47383 (fixes runbenchmarks("sort")) #47822

Merged
merged 5 commits into from
Dec 13, 2022

Conversation

LilithHafner
Copy link
Member

@LilithHafner LilithHafner commented Dec 7, 2022

  • Test and fix overflow in CountSort
  • Test and fix overly strict type signature in setindex!(v::WithoutMissingVector, x, i)

I believe this is the issue behind JuliaCI/BaseBenchmarks.jl#305, #47795, etc.

@LilithHafner LilithHafner added bugfix This change fixes an existing bug sorting Put things in order backport 1.9 Change should be backported to release-1.9 labels Dec 7, 2022
@LilithHafner LilithHafner added this to the 1.9 milestone Dec 7, 2022
@LilithHafner
Copy link
Member Author

The problem is a missing maybe_unsigned here

range = o === Reverse ? mn-mx : mx-mn

That should correspond with the line here:

range = maybe_unsigned(o === Reverse ? mn-mx : mx-mn)

I have a fix queued for once CI fails with the new test.

@LilithHafner
Copy link
Member Author

LilithHafner commented Dec 7, 2022

The new test failed. 👍

@LilithHafner LilithHafner marked this pull request as ready for review December 7, 2022 05:25
@LilithHafner
Copy link
Member Author

Now the new test passes (also 👍)

@LilithHafner
Copy link
Member Author

@nanosoldier runbenchmarks("sort")

@nanosoldier
Copy link
Collaborator

Your job failed.

@LilithHafner
Copy link
Member Author

@vtjnash, would you post the log of this failure? I thought this would fix it, but perhaps it's a separate bug or another.

@vtjnash
Copy link
Member

vtjnash commented Dec 7, 2022

Sure, it seems to be some sort of error with loading the package:

      From worker 3:    2022-12-07T03:12:38.160 | starting benchscript.jl (STDOUT/STDERR will be redirected to the result folder)                                                                                                                  
      From worker 3:    ERROR: LoadError: CanonicalIndexError: setindex! not defined for Base.Sort.WithoutMissingVector{Int8, Vector{Union{Missing, Int8}}}                                                                                        
      From worker 3:    Stacktrace:                                                                                                                                                                                                                
      From worker 3:      [1] error_if_canonical_setindex(#unused#::IndexCartesian, A::Base.Sort.WithoutMissingVector{Int8, Vector{Union{Missing, Int8}}}, #unused#::Int64)                                                                        
      From worker 3:        @ Base ./abstractarray.jl:1407                                                                                                                                                                                         
      From worker 3:      [2] setindex!                                                                                                                                                                                                            
      From worker 3:        @ ./abstractarray.jl:1396 [inlined]                                                                                                                                                                                    
      From worker 3:      [3] _sort!(v::Base.Sort.WithoutMissingVector{Int8, Vector{Union{Missing, Int8}}}, #unused#::Base.Sort.CountingSort, o::Base.Order.ForwardOrdering, kw::NamedTuple{(:scratch, :lo, :hi, :mn, :mx), Tuple{Nothing, Int64, I
nt64, Int8, Int8}})                                         
      From worker 3:        @ Base.Sort ./sort.jl:846                                                                    
      From worker 3:      [4] _sort!                        
      From worker 3:        @ ./sort.jl:811 [inlined]                                                                    
      From worker 3:      [5] _sort!(v::Base.Sort.WithoutMissingVector{Int8, Vector{Union{Missing, Int8}}}, a::Base.Sort.ComputeExtrema{Base.Sort.ConsiderCountingSort{Base.Sort.CountingSort, Base.Sort.ConsiderRadixSort{Base.Sort.RadixSort, Bas
e.Sort.Small{80, Base.Sort.InsertionSortAlg, PartialQuickSort{Missing, Missing, Base.Sort.InsertionSortAlg}}}}}, o::Base.Order.ForwardOrdering, kw::NamedTuple{(:scratch, :lo, :hi), Tuple{Nothing, Int64, Int64}})
      From worker 3:        @ Base.Sort ./sort.jl:787                                                                    
      From worker 3:      [6] _sort!                        
      From worker 3:        @ ./sort.jl:760 [inlined]                                                                    
      From worker 3:      [7] _sort!(v::Base.Sort.WithoutMissingVector{Int8, Vector{Union{Missing, Int8}}}, a::Base.Sort.Small{40, Base.Sort.InsertionSortAlg, Base.Sort.CheckSorted{Base.Sort.ComputeExtrema{Base.Sort.ConsiderCountingSort{Base.S
ort.CountingSort, Base.Sort.ConsiderRadixSort{Base.Sort.RadixSort, Base.Sort.Small{80, Base.Sort.InsertionSortAlg, PartialQuickSort{Missing, Missing, Base.Sort.InsertionSortAlg}}}}}}}, o::Base.Order.ForwardOrdering, kw::NamedTuple{(:scratch, :
lo, :hi), Tuple{Nothing, Int64, Int64}})                    
      From worker 3:        @ Base.Sort ./sort.jl:692                                                                    
      From worker 3:      [8] _sort!                        
      From worker 3:        @ ./sort.jl:668 [inlined]                                                                    
      From worker 3:      [9] _sort!                        
      From worker 3:        @ ./sort.jl:621 [inlined]                                                                    
      From worker 3:     [10] _sort!                        
      From worker 3:        @ ./sort.jl:692 [inlined]                                                                    
      From worker 3:     [11] _sort!                        
      From worker 3:        @ ./sort.jl:637 [inlined]                                                                    
      From worker 3:     [12] _sort!                        
      From worker 3:        @ ./sort.jl:568 [inlined]                                                                    
      From worker 3:     [13] #sort!#28                     
      From worker 3:        @ ./sort.jl:1359 [inlined]                                                                   
      From worker 3:     [14] sort!                         
      From worker 3:        @ ./sort.jl:1352 [inlined]                                                                   
      From worker 3:     [15] sort(v::Vector{Union{Missing, Int8}}; kws::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})                                                                                                            
      From worker 3:        @ Base.Sort ./sort.jl:1385                                                                   
      From worker 3:     [16] sort                          
      From worker 3:        @ ./sort.jl:1385 [inlined]                                                                   
      From worker 3:     [17] var"##core#9673"(A#9309::Vector{Union{Missing, Int8}})                                     
      From worker 3:        @ BaseBenchmarks.UnionBenchmarks ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:489                                                                                                                           
      From worker 3:     [18] var"##sample#9674"(::Tuple{Vector{Union{Missing, Int8}}}, __params::BenchmarkTools.Parameters)                                                                                                                       
      From worker 3:        @ BaseBenchmarks.UnionBenchmarks ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:495                                                                                                                           
      From worker 3:     [19] _run(b::BenchmarkTools.Benchmark, p::BenchmarkTools.Parameters; verbose::Bool, pad::String, kwargs::Base.Pairs{Symbol, Integer, NTuple{4, Symbol}, NamedTuple{(:samples, :evals, :gctrial, :gcsample), Tuple{Int64, I
nt64, Bool, Bool}}})                                        
      From worker 3:        @ BenchmarkTools ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:99                                                                                                                                            
      From worker 3:     [20] kwcall(::NamedTuple{(:verbose, :pad, :samples, :evals, :gctrial, :gcsample), Tuple{Bool, String, Int64, Int64, Bool, Bool}}, ::typeof(BenchmarkTools._run), b::BenchmarkTools.Benchmark, p::BenchmarkTools.Parameters
)                                                           
      From worker 3:        @ BenchmarkTools ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:92                                                                                                                                            
      From worker 3:     [21] #invokelatest#2               
      From worker 3:        @ ./essentials.jl:818 [inlined]                                                              
      From worker 3:     [22] #run_result#45                
      From worker 3:        @ ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:34 [inlined]                       
      From worker 3:     [23] run(b::BenchmarkTools.Benchmark, p::BenchmarkTools.Parameters; progressid::Base.UUID, nleaves::Int64, ndone::Int64, kwargs::Base.Pairs{Symbol, Any, NTuple{6, Symbol}, NamedTuple{(:verbose, :pad, :samples, :evals, 
:gctrial, :gcsample), Tuple{Bool, String, Int64, Int64, Bool, Bool}}})                                                   
      From worker 3:        @ BenchmarkTools ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:117                                                                                                                                           
      From worker 3:     [24] macro expansion               
      From worker 3:        @ ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:135 [inlined]                      

Lilith Hafner added 2 commits December 8, 2022 16:45
This fixes the test failure because it allows for automatic conversion.
The manual for implementing the AbstractArray interface also does not recomend
a type signature for the value arg in setindex!.
@LilithHafner
Copy link
Member Author

The bug is still from #47383; the first pair of commits fixed one bug only to unveil another. The second pair of commits test and fix a second bug. It's wild that these bugs come up in benchmarks but neither CI nor PkgEval (though they will now come up in CI). Perhaps we are benchmarking code paths that are not used anywhere in the package ecosystem.

@nanosoldier runbenchmarks("sort")

@LilithHafner LilithHafner changed the title Test and fix overflow in countsort Fixups for #47383 Dec 8, 2022
@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - successfully executed benchmarks. A full report can be found here.

@LilithHafner LilithHafner changed the title Fixups for #47383 Fixups for #47383 (fixes runbenchmarks("sort")) Dec 8, 2022
@LilithHafner LilithHafner requested a review from petvana December 13, 2022 09:24
Copy link
Member

@petvana petvana left a comment

Choose a reason for hiding this comment

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

I don't see any problem with the PR.

@KristofferC KristofferC merged commit 965bc7d into master Dec 13, 2022
@KristofferC KristofferC deleted the lh/countsort-overflow branch December 13, 2022 12:38
KristofferC pushed a commit that referenced this pull request Dec 14, 2022
* add test demonstrating overflow in countsort

* fix overflow in countsort

* remove unnecessary type annotations (fixes tests)

This fixes the test failure because it allows for automatic conversion.
The manual for implementing the AbstractArray interface also does not recomend
a type signature for the value arg in setindex!.

Co-authored-by: Lilith Hafner <Lilith.Hafner@gmail.com>
(cherry picked from commit 965bc7d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug sorting Put things in order
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants