-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
use ScopedValues for MPFR precision and rounding #51362
Conversation
Could this fix #51058? |
const DEFAULT_PRECISION = Ref{Clong}(256) | ||
|
||
const CURRENT_PRECISION = ScopedValue{Clong}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could set it to default precision? Or is it important that the default precision could be changed globally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We support it at the moment, so dropping it would be breaking, but I'm not sure how necessary it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need to be able to change it, don't you need this to be
const CURRENT_PRECISION = ScopedValue{RefValue{Clong}}(Ref{Clong}())
since ScopedValue
is immutable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but the default
field is marked const
:
Lines 40 to 46 in b189bed
mutable struct ScopedValue{T} | |
const has_default::Bool | |
const default::T | |
ScopedValue{T}() where T = new(false) | |
ScopedValue{T}(val) where T = new{T}(true, val) | |
ScopedValue(val::T) where T = new{T}(true, val) | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mean that it's an immutable struct, I just mean that you can't actually change the value other than by introducing a new dynamic scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's correct.
Nice! I would be curious to see what's the performance impact. |
@@ -158,11 +159,17 @@ end | |||
# The rounding mode here shouldn't matter. | |||
significand_limb_count(x::BigFloat) = div(sizeof(x._d), sizeof(Limb), RoundToZero) | |||
|
|||
rounding_raw(::Type{BigFloat}) = ROUNDING_MODE[] | |||
rounding_raw(::Type{BigFloat}) = something(Base.ScopedValues.get(CURRENT_ROUNDING_MODE), ROUNDING_MODE[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vchuravy should this overload Base.get
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has different API so I decided not to.
get(dict, key, value)
vs get(ScopedValue)
. But that was me being conservative, also it returns Some/Nothing
.
I could see an argument for get(SValue, value)
I see a minor impact: function f(N)
x = big(0.0)
for i = 1:N
x += BigFloat("0.1")
end
return x
end
function g(N)
setrounding(BigFloat, RoundDown) do
f(N)
end
end On
On this branch:
So a bit of overhead, but not too bad. |
To clarify, this would access the scoped values around 4 million times (in each iteration, access both rounding mode and precision for thee decimal to binary conversion, then the addition), so 40 milliseconds of overhead would be about 10ns per call. |
No. |
Changing the precision has a bit more overhead, but we would assume that's a rare operation to begin with? |
Actually, that's probably more common. However I see roughly the same overhead when using the following: function h(N)
setprecision(BigFloat, 256) do
f(N)
end
end |
What's the status here @simonbyrne? Rebase and merge? |
9aee915
to
f9dc27f
Compare
I think this is ready to go now. |
Should fix thread safety issues. Actually fixes JuliaLang#27139
Should fix thread safety issues. Actually fixes JuliaLang#27139
PR #55911 fixes a regression introduced by this PR, could anyone help review it? |
Another follow-up to JuliaLang#51362. Make sure the global default precision and rounding mode are only dereferenced when necessary (when there's no relevant scope, in the ScopedValues sense). Currently this change doesn't result in performance improvements, presumably due to how costly the access to a `ScopedValue` currently is, but the idea is to avoid the cost of the dereference when possible. Once ScopedValues are better optimized by the compiler, I guess this would also result in better effects in case it's known that a call is within a ScopedValues scope.
Another follow-up to JuliaLang#51362. Make sure the global default precision and rounding mode are only dereferenced when necessary (when there's no relevant scope, in the ScopedValues sense). Currently this change doesn't result in performance improvements, presumably due to how costly the access to a `ScopedValue` currently is, but the idea is to avoid the cost of the dereference when possible. Once ScopedValues are better optimized by the compiler, I guess this would also result in better effects in case it's known that a call is within a ScopedValues scope.
Tried cherry-picking on top of v1.11.3 and it seems to apply cleanly. Marking for backport. |
I'm not sure this is a great candidate for backport, as it does change behavior slightly |
There is also an open bug for scoped values #56062 that may cause problems |
You're right. There's also this regression, not fixed yet: #55899. |
Should fix thread safety issues.
Actually fixes #27139