More structural comparisons during variance computation #45628
Conversation
typescript-bot
added
the
For Milestone Bug
|
@typescript-bot perf test this |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at a1f82c1. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - main..45628
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Performance looks good, even slightly improved, likely because we're no longer jamming an |
|
The failed fourslash test appears to be a preexisting problem in the main branch. |
ahejlsberg
requested review from
andrewbranch,
DanielRosenwasser and
weswigham
src/compiler/checker.ts
Outdated
| function createTypeReference(target: GenericType, typeArguments: readonly Type[] | undefined): TypeReference { | ||
| const id = getTypeListId(typeArguments); | ||
| let type = target.instantiations.get(id); | ||
| if (!type) { | ||
| type = createObjectType(ObjectFlags.Reference, target.symbol) as TypeReference; | ||
| target.instantiations.set(id, type); | ||
| type.objectFlags |= typeArguments ? getPropagatingFlagsOfTypes(typeArguments, /*excludeKinds*/ 0) : 0; | ||
| type.objectFlags |= (typeArguments ? getPropagatingFlagsOfTypes(typeArguments, /*excludeKinds*/ 0) : 0) | | ||
| (containsMarkerType(typeArguments) ? ObjectFlags.MarkerType : 0); |
There was a problem hiding this comment.
Why not just add MarkerType to PropagatingFlags?
There was a problem hiding this comment.
I agree - it was a flag on the type/list before so we could avoid doing any kind of deep checks for markers and catch pretty much the exact type we initially make. It was a known shortcoming that we missed markers that flowed into other versions of the reference we're checking the variance of (known to me, at least - I thought it was an intentional performance tradeoff). This now catches if a Whatever<?, T> has a Whatever<T, ?> dependency (and forces structural comparison, rather than assuming covariance), but still misses a Whatever<{x: ?}, T> dependency. Shouldn't we go all the way and catch if the marker appears in the type argument list in any way?
There was a problem hiding this comment.
Ideally we'd identify any occurrence of a marker type, but it would require eager exploration which we know doesn't end well for many classes of types. Our variance measurement is effectively a balance between correctness and performance, and here we're moving a little further towards correctness. I know we've tried doing only structural comparisons when computing variance, but it isn't feasible. It just reveals the same problems with (almost) infinite types that we're trying to solve by doing variance measurement in the first place.
There was a problem hiding this comment.
We could add MarkerType to PropagatingFlags, but it wouldn't save us much and would require us to also include TypeFlags.TypeParameter in TypeFlags.ObjectFlagsType, which would mean an extra property on every instance that we otherwise don't need.
|
@typescript-bot test this |
|
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at 813b04b. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 813b04b. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 813b04b. You can monitor the build here. |
src/compiler/checker.ts
Outdated
| @@ -12901,13 +12901,18 @@ namespace ts { | |||
| return result & ObjectFlags.PropagatingFlags; | |||
| } | |||
|
|
|||
| function containsMarkerType(typeArguments: readonly Type[] | undefined) { | |||
| return some(typeArguments, t => t === markerSuperType || t === markerSubType || t === markerOtherType); | |||
There was a problem hiding this comment.
This check happens a few times - maybe just hoist it out to its own function and call it in reportUnmeasurableMarkers/reportUnreliableMarkers.
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
Would it be possible to generate a playground for this PR? |
|
I forget whether it checks out the head or merge commit—if the latter, it would need its merge conflicts fixed, but we can try. @typescript-bot pack this |
|
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 82c02a7. You can monitor the build here. |
|
@ahejlsberg @DanielRosenwasser did we have any big concerns over this or did we mostly forget about it? If we think we might want to merge it, let’s work on getting it in early for 4.7. |
# Conflicts: # src/compiler/checker.ts
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 2e859a2. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the diff-based community code test suite on this PR at 2e859a2. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 2e859a2. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 2e859a2. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - main..refs/pull/45628/merge TypeScript-Node-Starter1 of 1 projects failed to build with the old tsc /mnt/ts_downloads/TypeScript-Node-Starter/tsconfig.json
|
|
@andrewbranch could you request a new tarball? the spawned task has failed but that was executed before the recent merge, perhaps a fresh build after this merge would succeed |
|
@typescript-bot pack this |
|
Heya @ahejlsberg, I've started to run the tarball bundle task on this PR at 2e859a2. You can monitor the build here. |
|
@ahejlsberg Here they are:Comparison Report - main..45628
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @ahejlsberg, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
|
Yikes, the |
|
Hah, I'm actually XState maintainer 😅 In a way - I think that our types need a solid overhaul anyway. I wonder if there is anything in particular that we could take into consideration when refactoring, are there any particular things that we could try to avoid. While... we are flattered to be included in your perf test suite 😉 it's also a good sign that maybe we are overcomplicating things for the typechecker. Some recent changes to our types are in particular quite crazy (those designed to work with the typegen and have very unusual patterns used in them to "stitch" types together). My sincere hope is that this particular perf problem is not related to this part of the types as I took a loooot of time to get them working. Could you tell me against which version of XState are you running this benchmark? It would be useful for such information to be included somewhere in the result - as far as I can tell it's not in the comment of the bot nor in the attached benchmark file. When it comes to the variance itself - I'm pretty sure our types "get it wrong" (from the user perspective). While I understand the concept of variance and can "compute" variance type in simple cases, I have no idea how I should think about it with such complicated types as ours, with a lot of layers, a lot of generics etc. Are there any resources/comments from the past that could help me understand this? Are there any tricks that could help TS to determine the variance type "quicker"? On a more positive note - I've tested this build and it seems to fix my minimal repro case reported here and it also fixes the issue in our repo from which I've distilled this minimal repro. 🎉 |
|
@Andarist Thanks for taking such an interest in this! Our current instructions for profiling compilation times are at https://github.com/microsoft/TypeScript/wiki/Performance-Tracing and there are some other resources at https://github.com/microsoft/TypeScript/wiki/Performance The benchmarks run against whatever is currently checked into GitHub. We run against latest to avoid getting stale versions of projects which might not be taking advantage of newer language features. How to make a project build faster in general is outside the scope of this comment box, but I'd be happy to talk about it in a separate issue after you've run tracing and identified some hotspots. |
|
@RyanCavanaugh would you recommend performing this profiling using the latest TS or using the build from this PR? I'm unsure what's better from your perspective - for us to attempt to recognize current issues~ or the stuff specifically related to this PR?
Thanks, that's good to know - I would recommend adding information somewhere about the commit against which those profiling sessions were executed. It would ensure that we can always investigate exactly the same builds etc when any issues are recognized by this automatic benchmarking.
Thanks, I will certainly come back asking questions 🤣 I'll try to book some time in our next cycle to poke around this |
|
I might also have found a problem with this PR unless the goal of this PR is not to handle all of the possible cases. Are perhaps still situations when you might bail out from the process and assume that some types are assignable when they shouldn't? As far as I can tell the result of the
I can try to prepare a reduced test case but since that's sometimes up to a day of work I'd like to confirm with you that it's worth it. The issue can be observed on commit f256378e6 in xstate repo (this is a note for my own reference) |
|
Closing in favor of #48080. |
This PR fixes variance computation to perform structural comparisons for all type alias instantiations and object type instantiations with one or more variance markers as type arguments. Further discussion of the fix here.
Fixes #44572.