Cache mapper instantiations#61505
Merged
Cache mapper instantiations #61505
+5,474 −81
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 added the For Uncommitted Bug 
Collaborator
typescript-bot commented Mar 30, 2025
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
Andarist commented Mar 30, 2025
Uh oh!
There was an error while loading. Please reload this page.
Andarist commented Mar 30, 2025
Uh oh!
There was an error while loading. Please reload this page.
Member
jakebailey commented Mar 30, 2025
@typescript-bot test it |
Collaborator
Member
jakebailey commented Mar 31, 2025
@typescript-bot test it |
Collaborator
Collaborator
typescript-bot commented Mar 31, 2025
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
Collaborator
typescript-bot commented Mar 31, 2025
@jakebailey Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
Collaborator
typescript-bot commented Mar 31, 2025
@jakebailey Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Member
jakebailey commented Mar 31, 2025
@typescript-bot pack this |
Collaborator
Collaborator
typescript-bot commented Mar 31, 2025
Hey @jakebailey, something went wrong when looking for the build artifact. (You can check the log here). |
Collaborator
typescript-bot commented Mar 31, 2025
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
ssalbdivad commented Apr 1, 2025
This is a set of instantiation benchmarks with results from These can be run against the installed TS version by executing this with import{bench}from"@ark/attest"typemerge<base,props>=Omit<base,keyofprops&keyofbase>&propsdeclareconstmerge: <l,r>(l: l,r: r)=>merge<l,r>bench("functional(10)",()=>{consta=merge({a: 1},{b: 2})constb=merge(a,{c: 3})constc=merge(b,{d: 4})constd=merge(c,{e: 5})conste=merge(d,{f: 6})constf=merge(e,{g: 7})constg=merge(f,{h: 8})consth=merge(g,{i: 9})consti=merge(h,{j: 10})}).types([59386,"instantiations"])bench("functional(11)",()=>{consta=merge({a: 1},{b: 2})constb=merge(a,{c: 3})constc=merge(b,{d: 4})constd=merge(c,{e: 5})conste=merge(d,{f: 6})constf=merge(e,{g: 7})constg=merge(f,{h: 8})consth=merge(g,{i: 9})consti=merge(h,{j: 10})constj=merge(i,{k: 11})}).types([177537,"instantiations"])bench("functional(12)",()=>{consta=merge({a: 1},{b: 2})constb=merge(a,{c: 3})constc=merge(b,{d: 4})constd=merge(c,{e: 5})conste=merge(d,{f: 6})constf=merge(e,{g: 7})constg=merge(f,{h: 8})consth=merge(g,{i: 9})consti=merge(h,{j: 10})constj=merge(i,{k: 11})constk=merge(j,{l: 12})// should be linear relative to functional(10) and functional(11)}).types([531887,"instantiations"])bench("functional(12) with spread",()=>{consta=merge({a: 1},{b: 2})constb=merge(a,{c: 3})constc=merge(b,{d: 4})constd=merge(c,{e: 5})conste=merge(d,{f: 6})constf=merge(e,{g: 7})constg=merge(f,{h: 8})consth=merge(g,{i: 9})consti=merge(h,{j: 10})constj=merge(i,{k: 11})constk=merge(j,{l: 12})return{ ...i, ...j, ...k}// should be similar to functional(12)}).types([797849,"instantiations"])typeType<t>={merge: <r>(r: r)=>Type<merge<t,r>>unwrapped: t}declareconsta: Type<{a: 1}>bench("chained(10)",()=>{constb=a.merge({b: 2})constc=b.merge({c: 3})constd=c.merge({d: 4})conste=d.merge({e: 5})constf=e.merge({f: 6})constg=f.merge({g: 7})consth=g.merge({h: 8})consti=h.merge({i: 9})constj=i.merge({j: 10})}).types([178394,"instantiations"])bench("chained(11)",()=>{constb=a.merge({b: 2})constc=b.merge({c: 3})constd=c.merge({d: 4})conste=d.merge({e: 5})constf=e.merge({f: 6})constg=f.merge({g: 7})consth=g.merge({h: 8})consti=h.merge({i: 9})constj=i.merge({j: 10})constk=j.merge({k: 11})}).types([532898,"instantiations"])bench("chained(12)",()=>{constb=a.merge({b: 2})constc=b.merge({c: 3})constd=c.merge({d: 4})conste=d.merge({e: 5})constf=e.merge({f: 6})constg=f.merge({g: 7})consth=g.merge({h: 8})consti=h.merge({i: 9})constj=i.merge({j: 10})constk=j.merge({k: 11})constl=k.merge({l: 12})// should be linear relative to chained(10) and chained(11)}).types([1596004,"instantiations"])bench("chained(12) with spread",()=>{constb=a.merge({b: 2})constc=b.merge({c: 3})constd=c.merge({d: 4})conste=d.merge({e: 5})constf=e.merge({f: 6})constg=f.merge({g: 7})consth=g.merge({h: 8})consti=h.merge({i: 9})constj=i.merge({j: 10})constk=j.merge({k: 11})constl=k.merge({l: 12})return{ ...i.unwrapped, ...j.unwrapped, ...k.unwrapped}// should be similar to chained(12)}).types([1684778,"instantiations"])I would expect the instantiation scaling to not be purely linear for this sort of operation as the number of props is increasing, but it should be relatively close. None of these scenarios should result in the exponential scaling we see currently. I'm also including the original end-to-end type benchmarks that identified this issue here. Once the minimal repo scaling has been addressed, we should sanity check that the scaling here is also fixed: import{bench}from'@ark/attest';import{initTRPC}from'@trpc/server';import{type}from'arktype';import{z}from'zod';constt=initTRPC.create();// avoid pollution from one-time library setupbench.baseline(()=>{constrouter=t.router({baseline: t.procedure.input(z.object({baseline: z.string(),}),).query(({ input })=>`hello ${input.baseline}`),arkBaseline: t.procedure.input(type({baseline: 'string',}),).query(({ input })=>`hello ${input.baseline}`),});});constbase=z.object({a: z.string(),b: z.string(),c: z.string(),});bench('non-sequential Zod type',async()=>{constnonSequentialRouter=t.router({query1: t.procedure.input(base).query(({ input })=>`hello ${input.a}`),mutation1: t.procedure.input(base).mutation(({ input })=>`hello ${input.a}`),});// this is relatively cheap}).types([1659,'instantiations']);// even though sequential is totally equivalent to nonSequential, its// Zod representation is not reduced and still includes intermediate operationsconstsequential=base.partial().merge(base).pick({a: true,b: true,c: true}).omit({}).merge(base);constbase2=z.object({d: z.string(),e: z.string(),f: z.string(),});bench('isolated sequential zod',()=>{constsequential=base2.partial().merge(base2).pick({d: true,e: true,f: true}).omit({}).merge(base2);// this is expensive}).types([11420,'instantiations']);bench('sequential Zod type',async()=>{constsequentialRouter=t.router({query1: t.procedure.input(sequential).query(({ input })=>`hello ${input.a}`),mutation1: t.procedure.input(sequential).mutation(({ input })=>`hello ${input.a}`),});// but it's in combination with trpc that these sequentially evaluated// Zod types get out of control. instead of incurring a 1-time evaluation// cost, it seems it can't be cached and the extra inference cost// is incurred multiple times (even worse with deepPartial)}).types([49173,'instantiations']);constarkBase=type({a: 'string',b: 'string',c: 'string',});bench('non-sequential arktype',async()=>{constsequentialRouter=t.router({query1: t.procedure.input(arkBase).query(({ input })=>`hello ${input.a}`),mutation1: t.procedure.input(arkBase).mutation(({ input })=>`hello ${input.a}`),});// realtively cheap}).types([2961,'instantiations']);constarkBase2=type({d: 'string',e: 'string',f: 'string',});bench('isolated sequential arktype',()=>{arkBase2.partial().merge(arkBase2).pick('d','e','f').omit().merge(arkBase2);// these kind of operations are much cheaper in ArkType than Zod}).types([2336,'instantiations']);constarkSequential=arkBase.partial().merge(arkBase).pick('a','b','c').omit().merge(arkBase);bench('sequential arktype',async()=>{constsequentialRouter=t.router({query1: t.procedure.input(arkSequential).query(({ input })=>`hello ${input.a}`),mutation1: t.procedure.input(arkSequential).mutation(({ input })=>`hello ${input.a}`),});// even though hovering arkSequential is identical to hovering arkBase,// TS still seems to do a lot of repeated work inferring it somehow (though less than Zod)}).types([17906,'instantiations']); |
Andaristforce-pushed the cache-mapper-instaniations branch from 688f5d5 to b0f5c70Compare
ContributorAuthor
Andarist commented Apr 1, 2025
@ssalbdivad tested this in a trpc-based repo. TS 5.8: This branch (commit b0f5c70): |
Member
jakebailey commented Apr 1, 2025
@typescript-bot test it |
ssalbdivad commented Apr 18, 2025
@ahejlsberg This What concerns me is that whether or not individual instantiations are expensive, if there are O(3^N) of them, they will eventually dominate the check time of the entire type. IMO given @Andarist's second PR has no clear downsides in terms of check time or memory consumption, it seems worthwhile to just eliminate the logic that could lead to this scaling altogether. |
ContributorAuthor
Andarist commented Apr 21, 2025
This is a valid concern 👍 I'll see what I can do to address it |
Member
ahejlsberg commented Apr 21, 2025
You might experiment with clearing all caches whenever a new inference is made. If that doesn't adversely affect performance I think that might be the best option. Also, as I mentioned earlier, you can remove some overhead by only caching results of union and intersection instantiations. |
1 task
Member
jakebailey commented May 2, 2025
@typescript-bot test it |
Collaborator
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
Collaborator
typescript-bot commented May 2, 2025
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
Collaborator
typescript-bot commented May 2, 2025
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
Collaborator
typescript-bot commented May 2, 2025
@jakebailey Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Collaborator
typescript-bot commented May 2, 2025
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
ContributorAuthor
I've followed this advice and the numbers are still pretty much the same as the ones I mentioned here. Instantiations went up a little bit but by such a negligible number (+341), it doesn't matter at all.
The current version doesn't come with any noticeable overhead, from what I can tell. It seems to me doing this just for union/intersections would only complicate the code right now. If you feel strongly about it though, I could certainly run an extra experiment like this. |
jakebailey approved these changes Jun 5, 2025
Member
jakebailey left a comment
jakebailey left a comment There was a problem hiding this comment.
So, I feel like this must be good to go at this point. The code looks pretty simple, the benchmarks are good, and it should be straightforward to port into the Go codebase (eventually) as there aren't any goofy uses of maps / prototype modification anymore.
Are there any remaining concerns here?
jakebailey merged commit 51dcd90 into microsoft:main 32 checks passed
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jun 30, 2025
This was referenced Jul 3, 2025
mikeduminy commented Jul 22, 2025
This was a game-changer PR for us. Thank you!
|
Member
jakebailey commented Jul 22, 2025
What repo is that? Is it public? |
It's a private repo unfortunately. It is for the Klarna app (react-native, web, etc). But we'd be happy to test out anything internally and get back to you if want 😄 |
Member
I want a public repo for our benchmarks, unfortunately |
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
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.
@ssalbdivad found out that a chain of somewhat trivial operations can easily end up with dreaded "Type instantiation is excessively deep and possibly infinite".
Two different reproductions were created:
We were able to bisect both to two different past PRs - one of which is quite old:
⚠️ that said - the "chain setup" avoids the error better but the property type lookup can still lead to the error. So, in some sense, the error just shifted place.I dug into this and what I found out is, those types refer to the same "base" types - they are chained from them and
instantiateTyperepeats the work on them a lot. At least part of the problem is thatgetObjectTypeInstantiationcallsinstantiateTypes(type.aliasTypeArguments, mapper)to create part of its cache key. So it can't even check if the result is cached before it instantiates those. This problem compounds heavily in longer chains like this.So I toyed with possible solutions and I realized that caching results on mappers could help here... to some extent at least. I don't think it's the best solution but I also don't yet understand this problem to its core to propose anything better.
It looks like this has very positive impact on instantiation counts across the test suite. It surely trades some memory for it though. It would be interesting to see perf results for this and the extended test suite run (even if only to get new data points for further investigation).
EDIT:// @ssalbdivad has tested this in some trpc-based repositories and the check time got halved for them. The current version is also able to typecheck the 50-item deep "chains" as it can be seen in the added
long*tests. Those previously were only instantiable to a certain depth (smth between 15-25, depending on the test case) and the last items in the chain were being instantiated really slow. Now those longer ones are handled pretty fast.