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.

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

Add invokelatest barrier to string(...) in @assert #55739

Merged

Conversation

topolarity
Copy link
Member

@topolarity topolarity commented Sep 11, 2024

This change protects @assert from invalidations to Base.string(...) by adding an invokelatest barrier.

A common source of invalidations right now is print(io, join(args...)). The problem is:

  1. Inference concludes that join(::Any...) returns Union{String,AnnotatedString}
  2. The print call is union-split to String and AnnotatedString
  3. This code is now invalidated when StyledStrings defines print(io, ::AnnotatedString)

The invalidation chain for @assert is similar: @assert 1 == 1 calls into string(::Expr) which calls into print(io, join(args::Any...)). Unfortunately that leads to the invalidation of almost all @asserts without an explicit error message

Similar to #55583 (comment)

@IanButterworth
Copy link
Sponsor Member

IanButterworth commented Sep 11, 2024

Great! This fixes the Artifacts._artifact_str invalidation seen here JuliaPackaging/LazyArtifacts.jl#1 (comment)

Master with JuliaPackaging/LazyArtifacts.jl#1

% ./julia --start=no --trace-compile=stderr --trace-compile-timing -e "using InteractiveUtils; @time @time_imports using MicroMamba"
      0.5 ms  Printf
     15.0 ms  Dates
      0.3 ms  Scratch
     10.0 ms  LazyArtifacts
      1.0 ms  TOML
      7.2 ms  Preferences
      0.5 ms  JLLWrappers
               ┌ 545.2 ms micromamba_jll.__init__() 99.93% compilation time (88% recompilation)
    545.7 ms  micromamba_jll 99.84% compilation time (88% recompilation)
      0.4 ms  MicroMamba
  0.586889 seconds (763.19 k allocations: 40.931 MiB, 5.56% gc time, 92.84% compilation time: 88% of which was recompilation)

Full detail:

% ./julia --start=no --trace-compile=stderr --trace-compile-timing -e "using InteractiveUtils; @time @time_imports using MicroMamba"
      0.5 ms  Printf
     15.0 ms  Dates
      0.3 ms  Scratch
     10.0 ms  LazyArtifacts
      1.0 ms  TOML
      7.2 ms  Preferences
      0.5 ms  JLLWrappers
#=    0.0 ms =# precompile(Tuple{typeof(micromamba_jll.__init__)})
#=    0.0 ms =# precompile(Tuple{typeof(micromamba_jll.find_artifact_dir)})
#=    2.4 ms =# precompile(Tuple{Type{Base.Val{x} where x}, Module})
#=    2.0 ms =# precompile(Tuple{Type{NamedTuple{(:honor_overrides,), T} where T<:Tuple}, Tuple{Bool}})
#=  439.9 ms =# precompile(Tuple{typeof(Artifacts._artifact_str), Module, String, Base.SubString{String}, String, Base.Dict{String, Any}, Base.SHA1, Base.BinaryPlatforms.Platform, Base.Val{LazyArtifacts}})
#=   24.4 ms =# precompile(Tuple{typeof(Base.unique!), Array{String, 1}})
#=    1.3 ms =# precompile(Tuple{typeof(Base.invokelatest), Any})
#=    0.0 ms =# precompile(Tuple{typeof(JLLWrappers.get_julia_libpaths)})
#=    7.8 ms =# precompile(Tuple{typeof(Base.vcat), Array{String, 1}, Array{String, 1}})
               ┌ 545.2 ms micromamba_jll.__init__() 99.93% compilation time (88% recompilation)
    545.7 ms  micromamba_jll 99.84% compilation time (88% recompilation)
      0.4 ms  MicroMamba
