Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed single signature type instantiations leaking between calls with different mappers #59972

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Sep 15, 2024

fixes #59937 , cc @weswigham

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Sep 15, 2024
Comment on lines +43 to +46
// the first one here has a chance to pollute the cache
type Result1 = ComponentProps<typeof ComponentWithForwardRef>;
// that could be incorrectly reused by this one
type Result2 = Test<{ component: typeof ComponentWithForwardRef }>; // no `T` leak
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the process of resolving the first conditional type here a result with a conditional type's restrictive mapper was cached, then when resolving the second conditional type that cached result was reused incorrectly, failing to produce a type with its correct mapper applied to it

Comment on lines -20254 to -20258
if (type.objectFlags & ObjectFlags.SingleSignatureType) {
result = instantiateAnonymousType(type, mapper);
target.instantiations.set(id, result);
return result;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might feel like a downgrade that those are not cached right now. getObjectTypeInstantiation (the containing function) is only called from a single place and all the functions called from the branch leading to getObjectTypeInstantiation are implementing instantiation caching (createNormalizedTypeReference, instantiateReverseMappedType and getObjectTypeInstantiation).

But now not the branch I added - the one that calls instantiateAnonymousType for single signature types. I have no idea what I would use to create the cache key there. The only unique bits there are the results of the mapper but mapper isn't exactly called eagerly there so it doesn't have access to those.

However, I have a feeling that perhaps caching is already - sort of - implemented for those single signature types. It's just at a different level, like in the getReturnTypeOfSignature.

@jakebailey
Copy link
Member

@typescript-bot test it

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 16, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
test top400 ✅ Started ✅ Results
user test this ✅ Started ✅ Results
run dt ✅ Started ✅ Results
perf test this faster ✅ Started 👀 Results

@typescript-bot
Copy link
Collaborator

@jakebailey
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-Unions - node (v18.15.0, x64)
Errors 30 30 ~ ~ ~ p=1.000 n=6
Symbols 62,153 62,153 ~ ~ ~ p=1.000 n=6
Types 50,242 50,242 ~ ~ ~ p=1.000 n=6
Memory used 193,620k (± 0.93%) 193,083k (± 0.73%) ~ 192,403k 195,958k p=0.810 n=6
Parse Time 1.57s (± 1.12%) 1.56s (± 1.02%) ~ 1.54s 1.58s p=0.801 n=6
Bind Time 0.86s (± 0.95%) 0.86s (± 1.20%) ~ 0.85s 0.87s p=0.928 n=6
Check Time 11.42s (± 0.36%) 11.39s (± 0.47%) ~ 11.31s 11.45s p=0.572 n=6
Emit Time 3.23s (± 0.51%) 3.22s (± 0.75%) ~ 3.18s 3.24s p=0.742 n=6
Total Time 17.07s (± 0.23%) 17.03s (± 0.36%) ~ 16.92s 17.08s p=0.229 n=6
angular-1 - node (v18.15.0, x64)
Errors 7 7 ~ ~ ~ p=1.000 n=6
Symbols 945,753 945,753 ~ ~ ~ p=1.000 n=6
Types 410,067 410,067 ~ ~ ~ p=1.000 n=6
Memory used 1,222,733k (± 0.00%) 1,222,744k (± 0.00%) ~ 1,222,683k 1,222,829k p=0.872 n=6
Parse Time 7.89s (± 0.70%) 7.88s (± 0.51%) ~ 7.82s 7.92s p=0.565 n=6
Bind Time 2.22s (± 0.18%) 2.22s (± 0.62%) ~ 2.19s 2.23s p=0.924 n=6
Check Time 36.38s (± 0.38%) 36.40s (± 0.22%) ~ 36.28s 36.53s p=1.000 n=6
Emit Time 17.75s (± 0.56%) 17.79s (± 0.35%) ~ 17.68s 17.85s p=0.810 n=6
Total Time 64.24s (± 0.31%) 64.28s (± 0.17%) ~ 64.15s 64.42s p=0.748 n=6
mui-docs - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 2,520,527 2,520,527 ~ ~ ~ p=1.000 n=6
Types 935,974 935,974 ~ ~ ~ p=1.000 n=6
Memory used 2,350,484k (± 0.00%) 2,350,498k (± 0.00%) ~ 2,350,477k 2,350,516k p=0.810 n=6
Parse Time 13.59s (± 0.34%) 13.59s (± 0.36%) ~ 13.53s 13.65s p=0.936 n=6
Bind Time 3.16s (± 0.55%) 3.16s (± 0.65%) ~ 3.12s 3.18s p=0.935 n=6
Check Time 105.85s (± 0.40%) 106.05s (± 0.55%) ~ 105.20s 106.77s p=0.689 n=6
Emit Time 0.39s (± 3.49%) 0.38s (± 2.17%) ~ 0.38s 0.40s p=0.197 n=6
Total Time 122.98s (± 0.34%) 123.18s (± 0.47%) ~ 122.32s 123.97s p=0.630 n=6
self-build-src - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,245,952 1,245,952 ~ ~ ~ p=1.000 n=6
Types 264,213 264,212 -1 (- 0.00%) ~ ~ p=0.001 n=6
Memory used 2,397,537k (± 0.02%) 2,399,250k (± 0.10%) +1,713k (+ 0.07%) 2,397,663k 2,404,213k p=0.031 n=6
Parse Time 7.61s (± 0.52%) 7.61s (± 0.31%) ~ 7.58s 7.64s p=0.574 n=6
Bind Time 2.80s (± 0.64%) 2.80s (± 0.66%) ~ 2.79s 2.84s p=0.572 n=6
Check Time 51.07s (± 0.42%) 50.78s (± 0.73%) ~ 50.36s 51.23s p=0.173 n=6
Emit Time 4.46s (± 1.36%) 4.37s (± 1.39%) -0.09s (- 1.98%) 4.26s 4.42s p=0.037 n=6
Total Time 65.95s (± 0.32%) 65.57s (± 0.64%) ~ 65.09s 66.04s p=0.093 n=6
self-build-src-public-api - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,245,952 1,245,952 ~ ~ ~ p=1.000 n=6
Types 264,213 264,212 -1 (- 0.00%) ~ ~ p=0.001 n=6
Memory used 2,472,025k (± 0.04%) 2,471,595k (± 0.03%) ~ 2,470,998k 2,473,192k p=0.575 n=6
Parse Time 6.27s (± 0.44%) 6.28s (± 0.38%) ~ 6.25s 6.31s p=0.520 n=6
Bind Time 2.02s (± 0.52%) 2.03s (± 0.37%) ~ 2.02s 2.04s p=1.000 n=6
Check Time 41.51s (± 0.66%) 41.45s (± 0.79%) ~ 41.05s 41.90s p=0.936 n=6
Emit Time 3.57s (± 1.71%) 3.71s (± 2.95%) +0.14s (+ 3.92%) 3.61s 3.88s p=0.013 n=6
Total Time 53.39s (± 0.60%) 53.48s (± 0.44%) ~ 53.20s 53.89s p=0.689 n=6
self-compiler - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 259,885 259,885 ~ ~ ~ p=1.000 n=6
Types 106,140 106,139 -1 (- 0.00%) ~ ~ p=0.001 n=6
Memory used 435,572k (± 0.01%) 435,567k (± 0.02%) ~ 435,449k 435,643k p=1.000 n=6
Parse Time 2.84s (± 0.61%) 2.85s (± 0.62%) ~ 2.82s 2.87s p=0.803 n=6
Bind Time 1.08s (± 1.12%) 1.07s (± 1.13%) ~ 1.06s 1.09s p=0.070 n=6
Check Time 15.39s (± 0.26%) 15.37s (± 0.25%) ~ 15.30s 15.40s p=0.260 n=6
Emit Time 1.30s (± 0.90%) 1.28s (± 1.54%) -0.02s (- 1.92%) 1.25s 1.30s p=0.040 n=6
Total Time 20.61s (± 0.29%) 20.55s (± 0.20%) ~ 20.51s 20.61s p=0.092 n=6
ts-pre-modules - node (v18.15.0, x64)
Errors 68 68 ~ ~ ~ p=1.000 n=6
Symbols 225,018 225,018 ~ ~ ~ p=1.000 n=6
Types 94,249 94,249 ~ ~ ~ p=1.000 n=6
Memory used 370,265k (± 0.02%) 370,252k (± 0.02%) ~ 370,167k 370,337k p=0.810 n=6
Parse Time 2.28s (± 0.60%) 2.28s (± 0.71%) ~ 2.26s 2.31s p=0.867 n=6
Bind Time 1.31s (± 0.57%) 1.31s (± 2.31%) ~ 1.29s 1.37s p=0.187 n=6
Check Time 13.37s (± 0.29%) 13.38s (± 0.22%) ~ 13.35s 13.43s p=0.808 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 16.96s (± 0.26%) 16.97s (± 0.30%) ~ 16.93s 17.07s p=1.000 n=6
vscode - node (v18.15.0, x64)
Errors 1 1 ~ ~ ~ p=1.000 n=6
Symbols 3,067,616 3,067,616 ~ ~ ~ p=1.000 n=6
Types 1,059,253 1,059,253 ~ ~ ~ p=1.000 n=6
Memory used 3,173,020k (± 0.00%) 3,173,042k (± 0.00%) ~ 3,172,980k 3,173,111k p=0.936 n=6
Parse Time 13.91s (± 0.41%) 13.91s (± 0.09%) ~ 13.90s 13.93s p=0.467 n=6
Bind Time 4.42s (± 2.61%) 4.39s (± 2.08%) ~ 4.35s 4.58s p=0.366 n=6
Check Time 81.61s (± 0.40%) 81.64s (± 0.28%) ~ 81.29s 81.91s p=0.936 n=6
Emit Time 22.25s (± 0.33%) 22.30s (± 0.44%) ~ 22.18s 22.41s p=0.470 n=6
Total Time 122.19s (± 0.36%) 122.25s (± 0.25%) ~ 121.79s 122.60s p=0.810 n=6
webpack - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 277,156 277,156 ~ ~ ~ p=1.000 n=6
Types 112,946 112,946 ~ ~ ~ p=1.000 n=6
Memory used 427,007k (± 0.04%) 427,014k (± 0.01%) ~ 426,966k 427,107k p=0.630 n=6
Parse Time 4.92s (± 0.46%) 4.91s (± 0.36%) ~ 4.88s 4.93s p=0.372 n=6
Bind Time 2.14s (± 1.04%) 2.12s (± 0.97%) ~ 2.08s 2.14s p=0.063 n=6
Check Time 21.84s (± 0.40%) 21.77s (± 0.50%) ~ 21.62s 21.89s p=0.376 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 28.90s (± 0.18%) 28.79s (± 0.39%) ~ 28.67s 28.95s p=0.106 n=6
xstate-main - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 531,582 531,582 ~ ~ ~ p=1.000 n=6
Types 181,676 181,676 ~ ~ ~ p=1.000 n=6
Memory used 463,694k (± 0.00%) 463,747k (± 0.02%) ~ 463,666k 463,879k p=0.065 n=6
Parse Time 2.61s (± 0.51%) 2.60s (± 0.49%) ~ 2.59s 2.62s p=0.318 n=6
Bind Time 0.93s 0.93s (± 0.44%) ~ 0.93s 0.94s p=0.405 n=6
Check Time 15.45s (± 0.50%) 15.44s (± 0.39%) ~ 15.36s 15.52s p=0.688 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 18.98s (± 0.44%) 18.97s (± 0.33%) ~ 18.88s 19.04s p=0.873 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Compiler-Unions - node (v18.15.0, x64)
  • angular-1 - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-build-src-public-api - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • ts-pre-modules - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate-main - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user tests with tsc comparing main and refs/pull/59972/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top 400 repos with tsc comparing main and refs/pull/59972/merge:

Everything looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Status: Not started
Development

Successfully merging this pull request may close these issues.

React's ComponentProps type issues in TypeScript 5.6.2
3 participants