stored method interference graph #58948
Conversation
vtjnash
added
latency
Out of curiosity, what bad patterns exist in those packages? |
|
Lots of LinearAlgebra-related overrides with excessively complicated Unions which usually map very poorly to the actual intent of the functions |
|
And lots of functions that fail to infer, resulting in extra work to verify no overloads have been added since |
|
https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/995ff9d_vs_228edd6/report.html suggests this needs some fixup. |
Store full method interference relationship graph in interferences field
of Method to avoid expensive morespecific calls during dispatch. This
provides significant performance improvements:
- Replace method comparisons with precomputed interference lookup.
- Optimize ml_matches minmax computation using interference lookups.
- Optimize sort_mlmatches for large return sets by iterating over
interferences instead of all matching methods.
- Add method_morespecific_via_interferences in both C and Julia.
This representation may exclude some edges that are implied by
transitivity since sort_mlmatches will ensure the correct result by
following strong edges. Ambiguous edges are guaranteed to be checkable
without recursion.
Also fix a variety of bugs along the way:
- Builtins signature would cause them to try to discard all other
methods during `sort_mlmatches`.
- Some ambiguities were over-estimated, which now are improved upon.
- Setting lim==-1 now gives the same limited list of methods as lim>0,
since that is actually faster now than attempting to give the
unsorted list. This provides a better fix to #53814 than #57837 and
fixes #58766.
- Reverts recent METHOD_SIG_LATEST_HAS_NOTMORESPECIFIC attempt (though
not the whole commit), since I found a significant problem with any
usage of that bit during testing: it only tracks methods that
intersect with a target, but new methods do not necessarily intersect
with any existing target.
This provides a decent performance improvement to `methods` calls, which
implies a decent speed up to package loading also (e.g. ModelingToolkit
loads in about 4 seconds instead of 5 seconds).
Could restore this later, so keeping it around in the PR, but please squash merge this away.
type-intersect makes mistakes, and does not need to crash the process when it does
vtjnash
force-pushed
the
jn/method-interference-graph
branch
from
ff6b0e0 to
f175dd0
Compare
|
@nanosoldier |
|
This runs vs 1.12 now which might give kind of noisy results. |
|
The package evaluation job you requested has completed - possible new issues were detected. Report summary✖ Packages that failed12 packages failed only on the current version.
8 packages failed on the previous version too. ✔ Packages that passed tests3 packages passed tests only on the current version.
59 packages passed tests on the previous version too. |
|
I think the report looks good (taking into account one running on master and the other on 1.12). |
vtjnash
removed
the
needs pkgeval
vtjnash
force-pushed
the
jn/method-interference-graph
branch
from
ce35695 to
47c3d67
Compare
Locally, I measure about a 10-20% load time improvement for the packages in #57436, or about half of the measured regressions (and they hopefully should also run inference slightly faster too since all method lookup are now much faster). We'd hoped to stop computing this entirely before #265 was fixed, but since then it hasn't made a ton of sense that we have to compute most of this graph for identifying invalidations, and then we had to recompute it every time the user called
methods. At this time it now seems to make more sense to just store the graph explicitly (it is fairly small size impact) and reap the performance benefits later. Most good code should already have been hitting the fast paths here and not see any impact from this, but most packages also contain at least some transitive dependency on something like ArrayLayouts (or BlockArrays or LazyArrays) whose implementation forces it onto the slow fallback paths for much of those packages code. In the future, this should also allow us to focus our performance efforts on the load performance of activating Method and better caching for those, since the load performance ofverify_callis now relatively negligible.(please preserve the first commit message not this description when merging)