typemap: attempt split on typenames also #35384
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JeffBezanson
reviewed
Jul 2, 2020
src/typemap.c
Outdated
| jl_datatype_t *dt = (jl_datatype_t*)t1; | ||
| if ((abstract || !dt->abstract) && !jl_is_kind(t1)) | ||
| return 0; | ||
| return 1; |
There was a problem hiding this comment.
In this case we return NULL above, so in what sense is that imprecise? Or maybe a better question is, when does this function differ from jl_type_extract_name() == NULL?
There was a problem hiding this comment.
I suppose NULL is imprecise in the sense that we'll never consider asking this jl_type_extract_name_imprecise question if the result was NULL?
JeffBezanson
reviewed
Jul 2, 2020
vtjnash
commented
Jul 2, 2020
src/typemap.c
Outdated
| jl_datatype_t *dt = (jl_datatype_t*)t1; | ||
| if ((abstract || !dt->abstract) && !jl_is_kind(t1)) | ||
| return 0; | ||
| return 1; |
There was a problem hiding this comment.
I suppose NULL is imprecise in the sense that we'll never consider asking this jl_type_extract_name_imprecise question if the result was NULL?
|
@nanosoldier |
|
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
JeffBezanson
approved these changes
Jul 10, 2020
If the parameter is a TypeVar, it cnan have subtypes that might appear in the map itself and require a scan to locate.
This is an odd inversion in the specificity lattice where supertype(T) is more specific than T (and type-equal to T). Handle that here by consistently normalizing to the more specific copy, to avoid complications later.
… argument Previously, the TypeMap could only split on leaf types. Now it also has the ability to split on more abstract types. This helps gives types their own linear list to scan to find its constructors, instead of needing to search through all constructors.
f2f71e8 to
9bae8c4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a recreation of #16418, now working due to many changes in the meantime! In measurements, it appears to successfully shave significant time off
requirecalls: