- Notifications
You must be signed in to change notification settings - Fork13.2k
Use objects instead of closures for type mappers#36576
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ahejlsberg commentedFeb 2, 2020
@typescript-bot perf test this |
typescript-bot commentedFeb 2, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@ahejlsberg, I've started to run the perf test suite on this PR at63338f9. You can monitor the buildhere. It should now contribute to this PR's status checks. Update:The results are in! |
typescript-bot commentedFeb 2, 2020
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ahejlsberg commentedFeb 2, 2020
@typescript-bot perf test this |
typescript-bot commentedFeb 2, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@ahejlsberg, I've started to run the perf test suite on this PR at979bc6a. You can monitor the buildhere. It should now contribute to this PR's status checks. Update:The results are in! |
typescript-bot commentedFeb 2, 2020
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ahejlsberg commentedFeb 29, 2020
@typescript-bot perf test this |
typescript-bot commentedFeb 29, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@ahejlsberg, I've started to run the perf test suite on this PR at979bc6a. You can monitor the buildhere. Update:The results are in! |
typescript-bot commentedFeb 29, 2020
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ahejlsberg commentedMar 5, 2020
@typescript-bot perf test this |
typescript-bot commentedMar 5, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@ahejlsberg, I've started to run the perf test suite on this PR atc7d3806. You can monitor the buildhere. Update:The results are in! |
typescript-bot commentedMar 5, 2020
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ahejlsberg commentedMar 5, 2020
@typescript-bot perf test this |
typescript-bot commentedMar 5, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@ahejlsberg, I've started to run the perf test suite on this PR at61f0c7a. You can monitor the buildhere. Update:The results are in! |
typescript-bot commentedMar 5, 2020
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ahejlsberg commentedMar 6, 2020
@typescript-bot perf test this |
typescript-bot commentedMar 6, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@ahejlsberg, I've started to run the perf test suite on this PR at50de2db. You can monitor the buildhere. Update:The results are in! |
typescript-bot commentedMar 6, 2020
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ahejlsberg commentedMar 6, 2020
@amcasey This improves material-ui memory usage by ~8% and performance by ~1%. |
amcasey left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Some questions for my own edification.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/compiler/checker.ts Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 ingetMappedType 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 fromT toU[] and the second mapper fromU tostring. This also explains why we directly call thegetMappedType with the first mapper, but then callinstantiateType with the second one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I should add, in theaddTypeMapping 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 withsource-to-target on the left hand side?
ahejlsbergMar 7, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How come this transformation (concatenation?) wasn't interesting enough to become aTypeMapKind?
ahejlsberg commentedMar 8, 2020
@typescript-bot perf test this |
typescript-bot commentedMar 8, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@ahejlsberg, I've started to run the perf test suite on this PR atce9ddf3. You can monitor the buildhere. Update:The results are in! |
typescript-bot commentedMar 8, 2020
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ahejlsberg commentedMar 8, 2020
@typescript-bot perf test this |
typescript-bot commentedMar 8, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@ahejlsberg, I've started to run the perf test suite on this PR at30e7a18. You can monitor the buildhere. Update:The results are in! |
typescript-bot commentedMar 8, 2020
@ahejlsberg Here they are:Comparison Report - master..36576
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
weswigham left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does it make sense to recognize theidentityMapper singleton in prepend/append/merge and short-circuit the construction of a composite mapper, if it's found?
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I looked at that, but just makes the code more complex for no appreciable gain.
ahejlsberg commentedMar 10, 2020
@weswigham Easier to just get rid of the |
Uh oh!
There was an error while loading.Please reload this page.
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%.