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

Reduce invalidations from convert methods #45051

Closed
wants to merge 1 commit into from
Closed

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Apr 21, 2022

On nightly (but not earlier Julia versions), loading ColorTypes causes
more than 1800 invalidations. These changes eliminate all but ~20.

The convert(TypeOfBottom, x) method was added in #31602, I think to help with invalidations. Consequently I'd be grateful for a quick glance from @vtjnash. It now seems to be counterproductive, perhaps because we may have turned off invalidations in the case of method ambiguity. While this can't be the only issue, the method may simply be unnecessary now.

On nightly (but not earlier Julia versions), loading ColorTypes causes
more than 1800 invalidations. These changes eliminate all but ~20.
@timholy timholy added the compiler:latency Compiler latency label Apr 21, 2022
@timholy timholy requested a review from vtjnash April 21, 2022 15:25
@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 21, 2022

how does that method come up? its purpose is to help the algorithm with deciding that this call always throws

@timholy
Copy link
Sponsor Member Author

timholy commented Apr 22, 2022

Because there are a lot of invalidations with different causes, there's some merit in interactive exploration, so let me first provide a way you can investigate directly on your own:

pkg> add SnoopCompile SnoopCompileCore ColorTypes
using SnoopCompileCore
invs = @snoopr using ColorTypes;
using SnoopCompile                 # SnoopCompile uses ColorTypes via FlameGraphs so you can't load it before you collect `invs`
trees = invalidation_trees(invs)
tree = trees[end]
root = tree.backedges[end]
ascend(root)

The output of ascend is a FoldingTrees interactive menu that lets you launch Cthulhu on specific callsites.

Now here's a summary of the results (I'm using screenshots so you can see the colors to mark non-concrete types). The invalidations are

julia> tree
inserting convert(::Type{C}, c::Colorant) where C<:Colorant in ColorTypes at /home/tim/.julia/packages/ColorTypes/6m8P7/src/conversions.jl:73 invalidated:
   backedges: 1: superseding convert(::Type{Union{}}, x) in Base at essentials.jl:280 with MethodInstance for convert(::Core.TypeofBottom, ::Any) (2583 children)
   36 mt_cache

and of those 2583 children (some are duplicates, the unique number is ~1800) they come in several categories, of which the dominant ones are fixed here:

  1. Calls to maximum/minimum where the sentinel gets computed by fallback methods, but a lack of specialization on the function types means the convert call is uninferrable:
    image
    Fix: use maximum(a; init) to bypass the automatic sentinel code.
  2. Constructors where the concrete type is uninferrable:
    image
    for which Cthuhlu gives you (for the Perm constructor):
    image
    This issue also affect (::Type{IOContext{IO_t}})(::IO_t, ::Base.ImmutableDict{Symbol, Any}) where IO_t<:REPL.Terminals.TextTerminal, Base.Fix2(::typeof(==), ::Type), (::Type{Base.ReadEachIterator{Char, IOT}})(::IO) where IOT<:IO, and others.

I'm coming to think that we need a @concrete annotation to indicate that a given method should never be inferred for abstract types. That would force runtime dispatch and break the chain of invalidations.

@timholy
Copy link
Sponsor Member Author

timholy commented Apr 25, 2022

It's also worth emphasizing that this invalidation is new-ish on master, there are no major invalidations from ColorTypes even on 1.8-beta3.

@timholy
Copy link
Sponsor Member Author

timholy commented Apr 27, 2022

I'm not sure it's useful information, but since it's new I thought it would make sense to figure out what change caused the change in invalidation behavior. git-bisect blames #44512. CC @aviatesk

Does that PR only Union-split or does it also world-split? Should it be checking whether the method returns an error?

@timholy timholy added the kind:regression Regression in behavior compared to a previous version label Apr 29, 2022
@aviatesk
Copy link
Sponsor Member

aviatesk commented May 2, 2022

Thanks for bisect!!!

Does that PR only Union-split or does it also world-split?

When we union-split a callsite during inlining, we always need to world-split (i.e. add backedges to all the possible inlined/resolved callees) since the union-split optimization uses world-wise static information.

So taking from your blog post, this PR changed this heuristics:

Experience has shown that once a method has a couple of specializations, it may accumulate yet more, and to reduce the amount of recompilation needed Julia quickly abandons attempts to optimize the case handling containers with unknown element types.

Especially, with this PR, we sync the inlining optimization with the type inference, i.e. we union-split a callsite as far as type inference successfully figured out all the callees no matter if all the call signatures are concrete or not (previously we used to only union-split a callsite only when all the possible call signatures are concrete and final).

I'm still wondering if we want to revert that PR as this is one another instance of our endless fight against the trade-off between aggressive optimization and best latency -- I'd like to spend a bit more time to think how we can retain the added optimization while fixing this latency. If we can't come up with a solution I'm happy to partially revert the PR (i.e. enable it only within Core.Compiler) since my main motivation was to improve the performance of our high level compiler, not a general performance. But your idea to special case the obvious-error callee sounds interesting for example.

vtjnash added a commit that referenced this pull request Jul 11, 2022
This should make it impossible to accidentally define or call this
method on foreign types.

Refs: #31602
Fixes: #45837
Closes: #45051
KristofferC pushed a commit that referenced this pull request Jul 18, 2022
This should make it impossible to accidentally define or call this
method on foreign types.

Refs: #31602
Fixes: #45837
Closes: #45051
ffucci pushed a commit to ffucci/julia that referenced this pull request Aug 11, 2022
This should make it impossible to accidentally define or call this
method on foreign types.

Refs: JuliaLang#31602
Fixes: JuliaLang#45837
Closes: JuliaLang#45051
pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022
This should make it impossible to accidentally define or call this
method on foreign types.

Refs: JuliaLang#31602
Fixes: JuliaLang#45837
Closes: JuliaLang#45051
@DilumAluthge DilumAluthge deleted the teh/ct_invalidations branch September 3, 2022 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:latency Compiler latency kind:regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants