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.

Sign up for GitHub

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

Jump to bottom

fix invalidations in logging #46481

Merged
merged 1 commit into from
Aug 26, 2022
Merged

fix invalidations in logging #46481

merged 1 commit into from
Aug 26, 2022

Conversation

ranocha
Copy link
Member

@ranocha ranocha commented Aug 25, 2022 •
edited
Loading

This should hopefully fix 905 invalidations coming from Static.jl.

Here is the code: 
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.8.0 (2022-08-17)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

(@v1.8) pkg> activate --temp
  Activating new project at `/tmp/jl_PDrBpd`

(jl_PDrBpd) pkg> add Static
    Updating registry at `~/.julia/registries/General.toml`
   Resolving package versions...
    Updating `/tmp/jl_PDrBpd/Project.toml`
  [aedffcd0] + Static v0.7.6
    Updating `/tmp/jl_PDrBpd/Manifest.toml`
  [615f187c] + IfElse v0.1.1
  [aedffcd0] + Static v0.7.6

julia> using SnoopCompileCore

julia> invalidations = @snoopr using Static

julia> using SnoopCompile

julia> length(uinvalidated(invalidations))
1723

julia> trees = invalidation_trees(invalidations)
7-element Vector{SnoopCompile.MethodInvalidations}:
...
 inserting !(::False) in Static at ~/.julia/packages/Static/sVI3g/src/Static.jl:427 invalidated:
...
                 163: signature Tuple{typeof(!), Any} triggered MethodInstance for Base.CoreLogging.var"#handle_message#2"(::Base.Pairs{Symbol, V, Tuple{Vararg{Symbol, N}}, NamedTuple{names, T}} where {V, N, names, T<:Tuple{Vararg{Any, N}}}, ::typeof(Base.CoreLogging.handle_message), ::Base.CoreLogging.SimpleLogger, ::Base.CoreLogging.LogLevel, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any) (905 children)

help?> isopen
search: isopen partialsortperm partialsortperm! isconcretetype

  isopen(object) -> Bool

  Determine whether an object - such as a stream or timer – is not yet closed. Once an object is closed, it
  will never produce a new event. However, since a closed stream may still have data to read in its buffer, use
  eof to check for the ability to read data. Use the FileWatching package to be notified when a stream might be
  writable or readable.

  Examples
  ≡≡≡≡≡≡≡≡≡≡

  julia> io = open("my_file.txt", "w+");
  
  julia> isopen(io)
  true
  
  julia> close(io)
  
  julia> isopen(io)
  false

Since isopen is documented to return a Bool, this type annotation should help prevent some invalidations.

@Tokazama
Copy link
Contributor

Is it problematic for SimpleLogger to be parametric? Usually subtypes of IO aren't highly complicated so it shouldn't generate a bunch of new code.

@ranocha
Copy link
Member Author

ranocha commented Aug 26, 2022

Is it problematic for SimpleLogger to be parametric? Usually subtypes of IO aren't highly complicated so it shouldn't generate a bunch of new code.

I honestly don't know. I just saw the report of SnoopCompile.jl shown in my first post. I thought that Base.CoreLogging.var"#handle_message#2"(::Base.Pairs{Symbol, V, Tuple{Vararg{Symbol, N}}, NamedTuple{names, T}} where {V, N, names, T<:Tuple{Vararg{Any, N}}}, ::typeof(Base.CoreLogging.handle_message), ::Base.CoreLogging.SimpleLogger,... is the keyword sorter/handler for the method I changed. Since this method uses @nospecialize, it cannot determine all types perfectly, in particular not the one of stream. This can be problematic when downstream packages overload ! for any type, even without type piracy.

@ranocha
Copy link
Member Author

ranocha commented Aug 26, 2022

An alternative for this particular case could be to remove the overload of ! in Static.jl, see SciML/Static.jl#78. But this change here should still prevent invalidations when other packages overload !.

This was referenced Aug 26, 2022
Merged
Merged
@Tokazama
Copy link
Contributor

An alternative for this particular case could be to remove the overload of ! in Static.jl, see SciML/Static.jl#78. But this change here should still prevent invalidations when other packages overload !.

As you pointed out it is a quick fix to a bigger problem because other packages are still overloading it. I'm definitely not going to put a strong fight against not overloading it in static if that's the way the winds blow, but I think Static.jl gets caught first for some of these just because it's widely depended on.

@ChrisRackauckas
Copy link
Member

LoopVectorization.jl is the harder case because it needs to have the ! overload for user code. Users need the ! on Bool SIMD vectors in order for @turbo to work on their code. Static.jl can be kept dev-side, so static_! with no user-facing overload, tell people to const ! = static_! etc. and the issue will be gone, but LoopVectorization.jl invalidates just as much (the same functions) but cannot rely on such code changes because its user model has to assume a "basic user". These kinds of changes are thus required for that.

@ranocha
Copy link
Member Author

ranocha commented Aug 26, 2022

These kinds of changes are thus required for that.

Yeah, I submitted quite a lot of them today (see https://discourse.julialang.org/t/potential-performance-regressions-in-julia-1-8-for-special-un-precompiled-type-dispatches-and-how-to-fix-them/86359/6)

@KristofferC KristofferC merged commit ce6e9ee into JuliaLang:master Aug 26, 2022
@ranocha ranocha deleted the patch-4 branch August 26, 2022 13:39
@giordano giordano added the backport 1.8 Change should be backported to release-1.8 label Aug 26, 2022
ranocha added a commit to ranocha/julia that referenced this pull request Aug 30, 2022
@ranocha
fix invalidations in logging (JuliaLang#46481)
8a58592
ranocha added a commit to ranocha/julia that referenced this pull request Aug 30, 2022
@ranocha
fix another invalidation from Static.jl
902164d 
This is a follow-up to JuliaLang#46481. I suggest the labels `latency` and `backport-1.8`.
@ranocha ranocha mentioned this pull request Aug 30, 2022
Merged
ranocha added a commit to ranocha/julia that referenced this pull request Aug 30, 2022
@ranocha
fix another invalidation from Static.jl
b94b674 
This is a follow-up to JuliaLang#46481. I suggest the labels `latency` and `backport-1.8`.
KristofferC pushed a commit that referenced this pull request Aug 30, 2022
@ranocha @KristofferC
fix invalidations in logging (#46481)
bb94590 
(cherry picked from commit ce6e9ee)
@KristofferC KristofferC mentioned this pull request Aug 30, 2022
Merged
35 tasks
timholy pushed a commit that referenced this pull request Aug 31, 2022
@ranocha
fix another invalidation from Static.jl (#46546)
f1f5d59 
This is a follow-up to #46481. I suggest the labels `latency` and `backport-1.8`.
KristofferC pushed a commit that referenced this pull request Aug 31, 2022
@ranocha @KristofferC
fix another invalidation from Static.jl (#46546)
173f336 
This is a follow-up to #46481. I suggest the labels `latency` and `backport-1.8`.

(cherry picked from commit f1f5d59)
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Sep 7, 2022
@rikhuijzer rikhuijzer mentioned this pull request Oct 3, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aviatesk aviatesk aviatesk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ranocha @Tokazama @ChrisRackauckas @aviatesk @giordano @KristofferC