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

Avoid some stackoverflow during typeintersect. #48029

Merged
merged 3 commits into from
Jan 4, 2023
Merged

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented Dec 28, 2022

When I dig in #46736, I found that subtype_in_env_existential is somehow dangous as we flip all typevar to the right side.
I tried to follow #48006 style but it caused many test failure. So this PR just makes the following change to avoid some known stackoverflow:

  1. Make sure env is restored between 2 adjacent subtype_in_env_existential. Thus the second subtype check wont be influenced by the first one. (close SIGSEGV and StackOverflow from dispatching on ::Type{T}, ::Array{Union{T,Missing},N}) #36185)
  2. Add a fast path to skip the repeated intersect_aside (close Healpix.jl tests hang on Julia v1.9 #46736.)

@N5N3 N5N3 added the types and dispatch Types, subtyping and method dispatch label Dec 28, 2022
@KristofferC KristofferC requested a review from vtjnash December 29, 2022 07:30
@giordano
Copy link
Contributor

giordano commented Dec 30, 2022

On this branch tests of Healpix.jl crash instead of hanging (which is a slight improvement):

Internal error: stack overflow in type inference of readPolarizedMapFromFITS(String, Int64, Type{Float32}).
This might be caused by recursion over very long tuples or argument lists.
Map I/O: Error During Test at /Users/mose/.julia/packages/Healpix/UmwcZ/test/runtests.jl:48
  Got exception outside of a @test
  LoadError: TypeError: in new, expected TypeVar, got Type{Float32}
  Stacktrace:
    [1] readPolarizedMapFromFITS(fileName::AbstractString, column::Any, t::Type{T}) where T
      @ Healpix ~/.julia/packages/Healpix/UmwcZ/src/map_io.jl:107
    [2] top-level scope
      @ ~/.julia/packages/Healpix/UmwcZ/test/test_mapio.jl:22
    [3] include(fname::String)
      @ Base.MainInclude ./client.jl:478
    [4] macro expansion
      @ ~/.julia/packages/Healpix/UmwcZ/test/runtests.jl:49 [inlined]
    [5] macro expansion
      @ ~/repo/julia/usr/share/julia/stdlib/v1.10/Test/src/Test.jl:1502 [inlined]
    [6] top-level scope
      @ ~/.julia/packages/Healpix/UmwcZ/test/runtests.jl:49
    [7] include(fname::String)
      @ Base.MainInclude ./client.jl:478
    [8] top-level scope
      @ none:6
    [9] eval
      @ ./boot.jl:370 [inlined]
   [10] exec_options(opts::Base.JLOptions)
      @ Base ./client.jl:280
   [11] _start()
      @ Base ./client.jl:522
  in expression starting at /Users/mose/.julia/packages/Healpix/UmwcZ/test/test_mapio.jl:22
Test Summary: | Pass  Error  Total  Time
Map I/O       |    7      1      8  0.7s

Is this a bug in the package or something else to fix on the julia side? CC: @ziotom78

@KristofferC
Copy link
Member

FWIW, this seems to work around it:

diff --git a/src/map_io.jl b/src/map_io.jl
index 3bea696..d3c2699 100644
--- a/src/map_io.jl
+++ b/src/map_io.jl
@@ -104,7 +104,10 @@ function readPolarizedMapFromFITS(
     end
 
     f = CFITSIO.fits_open_table(fileName)
-    i, q, u = (readMapFromFITS(f, colidx, t) for colidx in (column_i, column_q, column_u))
+    i = readMapFromFITS(f, column_i, t)
+    q = readMapFromFITS(f, column_q, t)
+    u = readMapFromFITS(f, column_u, t)
+
     CFITSIO.fits_close_file(f)
 
     PolarizedHealpixMap(i, q, u)

@N5N3
Copy link
Member Author

N5N3 commented Dec 30, 2022

Not that suppressing as i, q, u = (readMapFromFITS(f, colidx, t) for ...... would meet #23618
and

julia> code_typed(Healpix.readMapFromFITS, Tuple{Healpix.CFITSIO.FITSFile,Int,DataType})
ERROR: StackOverflowError:

@N5N3
Copy link
Member Author

N5N3 commented Dec 30, 2022

Might be a smaller MWE
this gives stackoverflow

struct HealpixMap{T,AA<:AbstractArray{T,1}}
    pixels::AA
    function HealpixMap{Union{T,Nothing},AA}(a::Number) where {T,AA<:AbstractArray{Union{T,Nothing},1}}
        new(Union{T, Nothing}[nothing for i in 1:a])
    end
    function HealpixMap{T,AA}(arr::AA) where {T,AA<:AbstractArray{T,1}}
        new(arr)
    end
end
HealpixMap{T}(arr) where {T} = HealpixMap{T,Vector{T}}(collect(arr))
f(::Type{T}) where {T} = HealpixMap{T}(T[])
code_typed(f, Tuple{DataType})

but this gives the correct result

struct HealpixMap{T,AA<:AbstractArray{T,1}}
    pixels::AA
    function HealpixMap{Union{T,Nothing},AA}(a::Number) where {T,AA<:AbstractArray{Union{T,Nothing},1}}
        new{Union{T,Nothing},AA}(Union{T, Nothing}[nothing for i in 1:a])
    end
    function HealpixMap{T,AA}(arr::AA) where {T,AA<:AbstractArray{T,1}}
        new{T,AA}(arr)
    end
end

@N5N3 N5N3 force-pushed the inter-fix3 branch 2 times, most recently from 6e08876 to e467ded Compare December 31, 2022 07:48
@vtjnash
Copy link
Member

vtjnash commented Jan 3, 2023

Seems good to me. I think there was a small typo in the commit message and it needs to re-run CI since it had some networking issues. Could you push an update?

N5N3 added 3 commits January 3, 2023 12:55
When we perform re-`intersection_unionall`, the `Union` bounds might be generated from `simple_join  and thus not identical to the src `Union`.
This commit adds a fast-path to skip the following `intersect_all.
This fixes the MWE reported in JuliaLang#47874 (comment)

And this fixes the remaining internal error in `Healpix.jl`'s test.

gc fix
@vtjnash vtjnash added don't squash Don't squash merge 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 Jan 3, 2023
@vtjnash
Copy link
Member

vtjnash commented Jan 3, 2023

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@vtjnash
Copy link
Member

vtjnash commented Jan 4, 2023

I don't think it needs to be blocking to merge this, but AngularMomentumAlgebra may be good to look into https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/36cd9c6_vs_79e29e3/AngularMomentumAlgebra.primary.log (and maybe some of the other reported hangs)

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Jan 4, 2023
@N5N3
Copy link
Member Author

N5N3 commented Jan 4, 2023

I don't think it needs to be blocking to merge this, but AngularMomentumAlgebra may be good to look into https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/36cd9c6_vs_79e29e3/AngularMomentumAlgebra.primary.log (and maybe some of the other reported hangs)

I'll take a look at AngularMomentumAlgebra and other failure and push the fix (if needed) in #48006.

@N5N3 N5N3 merged commit a5ab48f into JuliaLang:master Jan 4, 2023
@N5N3 N5N3 deleted the inter-fix3 branch January 4, 2023 08:09
@KristofferC KristofferC added the backport 1.9 Change should be backported to release-1.9 label Jan 4, 2023
@N5N3 N5N3 removed the merge me PR is reviewed. Merge when all tests are passing label Jan 4, 2023
@KristofferC KristofferC mentioned this pull request Jan 10, 2023
41 tasks
KristofferC pushed a commit that referenced this pull request Jan 16, 2023
Avoid some stackoverflow during typeintersect.

(cherry picked from commit a5ab48f)
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
don't squash Don't squash merge types and dispatch Types, subtyping and method dispatch
Projects
None yet
5 participants