Set max_methods for lock and unlock to 1#60803
Conversation
This should prevent invalidations, particularly in IJulia.
|
xref PR #59377 |
|
(bump) |
| end | ||
|
|
||
| lock(::IO) = nothing | ||
| typeof(lock).name.max_methods = UInt8(1) |
There was a problem hiding this comment.
What happens if a method of lock gets defined and a method instance gets compiled/inferred before you change max_methods here? Would it cause miscompilation?
There was a problem hiding this comment.
From grepping the code and looking at the include order in Base.jl I think this is where the function is declared the first time, so that should not happen.
There was a problem hiding this comment.
What if some other PR switches the code around a bit, sometime in the future? Will we get miscompilation silently?
There was a problem hiding this comment.
This seems like a separate concern from this PR since this code pattern is already used in other places already. Maybe move the discussion to a separate issue?
There was a problem hiding this comment.
I don't think it is a separate issue. The pattern of modifying max_methods for a function is only used for a few examples currently:
-
copyto! -
merge -
error(PR merge the three methods oferrorinto a single method #60878 attempts to prevent that)
So if it becomes common to modify max_methods for various functions all over the code base, I think then we would actually need to start worrying about stuff like this.
There was a problem hiding this comment.
Yes, since it is already in the code base, a dedicated issue makes more sense rather than comments on a PR.
|
Any chance of this getting backported to 1.13? Would be really nice to have those invalidations fixed in IJulia. |
|
I took the liberty of adding the backport label, but feel free to remove it if it's too big a change. |
This should prevent invalidations, particularly in IJulia:
See for example: JuliaLang/IJulia.jl#1192 (comment)