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

Partially revert "Remove specialized sort! method for PartialQuickSort{Int} (fixes #12833)" #31632

Merged

Conversation

kmsquire
Copy link
Member

@kmsquire kmsquire commented Apr 6, 2019

This reverts commit 9e8fcd3, except for the
test. The underlying issue(s) causing #12833 seem to have been fixed with recent
changes in inference.

@StefanKarpinski
Copy link
Member

Can't quite tell of the Linux32 test run failure is of concern or not. cc @staticfloat

@staticfloat
Copy link
Member

Hmm, a stack overflow during the Profile tests.... not sure what that's about, but IMO it's unrelated, especially since it's only happening on Linux 32.

@kmsquire
Copy link
Member Author

Profile stack overflow addressed by #31693.

Should nanosoldier be run on this? Otherwise should be good to go.

@kmsquire kmsquire force-pushed the kms/reinstate-partial-quick-sort-int branch from e4f8e6a to a3803da Compare April 12, 2019 17:22
@kmsquire
Copy link
Member Author

Rebased to incorporate #31693.

@kmsquire
Copy link
Member Author

kmsquire commented Apr 12, 2019

Travis error was a DNS problem (failed to clone from https://github.com/JuliaLang/Example.jl.git, error: GitError(Code:ERROR, Class:Net, curl error: Could not resolve host: github.com). Restarted.

The buildbot error was the same error I got on OSX here, so it is probably a recently introduced bug.

      From worker 9:	Error During Test at /buildworker/worker/tester_linux32/build/share/julia/stdlib/v1.3/LinearAlgebra/test/ambiguous_exec.jl:4
      From worker 9:	  Test threw exception
      From worker 9:	  Expression: detect_ambiguities(LinearAlgebra; imported=true, recursive=true) == []
      From worker 9:	  TypeError: in TypeVar, in upper bound, expected Type, got Core.Compiler.Iterators.Filter{getfield(Core.Compiler, Symbol("##254#261")){Core.Compiler.IdDict{Int32,Int32}},Array{Int32,1}}
      From worker 9:	  Stacktrace:
      From worker 9:	   [1] typeintersect at ./reflection.jl:560 [inlined]
      From worker 9:	   [2] #isambiguous#23(::Bool, ::typeof(Base.isambiguous), ::Method, ::Method) at ./reflection.jl:1266
      From worker 9:	   [3] #isambiguous at ./none:0 [inlined]
      From worker 9:	   [4] #detect_ambiguities#30(::Bool, ::Bool, ::Bool, ::typeof(detect_ambiguities), ::Module) at /buildworker/worker/package_linux32/build/usr/share/julia/stdlib/v1.3/Test/src/Test.jl:1439
      From worker 9:	   [5] (::getfield(Test, Symbol("#kw##detect_ambiguities")))(::NamedTuple{(:imported, :recursive),Tuple{Bool,Bool}}, ::typeof(detect_ambiguities), ::Module) at ./none:0
      From worker 9:	   [6] top-level scope at /buildworker/worker/tester_linux32/build/share/julia/stdlib/v1.3/LinearAlgebra/test/ambiguous_exec.jl:4
      From worker 9:	   [7] include at ./boot.jl:328 [inlined]
      From worker 9:	   [8] include_relative(::Module, ::String) at ./loading.jl:1094
      From worker 9:	   [9] include(::Module, ::String) at ./Base.jl:31
      From worker 9:	   [10] exec_options(::Base.JLOptions) at ./client.jl:295
      From worker 9:	   [11] _start() at ./client.jl:464
      From worker 9:	  
      From worker 9:	ERROR: Error while loading expression starting at /buildworker/worker/tester_linux32/build/share/julia/stdlib/v1.3/LinearAlgebra/test/ambiguous_exec.jl:4
      From worker 9:	caused by [exception 1]
      From worker 9:	There was an error during testing
      From worker 9:	running testset LinearAlgebra/lapack...

Link in case it gets rerun: https://build.julialang.org/#/builders/13/builds/1602

…t{Int} (fixes JuliaLang#12833)"

This reverts commit 9e8fcd3, except for the
test.  The underlying issue(s) causing JuliaLang#12833 seem to have been fixed with recent
changes in inference.
@kmsquire kmsquire force-pushed the kms/reinstate-partial-quick-sort-int branch from 865a09c to 58d7769 Compare April 23, 2019 16:24
@kmsquire
Copy link
Member Author

32-bit AppVeyor build failed on a DNS lookup. Can someone please restart it? (Cc: @staticfloat @ararslan)

Everything else has passed.

@ararslan
Copy link
Member

Restarted.

@kmsquire
Copy link
Member Author

Another (seemingly unrelated) error. ☹️
Now getting two Profile test failures, followed by a related ArgumentError in 32-bit AppVeyor tests:

Test Failed at C:\projects\julia\julia-\share\julia\stdlib\v1.3\Profile\test\runtests.jl:34
  Expression: !(isempty(str))
   Evaluated: !(isempty(""))
Error in testset Profile:
Test Failed at C:\projects\julia\julia-\share\julia\stdlib\v1.3\Profile\test\runtests.jl:41
  Expression: !(isempty(String(take!(iobuf))))
   Evaluated: !(isempty(""))
Error in testset Profile:
Error During Test at C:\projects\julia\julia-\share\julia\test\testdefs.jl:20
  Got exception outside of a @test
  Error while loading expression starting at C:\projects\julia\julia-\share\julia\stdlib\v1.3\Profile\test\runtests.jl:27
  caused by [exception 1]
  ArgumentError: reducing over an empty collection is not allowed
...
   [11] maximum at .\reducedim.jl:652 [inlined]
   [12] print_flat(::Base.GenericIOBuffer{Array{UInt8,1}}, ::Array{Base.StackTraces.StackFrame,1}, ::Array{Int32,1}, ::Int32, ::Profile.ProfileFormat) at C:\projects\julia\usr\share\julia\stdlib\v1.3\Profile\src\Profile.jl:398

@ararslan
Copy link
Member

Sounds like #29880. The Windows buildbot passed, so I think the issue is just AppVeyor being weird and can be safely ignored here.

@kmsquire
Copy link
Member Author

👍
Merge then?

@fredrikekre fredrikekre merged commit c41d5a3 into JuliaLang:master Apr 25, 2019
@kmsquire kmsquire deleted the kms/reinstate-partial-quick-sort-int branch April 26, 2019 14:55
nalimilan added a commit that referenced this pull request Aug 3, 2020
This specialized method is actually considerably slower than the one that also
works for `PartialQuickSort{<:OrdinalRange}`. It had been removed before in
9e8fcd3 due to an inference issue, and reintroduced by c41d5a3 (#31632) once
that issue got fixed, but apparently without benchmarks. It turns out that saving
one comparison per iteration isn't worth it. The gain is especially large when
sorting a value close to the end of the vector, as the specialized method always
sorted all values lower than the requested one.
nalimilan added a commit that referenced this pull request Aug 3, 2020
This specialized method is actually considerably slower than the one that also
works for `PartialQuickSort{<:OrdinalRange}`. It had been removed before in
9e8fcd3 due to an inference issue, and reintroduced by c41d5a3 (#31632) once
that issue got fixed, but apparently without benchmarks. It turns out that saving
one comparison per iteration isn't worth it. The gain is especially large when
looking for a value among the largest in the array, as the specialized method always
sorted all values lower than the requested one.
nalimilan added a commit that referenced this pull request Aug 3, 2020
This specialized method is actually considerably slower than the one that also
works for `PartialQuickSort{<:OrdinalRange}`. It had been removed before in
9e8fcd3 due to an inference issue, and reintroduced by c41d5a3 (#31632) once
that issue got fixed, but apparently without benchmarks. It turns out that saving
one comparison per iteration isn't worth it. The gain is especially large when
looking for a value among the largest in the array, as the specialized method always
sorted all values lower than the requested one.
nalimilan added a commit that referenced this pull request Aug 3, 2020
This specialized method is actually considerably slower than the one that also
works for `PartialQuickSort{<:OrdinalRange}`. It had been removed before in
9e8fcd3 due to an inference issue, and reintroduced by c41d5a3 (#31632) once
that issue got fixed, but apparently without benchmarks. It turns out that saving
one comparison per iteration isn't worth it. The gain is especially large when
looking for a value among the largest in the array, as the specialized method always
sorted all values lower than the requested one.
StefanKarpinski pushed a commit that referenced this pull request Sep 30, 2020
This specialized method is actually considerably slower than the one that also
works for `PartialQuickSort{<:OrdinalRange}`. It had been removed before in
9e8fcd3 due to an inference issue, and reintroduced by c41d5a3 (#31632) once
that issue got fixed, but apparently without benchmarks. It turns out that saving
one comparison per iteration isn't worth it. The gain is especially large when
looking for a value among the largest in the array, as the specialized method always
sorted all values lower than the requested one.
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.

5 participants