Cache complex union and intersection relations #37910
Merged
Conversation
This file contains hidden or 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 1cd5b0a. You can monitor the build here. Update: The results are in! |
ahejlsberg
changed the title
|
@ahejlsberg Here they are:Comparison Report - master..37910
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Not much effect on our perf test suites, so not clear how valuable this is. But it definitely has a pretty positive effect on the compiler with unions code base. |
|
I'm surprised there's not a more pronounced effect on Material UI but I guess it makes sense |
sandersn
added
the
For Uncommitted Bug
weswigham
approved these changes
Aug 27, 2020
There was a problem hiding this comment.
I don't remember if we chatted about this back in April, but I'm still assigned, so 🤷. It'd be nice to enable the compiler with unions perf test (IIRC it was disabled because it caused an older version of the compiler to crash, but that shouldn't be an issue right now - cc @rbuckton - you know if/how we can turn it on?) to show the beneficial effect this actually has, but this seems OK, implementation-wise.
|
@rbuckton know if we can go about enabling that perf test at some point (and get some historical data for it)? @ahejlsberg we going to merge this at some point? Should it have a milestone? It looks like it needs a merge for a baseline update. |
# Conflicts: # tests/baselines/reference/classPropertyErrorOnNameOnly.errors.txt
Zzzen
pushed a commit
to Zzzen/TypeScript
that referenced
this pull request
Jan 16, 2021
* Cache complex union/intersection relations * Accept new baselines * Accept new baselines
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.
We've discussed caching union and intersection relations before, but had concerns about the increased memory usage. This PR introduces caching for complex union and intersection relations, specifically those than have a combined number of constituents of at least 4. This ideally strikes a balance between time and memory.
This PR improves check time for our "compiler with unions" code base by about 12%.