#=    2.0 ms =# precompile(Tuple{Type{NamedTuple{(:value, :time, :bytes, :gctime, :gcstats, :lock_conflicts, :compile_time, :recompile_time), T} where T<:Tuple}, Tuple{Nothing, Float64, Int64, Float64, Base.GC_Diff, Int64, Float64, Float64}})
#=    2.9 ms =# precompile(Tuple{typeof(Base.getproperty), NamedTuple{(:value, :time, :bytes, :gctime, :gcstats, :lock_conflicts, :compile_time, :recompile_time), Tuple{Nothing, Float64, Int64, Float64, Base.GC_Diff, Int64, Float64, Float64}}, Symbol})
#=   26.4 ms =# precompile(Tuple{typeof(Core.kwcall), NamedTuple{(:msg,), Tuple{Nothing}}, typeof(Base.time_print), Base.TTY, Float64, Int64, Int64, Int64, Int64, Float64, Float64, Bool})
  0.586889 seconds (763.19 k allocations: 40.931 MiB, 5.56% gc time, 92.84% compilation time: 88% of which was recompilation)

This PR with JuliaPackaging/LazyArtifacts.jl#1

% ./julia --start=no -e "using InteractiveUtils; @time @time_imports using MicroMamba"
      0.7 ms  Printf
     21.5 ms  Dates
      0.5 ms  Scratch
     14.9 ms  LazyArtifacts
      0.9 ms  TOML
      8.7 ms  Preferences
      0.5 ms  JLLWrappers
               ┌ 44.7 ms micromamba_jll.__init__() 99.60% compilation time (3% recompilation)
     45.3 ms  micromamba_jll 98.17% compilation time (3% recompilation)
      0.4 ms  MicroMamba
  0.129260 seconds (177.54 k allocations: 9.573 MiB, 34.42% compilation time: 3% of which was recompilation)

More info & improvement at #55740

This change protects `@assert` from invalidations to `Base.string(...)`
which are fairly easy to trigger.

Perhaps the worst offender right now is `using StyledStrings`
which invalidates `string(::Expr)`, which unfortunately leads to
invalidation of almost `@assert`s (without an explicit message)
@aviatesk
Copy link
Sponsor Member

I think this is a great idea. The 1-arg @assert often leads to poor type inference due to recursive extended inference (xref: #55625). Using invokelatest would eliminate that concern too.

@fredrikekre fredrikekre merged commit 945517b into JuliaLang:master Sep 12, 2024
9 checks passed
@KristofferC KristofferC added the backport 1.11 Change should be backported to release-1.11 label Sep 12, 2024
kshyatt pushed a commit that referenced this pull request Sep 12, 2024
This change protects `@assert` from invalidations to `Base.string(...)`
by adding an `invokelatest` barrier.

A common source of invalidations right now is `print(io,
join(args...))`. The problem is:
1. Inference concludes that `join(::Any...)` returns
`Union{String,AnnotatedString}`
2. The `print` call is union-split to `String` and `AnnotatedString`
3. This code is now invalidated when StyledStrings defines `print(io,
::AnnotatedString)`

The invalidation chain for `@assert` is similar: ` @Assert 1 == 1` calls
into `string(::Expr)` which calls into `print(io, join(args::Any...))`.
Unfortunately that leads to the invalidation of almost all `@assert`s
without an explicit error message

Similar to
#55583 (comment)
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we want to do invokelatest here, as it could do some very weird things if Core.Compiler runs into an asserts if the resulting string code then does anything unexpected. Should we try to separate that so that Core.Compiler uses a slightly different asserts macro?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 14, 2024

We could also use invokeinworld with tls_world_age, or an inferencebarrier to achieve similar effects?

aviatesk added a commit that referenced this pull request Sep 16, 2024
This version would be better as per this comment: <#55739 (review)>
I confirmed this still allows us to avoid invalidations reported at
#55583.
aviatesk added a commit that referenced this pull request Sep 16, 2024
This version would be better as per this comment: <#55739 (review)>
I confirmed this still allows us to avoid invalidations reported at
#55583.
aviatesk added a commit that referenced this pull request Sep 17, 2024
This version would be better as per this comment: <#55739 (review)>
I confirmed this still allows us to avoid invalidations reported at
#55583.
aviatesk added a commit that referenced this pull request Sep 17, 2024
…55783)

