pkgimage: use faster flags to jl_matching_methods #48841
Conversation
This does a better job of filtering out duplicates, which can both save a lot of time and avoid spurious invalidations. For example, this is an example I had been shown recently: ``` julia> @time Base._methods_by_ftype(Tuple{typeof(eltype), Type{A} where A<:(NamedTuple{names, T} where T<:Tuple where names)}, -1, Base.get_world_counter()); 5.200027 seconds (651.79 k allocations: 28.765 MiB, 93.73% gc time) julia> @time Base._methods_by_ftype(Tuple{typeof(eltype), Type{A} where A<:(NamedTuple{names, T} where T<:Tuple where names)}, 10, Base.get_world_counter()); 0.403162 seconds (647.60 k allocations: 28.124 MiB, 20.61% gc time) ```
|
Should we do some benchmarking with this, e.g. on a PkgEval run? Or do we expect this to be uniformly faster for algorithmic reasons? |
|
Er, no, we know this is algorithmically slower in general itself. It is faster because it can help avoid invalidations. (I fixed the numbers in the example above) |
There was a problem hiding this comment.
OK that aligns with my initial understanding that lim == -1 is generally the fast-path, if you're getting all matches.
Sounds like an important change. I didn't realize that we are marking extra invalidations due to imprecise method matching. That's probably worth the extra perf cost to fix, but given the performance variation we've been seeing lately (e.g. #48811) I do think we'll want a good benchmark for these kinds of hairy improvements in general
Not that we need to gate the PR on that necessarily, but is there a quick MWE that this is known to improve?
|
It seems more impactful when #48846 is included too, since that seems much better at avoiding work. |
oscardssmith
removed
the
merge me
This does a better job of filtering out duplicates, which can both save a lot of time and avoid spurious invalidations. (cherry picked from commit eb36cb9)
|
I had to drop this from 1.9 since a straight up backport caused errors in the precompile tests. |
| @@ -936,7 +936,7 @@ precompile_test_harness("code caching") do dir | |||
| j = findfirst(==(tagbad), invalidations) | |||
| @test invalidations[j-1] == "insert_backedges_callee" | |||
| @test isa(invalidations[j-2], Type) | |||
| @test isa(invalidations[j+1], Vector{Any}) # [nbits(::UInt8)] | |||
| @test invalidations[j+1] === nothing || isa(invalidations[j+1], Vector{Any}) # [nbits(::UInt8)] | |||
There was a problem hiding this comment.
The whole point of this test was to ensure that we could identify the cause of the invalidation.
There was a problem hiding this comment.
It can be very costly to decide, which is why we don't here on deserialization, cutting off the search as soon as we show that there is more than one reason, without knowing what they are.
There was a problem hiding this comment.
We need to be able to find the source of invalidations, otherwise how can we fix them? But #48982 may provide a solution.
|
I think this needs to be reverted. |
This modifies #48841 to restore the required logging data. When logging is on, it recalculates the method-match as needed, casting a wider net to ensure that we identify the trigger of the invalidation.
This modifies #48841 to restore the required logging data. By collecting at least one additional match, we retain the possibility of identifying at least one trigger of invalidation.
* Ensure accurate invalidation logging data This modifies #48841 to restore the required logging data. By collecting at least one additional match, we retain the possibility of identifying at least one trigger of invalidation. * Bump number of invalidation causes * Update src/staticdata_utils.c Co-authored-by: Jameson Nash <[email protected]> --------- Co-authored-by: Jameson Nash <[email protected]>
* Ensure accurate invalidation logging data This modifies JuliaLang#48841 to restore the required logging data. By collecting at least one additional match, we retain the possibility of identifying at least one trigger of invalidation. * Bump number of invalidation causes * Update src/staticdata_utils.c Co-authored-by: Jameson Nash <[email protected]> --------- Co-authored-by: Jameson Nash <[email protected]>
* Ensure accurate invalidation logging data This modifies JuliaLang#48841 to restore the required logging data. By collecting at least one additional match, we retain the possibility of identifying at least one trigger of invalidation. * Bump number of invalidation causes * Update src/staticdata_utils.c Co-authored-by: Jameson Nash <[email protected]> --------- Co-authored-by: Jameson Nash <[email protected]>
This does a better job of filtering out duplicates, which can both save a lot of time and avoid spurious invalidations.
For example, this is an example I had been shown recently: