De-duplicate edges in typeinfer instead of gf.c
#58117
Conversation
| assert(!found && "duplicate back-edge registered"); | ||
| #endif | ||
| // reuse an already cached instance of this type, if possible | ||
| // TODO: use jl_cache_type_(tt) like cache_method does, instead of this linear scan? |
There was a problem hiding this comment.
It's worth seeing if we can bypass this remaining linear scan in the MethodTable edge insertion.
If I disable it manually, the timing drops to 440 milliseconds
topolarity
force-pushed
the
ct/typeinfer-edge-dedupe
branch
from
72735ee to
b8aa007
Compare
src/gf.c
Outdated
| jl_gc_wb(callee, backedges); | ||
| } | ||
| else { | ||
| #ifndef JL_NDEBUG |
There was a problem hiding this comment.
This introduces a race condition into the code (because of gc, threads, etc). Probably useful to know this scan is costly on some benchmarks though
There was a problem hiding this comment.
Can you clarify where the race condition is?
Do you mean that inference may simultaneously try to store_backedges for the same caller CI from two different threads?
There was a problem hiding this comment.
Ah, I see, we might be able to now assume that each CI is unique, while in older versions the MI were not expected to be unique
topolarity
force-pushed
the
ct/typeinfer-edge-dedupe
branch
2 times, most recently
from
27c2e29 to
a42a06a
Compare
|
This test points to a serious flaw with this approach: https://buildkite.com/julialang/julia-master/builds/47272#0196ad27-29a2-4ce6-b08d-d7a11a23125b/914-1711 An invalidated CI is likely to repeatedly add the same back-edges when it is re-inferred, unless we force CI invalidation to also perform deletion of stale back-edges. We likely need to take a different direction here. |
|
Actually this should be fine (for the same reason pointed out at #58117 (comment)). It is true that we leave a number of "zombie" back-edges around to invalidated CodeInstances, but that is made no worse by this PR This is ready to go if CI is happy. |
|
That test calls |
Since the caller CodeInstance is always part of the identity that we are de-duplicating on, this makes the linear scan much faster than it is in `gf.c`
…Table` These are restored in their entirety by staticdata.jl, so there's no need to serialize them. Dropping them has the additional advantage of making it unnecessary to de-duplicate edges in `gf.c`
On my system, this saves ~500 ms when loading CairoMakie (and all dependent packages)
topolarity
force-pushed
the
ct/typeinfer-edge-dedupe
branch
from
67232ca to
7525568
Compare
topolarity
requested review from
a team and
StefanKarpinski
and removed request for
a team
2 participants
Without this PR, my system spends ~1.07 seconds just running
store_backedgeswhen doingusing CairoMakieWith this change, that drops to
0.641 secondsThat's still not fast enough for me, but we do call this function 236,092 times so maybe it's understandable.