This version would be better as per this comment:
<#55739 (review)>
I confirmed this still allows us to avoid invalidations reported at
#55583.
@KristofferC KristofferC mentioned this pull request Sep 24, 2024
30 tasks
KristofferC pushed a commit that referenced this pull request Sep 24, 2024
This change protects `@assert` from invalidations to `Base.string(...)`
by adding an `invokelatest` barrier.

A common source of invalidations right now is `print(io,
join(args...))`. The problem is:
1. Inference concludes that `join(::Any...)` returns
`Union{String,AnnotatedString}`
2. The `print` call is union-split to `String` and `AnnotatedString`
3. This code is now invalidated when StyledStrings defines `print(io,
::AnnotatedString)`

The invalidation chain for `@assert` is similar: ` @Assert 1 == 1` calls
into `string(::Expr)` which calls into `print(io, join(args::Any...))`.
Unfortunately that leads to the invalidation of almost all `@assert`s
without an explicit error message

Similar to
#55583 (comment)

(cherry picked from commit 945517b)
KristofferC pushed a commit that referenced this pull request Sep 24, 2024
…55783)

This version would be better as per this comment:
<#55739 (review)>
I confirmed this still allows us to avoid invalidations reported at
#55583.

(cherry picked from commit f808606)
aviatesk added a commit that referenced this pull request Sep 24, 2024
…55783)

This version would be better as per this comment:
<#55739 (review)>
I confirmed this still allows us to avoid invalidations reported at
#55583.

(cherry picked from commit f808606)
KristofferC added a commit that referenced this pull request Sep 25, 2024
Backported PRs:
- [x] #55773 <!-- Add compat entry for `Base.donotdelete` -->
- [x] #41244 <!-- Fix shell `cd` error when working dir has been deleted
-->
- [x] #55795 <!-- fix #52986, regression in `@doc` of macro without REPL
loaded -->
- [x] #55829 <!-- [Dates] Make test more robust against non-UTC
timezones -->
- [x] #55641 <!-- fall back to slower stat filesize if optimized
filesize fails -->
- [x] #55744 <!-- fix #45494, error in ssa conversion with complex type
decl -->
- [x] #55783 <!-- use `inferencebarrier` instead of `invokelatest` for
1-arg `@assert` -->
- [x] #55739 <!-- Add `invokelatest` barrier to `string(...)` in
`@assert` -->

Need manual backport:
- [ ] #55798 <!-- Broadcast binary ops involving strided triangular -->

Contains multiple commits, manual intervention needed:
- [ ] #55509 <!-- Fix cong implementation to be properly random and not
just cycling. -->
- [ ] #55569 <!-- Add a docs section about loading/precomp/ttfx time
tuning -->
- [ ] #55824 <!-- Replace regex package module checks with actual code
checks -->

Non-merged PRs with backport label:
- [ ] #55845 <!-- privatize annotated string API, take two -->
- [ ] #55828 <!-- Fix some corner cases of `isapprox` with unsigned
integers -->
- [ ] #55813 <!-- Check for conflicting `@ccallable` name before JIT
registration -->
- [ ] #55743 <!-- doc: heap snapshot viewing -->
- [ ] #55741 <!-- Change annotations to use a NamedTuple -->
- [ ] #55534 <!-- Set stdlib sources as read-only during installation
-->
- [ ] #55499 <!-- propagate the terminal's `displaysize` to the
`IOContext` used by the REPL -->
- [ ] #55458 <!-- Allow for generically extracting unannotated string
-->
- [ ] #55457 <!-- Make AnnotateChar equality consider annotations -->
- [ ] #55453 <!-- Privatise the annotations API, for StyledStrings -->
- [ ] #55355 <!-- relocation: account for trailing path separator in
depot paths -->
- [ ] #55220 <!-- `isfile_casesensitive` fixes on Windows -->
- [ ] #55169 <!-- `propertynames` for SVD respects private argument -->
- [ ] #54457 <!-- Make `String(::Memory)` copy -->
- [ ] #53957 <!-- tweak how filtering is done for what packages should
be precompiled -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
- [ ] #50813 <!-- More doctests for Sockets and capitalization fix -->
- [ ] #50157 <!-- improve docs for `@inbounds` and
`Base.@propagate_inbounds` -->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants