break some dependency backedges #20792
Conversation
| max{T<:Real}(x::T, y::T) = ifelse(y < x, x, y) | ||
| min{T<:Real}(x::T, y::T) = ifelse(y < x, y, x) | ||
| max{T<:Real}(x::T, y::T) = select_value(y < x, x, y) | ||
| min{T<:Real}(x::T, y::T) = select_value(y < x, y, x) |
There was a problem hiding this comment.
Now that the other methods of ifelse are deprecated, I suppose we could replace ifelse and select_value with a single ifelse builtin.
There was a problem hiding this comment.
once the deprecations get removed, that does seem to be true
|
With this PR, loading Crayons.jl take 0.037s instead of 1.0s as on master and 0.017 on 0.5. Good enough for me :) |
base/deprecated.jl
Outdated
| @@ -826,6 +826,7 @@ end | |||
| # Deprecate vectorized ! | |||
| @deprecate(!(A::AbstractArray{Bool}), .!A) # parens for #20541 | |||
| @deprecate(!(B::BitArray), .!B) # parens for #20541 | |||
| !(::typeof(!)) = identity # make sure ! has at least 4 methods so that for-loops don't end up getting a back-edge to depwarn | |||
There was a problem hiding this comment.
I think it's a bit of a gotcha if !! accepts arguments that ! doesn't. Also, not too long from now the two deprecated methods of ! will be removed, which also removes the need for this method (IIUC), but in the meantime code could have become dependent on this behavior. So I think it would be helpful to type-restrict this.
There was a problem hiding this comment.
OK. I went with just creating an unused Function for this purpose
|
it's not exactly recommended, but there are packages that define comparison operators that return non Bool, that might get caught by these type annotations |
|
If a package wants to do that, it should be able to work around this by adding methods for e.g. |
|
wouldn't it have to do that for all possible subtypes of Real and Number that promotion might send through these methods? |
|
I see what you mean now; yes the ones on the methods that call |
|
Maybe I should assert that |
|
Is it possible to achieve the goals of this PR in another way that doesn't force promotion constraints for arbitrary user types in these fallbacks? |
|
No. This PR is all about making sure type information can be better preserved. |
|
By imposing type-assertion constraints that we previously have not been forcing on user code, which makes this a potentially breaking change post-feature-freeze. |
This would be nicer if we had function types, but this tries to at least hit a couple of the high points fix #20780
|
Ah well, it looks like that was insufficient to break that edge anyways – it's too prevalent from other code too. |
|
The version in 7be9009 looks safer, the I'd run nanosoldier here if I weren't currently hogging the workers elsewhere. Can keep an eye on the nightly runs. |
|
This caused a sizable regression in dates performance https://github.com/JuliaCI/BaseBenchmarkReports/blob/fe41d40a910c29c0489bd50a21458cb68701e9b8/daily_2017_3_4/report.md |
|
Looks like that should be fixed by #20853. This commit had a bunch of coupled side-effects (type-assert not being effect-free meant that we were now able to generate line-number information for |
This might be nicer if we had function types (so that we could declared that
function == end::Boolfor example), but this tries to at least hit a couple of the high points.fix #20780
The main issue here was that BitArray deprecated bitwise operations, so
|and!ended with a lot of edges between anything with bitwise operations or for loops and anything required for printing warnings.