Use objects instead of closures for type mappers #36576
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
|
@typescript-bot perf test this |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 63338f9. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 979bc6a. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ahejlsberg
added
the
Experiment
|
@typescript-bot perf test this |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 979bc6a. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at c7d3806. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 61f0c7a. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 50de2db. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ahejlsberg
removed
the
Experiment
|
@amcasey This improves material-ui memory usage by ~8% and performance by ~1%. |
amcasey
reviewed
Mar 6, 2020
| function createReplacementMapper(source: Type, target: Type, baseMapper: TypeMapper): TypeMapper { | ||
| return t => t === source ? target : baseMapper(t); | ||
| function addTypeMapping(mapper: TypeMapper | undefined, source: TypeParameter, target: Type) { | ||
| return mapper && mapper.kind === TypeMapKind.Simple && mapper.source2 === mapper.target2 ? |
There was a problem hiding this comment.
I'm getting mixed up. When you combine two maps, you look in the first map first and then, if you find something, do you stop looking or apply the second map to the first target? From the handling of composite maps in getMappedType it seems like it might be the latter, but this appears to do the former for unary maps?
There was a problem hiding this comment.
The particular test here is to see if the second source/target pair in the simple mapper is unused (recall, when source and target are the same, we have a no-op). If so, we create a new simple mapper with both source/target pairs in use.
One added twist with composite mappers is that the first mapper may map to some type that the second mapper further maps. For example, the first mapper might map from T to U[] and the second mapper from U to string. This also explains why we directly call the getMappedType with the first mapper, but then call instantiateType with the second one.
There was a problem hiding this comment.
I should add, in the addTypeMapping case, we actually don't care about the ability for the second mapping to affect the first mapping, which is why we can do the simple mapper optimization.
There was a problem hiding this comment.
So maps are combined by composition, but we happen to know that this particular composition is equivalent to concatenation?
src/compiler/checker.ts
Outdated
| case TypeMapKind.Array: | ||
| return makeArrayTypeMapper(baseMapper.sources, map(baseMapper.targets, (t, i) => baseMapper.sources[i] === source ? target : t)); | ||
| } | ||
| return makeFunctionTypeMapper(t => t === source ? target : getMappedType(t, baseMapper)); |
There was a problem hiding this comment.
Depending on how badly I've mixed up composite maps (see my question above), this seems like it might be equivalent to a composite map with source-to-target on the left hand side?
There was a problem hiding this comment.
Not quite. We don't want the second mapping to be applied to the result of the first. We basically just want to replace one of the mappings in the second mapper, which we can do by putting a check in front of the second mapper.
There was a problem hiding this comment.
How come this transformation (concatenation?) wasn't interesting enough to become a TypeMapKind?
|
@typescript-bot perf test this |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at ce9ddf3. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 30e7a18. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
weswigham
approved these changes
Mar 9, 2020
| } | ||
|
|
||
| function makeBinaryTypeMapper(source1: Type, target1: Type, source2: Type, target2: Type) { | ||
| return (t: Type) => t === source1 ? target1 : t === source2 ? target2 : t; | ||
| function getMappedType(type: Type, mapper: TypeMapper): Type { |
There was a problem hiding this comment.
I kinda-sorta wanna preemptively make this a loop rather than a recursive function to better optimize the recursive cases, but... it's not strictly required.
There was a problem hiding this comment.
I looked at that, but just makes the code more complex for no appreciable gain.
|
@weswigham Easier to just get rid of the |
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 PR switches our representation of type mappers from functions (that close over state) to objects. This brings greater transparency to the actual mappings performed by type mappers and enables more efficient construction of composite mappers.
The PR improves performance by up to 1% (with a 9% outlier on older versions of Node.js) and memory usage by up to 8%.