Skip to content

Commit

Permalink
don't mutate globals when constructing Rational from `AbstractIrrat…
Browse files Browse the repository at this point in the history
…ional` (#55853)

Relying on `ScopedValues`, set `BigFloat` precision without mutating the
global default, while constructing `Rational` from `AbstractIrrational`.

Also helps avoid reading the global defaults for the precision and
rounding mode, together with #56095.

What does this fix:
* in the case of the `Irrational` constants defined in `MathConstants`:
relevant methods have `@assume_effects :foldable` applied, which
includes `:effect_free`, which requires that no globals be mutated
(followup on #55886)
* in the case of `AbstractIrrational` values in general, this PR
prevents data races on the global `BigFloat` precision

(cherry picked from commit 2a89f71)
  • Loading branch information
nsajko authored and KristofferC committed Feb 26, 2025
1 parent b565c28 commit d04631d
Show file tree
Hide file tree
Showing 2 changed files with 119 additions and 7 deletions.
34 changes: 27 additions & 7 deletions base/irrationals.jl
Original file line number Diff line number Diff line change
Expand Up @@ -60,20 +60,40 @@ AbstractFloat(x::AbstractIrrational) = Float64(x)::Float64
Float16(x::AbstractIrrational) = Float16(Float32(x)::Float32)
Complex{T}(x::AbstractIrrational) where {T<:Real} = Complex{T}(T(x))

function _irrational_to_rational(::Type{T}, x::AbstractIrrational) where T<:Integer
o = precision(BigFloat)
function _irrational_to_rational_at_current_precision(::Type{T}, x::AbstractIrrational) where {T <: Integer}
bx = BigFloat(x)
r = rationalize(T, bx, tol = 0)
if abs(BigFloat(r) - bx) > eps(bx)
r
else
nothing # Error is too small, repeat with greater precision.
end
end
function _irrational_to_rational_at_precision(::Type{T}, x::AbstractIrrational, p::Int) where {T <: Integer}
f = let x = x
() -> _irrational_to_rational_at_current_precision(T, x)
end
setprecision(f, BigFloat, p)
end
function _irrational_to_rational_at_current_rounding_mode(::Type{T}, x::AbstractIrrational) where {T <: Integer}
if T <: BigInt
_throw_argument_error_irrational_to_rational_bigint() # avoid infinite loop
end
p = 256
while true
setprecision(BigFloat, p)
bx = BigFloat(x)
r = rationalize(T, bx, tol=0)
if abs(BigFloat(r) - bx) > eps(bx)
setprecision(BigFloat, o)
r = _irrational_to_rational_at_precision(T, x, p)
if r isa Number
return r
end
p += 32
end
end
function _irrational_to_rational(::Type{T}, x::AbstractIrrational) where {T <: Integer}
f = let x = x
() -> _irrational_to_rational_at_current_rounding_mode(T, x)
end
setrounding(f, BigFloat, RoundNearest)
end
Rational{T}(x::AbstractIrrational) where {T<:Integer} = _irrational_to_rational(T, x)
_throw_argument_error_irrational_to_rational_bigint() = throw(ArgumentError("Cannot convert an AbstractIrrational to a Rational{BigInt}: use rationalize(BigInt, x) instead"))
Rational{BigInt}(::AbstractIrrational) = _throw_argument_error_irrational_to_rational_bigint()
Expand Down
92 changes: 92 additions & 0 deletions test/threads_exec.jl
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,61 @@ end
# (expected test duration is about 18-180 seconds)
Timer(t -> killjob("KILLING BY THREAD TEST WATCHDOG\n"), 1200)

module ConcurrencyUtilities
function new_task_nonsticky(f)
t = Task(f)
t.sticky = false
t
end

"""
run_concurrently(worker, n)::Nothing
Run `n` tasks of `worker` concurrently. Return when all workers are done.
"""
function run_concurrently(worker, n)
tasks = map(new_task_nonsticky Returns(worker), Base.OneTo(n))
foreach(schedule, tasks)
foreach(fetch, tasks)
end

"""
run_concurrently_in_new_task(worker, n)::Task
Return a task that:
* is not started yet
* when started, runs `n` tasks of `worker` concurrently
* returns when all workers are done
"""
function run_concurrently_in_new_task(worker, n)
function f(t)
run_concurrently(t...)
end
new_task_nonsticky(f Returns((worker, n)))
end
end

module AbstractIrrationalExamples
for n 0:9
name_aa = Symbol(:aa, n)
name_ab = Symbol(:ab, n)
name_ba = Symbol(:ba, n)
name_bb = Symbol(:bb, n)
@eval begin
Base.@irrational $name_aa exp(BigFloat(2)^$n)
Base.@irrational $name_ab exp(BigFloat(2)^-$n)
Base.@irrational $name_ba exp(-(BigFloat(2)^$n))
Base.@irrational $name_bb exp(-(BigFloat(2)^-$n))
end
end
const examples = (
aa0, aa1, aa2, aa3, aa4, aa5, aa6, aa7, aa8, aa9,
ab0, ab1, ab2, ab3, ab4, ab5, ab6, ab7, ab8, ab9,
ba0, ba1, ba2, ba3, ba4, ba5, ba6, ba7, ba8, ba9,
bb0, bb1, bb2, bb3, bb4, bb5, bb6, bb7, bb8, bb9,
)
end

@testset """threads_exec.jl with JULIA_NUM_THREADS == $(ENV["JULIA_NUM_THREADS"])""" begin

@test Threads.threadid() == 1
Expand Down Expand Up @@ -1347,6 +1402,43 @@ end
end
end

@testset "race on `BigFloat` precision when constructing `Rational` from `AbstractIrrational`" begin
function test_racy_rational_from_irrational(::Type{Rational{I}}, c::AbstractIrrational) where {I}
function construct()
Rational{I}(c)
end
function is_racy_rational_from_irrational()
worker_count = 10 * Threads.nthreads()
task = ConcurrencyUtilities.run_concurrently_in_new_task(construct, worker_count)
schedule(task)
ok = true
while !istaskdone(task)
for _ 1:1000000
ok &= precision(BigFloat) === prec
end
GC.safepoint()
yield()
end
fetch(task)
ok
end
prec = precision(BigFloat)
task = ConcurrencyUtilities.new_task_nonsticky(is_racy_rational_from_irrational)
schedule(task)
ok = fetch(task)::Bool
setprecision(BigFloat, prec)
ok
end
@testset "c: $c" for c AbstractIrrationalExamples.examples
Q = Rational{Int128}
# metatest: `test_racy_rational_from_irrational` needs the constructor
# to not be constant folded away, otherwise it's not testing anything.
@test !Core.Compiler.is_foldable(Base.infer_effects(Q, Tuple{typeof(c)}))
# test for race
@test test_racy_rational_from_irrational(Q, c)
end
end

@testset "task time counters" begin
@testset "enabled" begin
try
Expand Down

0 comments on commit d04631d

Please sign in to comment.