leaf-types cache for ml-matches #36166
Conversation
Since we are doing a subtype search (like a the top of the function), the resulting object from the search does not need to be the same.
This lets us put more objects in here without incurring additional search code (just the initial cost of computing the hash for the tuple type lookup computation).
| @@ -390,6 +390,7 @@ end | |||
| t = Timer(0) do t | |||
| tc[] += 1 | |||
| end | |||
| Libc.systemsleep(0.005) | |||
There was a problem hiding this comment.
Is there something better we can do here. Tests that depend on system scheduling behavior almost always turn out flakey.
There was a problem hiding this comment.
Without this, the test depends on the scheduling (this test assumes that this statement takes a least a millisecond to complete). This completely fixes the test to avoid that flakiness by ensuring it always takes at least that long.
| return env.t; | ||
| } | ||
| } | ||
| if (((jl_datatype_t*)unw)->isdispatchtuple) { |
There was a problem hiding this comment.
Isn't this the same condition as the previous if block?
There was a problem hiding this comment.
It is in characters in this PR right now. But conceptually, they don't share a common root, so I've listed them separately. That way, if someone alters one, it won't affect the other.
There was a problem hiding this comment.
Alright, you're the methodcache czar, it just looked odd as is. Maybe there should be helpers that make clear in which sense this is used, even if the implementation of these helpers is the same at the moment?
There was a problem hiding this comment.
True, I'll keep that in mind. It won't be the last time (even this month) that I edit the code, haha.
|
I'd like to plan to merge this tomorrow (after #36260). Please let me know if review is incomplete. |
|
Looks fine to me. No reason to block merging, but I have a couple thoughts. I wonder if we can take this farther to memoize more of |
| JL_TIMING(METHOD_LOOKUP_FAST); | ||
| mt = jl_gf_mtable(F); | ||
| entry = jl_typemap_assoc_exact(mt->cache, F, args, nargs, jl_cachearg_offset(mt), world); | ||
| if (entry == NULL) { |
There was a problem hiding this comment.
I suspect we'll lose a bit of performance from needing two lookups here sometimes. Would be nice to be able to combine the tables somehow.
There was a problem hiding this comment.
That would be likely to hurt performance and memory usage to combine them (had that situation in a intermediate state of this PR, before I finished separating the tables). What we're doing here is expecting that one of these two tables is most likely empty (functions usually either get specialized on leaf types entirely or they don't)—and so we're just going to bypass the previous table in most cases without even attempting a lookup.
| env.match.ti = mi->specTypes; | ||
| } | ||
| else { | ||
| // TODO: should we use jl_subtype_env instead (since we know that `type <: meth->sig` by transitivity) |
There was a problem hiding this comment.
👍, plus we can skip this entirely if there are no static params.
There was a problem hiding this comment.
Oh, wow, yeah, not sure how I missed that, as it seems so obvious. I'm also thinking of removing this value entirely from the results (since often it should be available from the later typeinf_edge cache lookup)
When we do a
methodslookup and get back a single result, it's very easy for us to cache that information, since we essentially already have the data-structure for it (TypeMapEntry). This lets us shave off a bit of time on micro-benchmarks by putting this information into a hash table instead of a tree (which is still used to handle the general case):