Cache mapper instantiations #61505
Cache mapper instantiations #61505
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
src/compiler/checker.ts
Outdated
| @@ -19960,7 +19961,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| } | |||
|
|
|||
| function makeFunctionTypeMapper(func: (t: Type) => Type, debugInfo: () => string): TypeMapper { | |||
| return Debug.attachDebugPrototypeIfDebug({ kind: TypeMapKind.Function, func, debugInfo: Debug.isDebugging ? debugInfo : undefined }); | |||
| const mapper = Debug.attachDebugPrototypeIfDebug({ kind: TypeMapKind.Function, func, debugInfo: Debug.isDebugging ? debugInfo : undefined }); | |||
| mapper.instantiations = voidMap; | |||
There was a problem hiding this comment.
those mappers are "global" and this is just a quick hack to prevent this caching mechanism to kick in on them
src/compiler/checker.ts
Outdated
| if (context) { | ||
| context.nonFixingMapper.instantiations = undefined; | ||
| } |
There was a problem hiding this comment.
nonFixingMapper isn't idempotent so whenever inferences are cleared it's instantiation cache has to be cleared too
|
@typescript-bot test it |
|
@typescript-bot test it |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot pack this |
|
Hey @jakebailey, something went wrong when looking for the build artifact. (You can check the log here). |
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
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"
type merge<base, props> = Omit<base, keyof props & keyof base> & props
declare const merge: <l, r>(l: l, r: r) => merge<l, r>
bench("functional(10)", () => {
const a = merge({ a: 1 }, { b: 2 })
const b = merge(a, { c: 3 })
const c = merge(b, { d: 4 })
const d = merge(c, { e: 5 })
const e = merge(d, { f: 6 })
const f = merge(e, { g: 7 })
const g = merge(f, { h: 8 })
const h = merge(g, { i: 9 })
const i = merge(h, { j: 10 })
}).types([59386, "instantiations"])
bench("functional(11)", () => {
const a = merge({ a: 1 }, { b: 2 })
const b = merge(a, { c: 3 })
const c = merge(b, { d: 4 })
const d = merge(c, { e: 5 })
const e = merge(d, { f: 6 })
const f = merge(e, { g: 7 })
const g = merge(f, { h: 8 })
const h = merge(g, { i: 9 })
const i = merge(h, { j: 10 })
const j = merge(i, { k: 11 })
}).types([177537, "instantiations"])
bench("functional(12)", () => {
const a = merge({ a: 1 }, { b: 2 })
const b = merge(a, { c: 3 })
const c = merge(b, { d: 4 })
const d = merge(c, { e: 5 })
const e = merge(d, { f: 6 })
const f = merge(e, { g: 7 })
const g = merge(f, { h: 8 })
const h = merge(g, { i: 9 })
const i = merge(h, { j: 10 })
const j = merge(i, { k: 11 })
const k = merge(j, { l: 12 })
// should be linear relative to functional(10) and functional(11)
}).types([531887, "instantiations"])
bench("functional(12) with spread", () => {
const a = merge({ a: 1 }, { b: 2 })
const b = merge(a, { c: 3 })
const c = merge(b, { d: 4 })
const d = merge(c, { e: 5 })
const e = merge(d, { f: 6 })
const f = merge(e, { g: 7 })
const g = merge(f, { h: 8 })
const h = merge(g, { i: 9 })
const i = merge(h, { j: 10 })
const j = merge(i, { k: 11 })
const k = merge(j, { l: 12 })
return {
...i,
...j,
...k
}
// should be similar to functional(12)
}).types([797849, "instantiations"])
type Type<t> = {
merge: <r>(r: r) => Type<merge<t, r>>
unwrapped: t
}
declare const a: Type<{ a: 1 }>
bench("chained(10)", () => {
const b = a.merge({ b: 2 })
const c = b.merge({ c: 3 })
const d = c.merge({ d: 4 })
const e = d.merge({ e: 5 })
const f = e.merge({ f: 6 })
const g = f.merge({ g: 7 })
const h = g.merge({ h: 8 })
const i = h.merge({ i: 9 })
const j = i.merge({ j: 10 })
}).types([178394, "instantiations"])
bench("chained(11)", () => {
const b = a.merge({ b: 2 })
const c = b.merge({ c: 3 })
const d = c.merge({ d: 4 })
const e = d.merge({ e: 5 })
const f = e.merge({ f: 6 })
const g = f.merge({ g: 7 })
const h = g.merge({ h: 8 })
const i = h.merge({ i: 9 })
const j = i.merge({ j: 10 })
const k = j.merge({ k: 11 })
}).types([532898, "instantiations"])
bench("chained(12)", () => {
const b = a.merge({ b: 2 })
const c = b.merge({ c: 3 })
const d = c.merge({ d: 4 })
const e = d.merge({ e: 5 })
const f = e.merge({ f: 6 })
const g = f.merge({ g: 7 })
const h = g.merge({ h: 8 })
const i = h.merge({ i: 9 })
const j = i.merge({ j: 10 })
const k = j.merge({ k: 11 })
const l = k.merge({ l: 12 })
// should be linear relative to chained(10) and chained(11)
}).types([1596004, "instantiations"])
bench("chained(12) with spread", () => {
const b = a.merge({ b: 2 })
const c = b.merge({ c: 3 })
const d = c.merge({ d: 4 })
const e = d.merge({ e: 5 })
const f = e.merge({ f: 6 })
const g = f.merge({ g: 7 })
const h = g.merge({ h: 8 })
const i = h.merge({ i: 9 })
const j = i.merge({ j: 10 })
const k = j.merge({ k: 11 })
const l = 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';
const t = initTRPC.create();
// avoid pollution from one-time library setup
bench.baseline(() => {
const router = 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}`),
});
});
const base = z.object({
a: z.string(),
b: z.string(),
c: z.string(),
});
bench('non-sequential Zod type', async () => {
const nonSequentialRouter = 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 operations
const sequential = base
.partial()
.merge(base)
.pick({ a: true, b: true, c: true })
.omit({})
.merge(base);
const base2 = z.object({
d: z.string(),
e: z.string(),
f: z.string(),
});
bench('isolated sequential zod', () => {
const sequential = 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 () => {
const sequentialRouter = 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']);
const arkBase = type({
a: 'string',
b: 'string',
c: 'string',
});
bench('non-sequential arktype', async () => {
const sequentialRouter = 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']);
const arkBase2 = 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']);
const arkSequential = arkBase
.partial()
.merge(arkBase)
.pick('a', 'b', 'c')
.omit()
.merge(arkBase);
bench('sequential arktype', async () => {
const sequentialRouter = 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']); |
Andarist
force-pushed
the
cache-mapper-instaniations
branch
from
688f5d5 to
b0f5c70
Compare
|
@ssalbdivad tested this in a trpc-based repo. TS 5.8: This branch (commit b0f5c70): |
|
@typescript-bot test it |
|
The new state seems less hacky for sure, though Map equality is unlikely to be portable to Go, so that's sort of a problem. @typescript-bot test it |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@jakebailey I don't think the portability should be a concern now. I can tweak this further or just handle it differently in the Go version. First, it would be great to get some eyes on this if this makes sense at all. Cause if it doesn't - what's even the point of code golfing this? ;p |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Just so we have something to test, is there a clear repo we could check this on? And not just a microbenchmark? It's a shame that no benchmark we have currently can target this. (xstate kinda?) |
|
@jakebailey from what I understand, @ssalbdivad found this when investigating a real trpc-based perf issue and he was able to narrow it down to this (TS playground): import { initTRPC } from '@trpc/server'; // types: 11.0.2
import { z } from 'zod'; // types: 3.24.2
const t = initTRPC.create();
const base = z.object({
a: z.string(),
b: z.string(),
c: z.string(),
});
const sequential = base
.partial()
.merge(base)
.pick({ a: true, b: true, c: true })
.omit({})
.merge(base);
// fast
const nonSequentialRouter = t.router({
query1: t.procedure.input(base).query(({ input }) => `hello ${input.a}`),
mutation1: t.procedure
.input(base)
.mutation(({ input }) => `hello ${input.a}`),
});
// equivalent, but slow
const sequentialRouter = t.router({
query1: t.procedure
.input(sequential)
.query(({ input }) => `hello ${input.a}`),
mutation1: t.procedure
.input(sequential)
.mutation(({ input }) => `hello ${input.a}`),
});Of course, this is a silly artificial example but it's a slimmed-down real thing. @ssalbdivad could comment on this further and maybe share more examples. |
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Andarist
changed the title
|
@Andarist's overview was good. The broader context is that these kind of sequentially derived Zod schemas are very common in the wild, especially in the repos I investigate due to type performance issues. What originally led me to this was two innocuous looking The Zod types themselves are not very efficient, but its only due to this exponential scaling issue that they are more than a blip. Given how easy this was to minimally reproduce and the fact that these chaining patterns are prevalent in some of the libraries most often responsible for significant type perf issues, I suspect it will drastically improve check timer for many other APIs in the ecosystem I may not be aware of. RE: the lack of existing benchmarks here, I think it points to a broader mismatch between the packages the team looks at most often for performance. It seems the focus is primarily large projects with many types but few generics like vscode and libraries like x-state which might define lots of generics but don't necessarily instantiate them a lot. Meanwhile, the most significant type performance issues almost always arise in large, end-user monorepos that instantiate an order of magnitude or more of these complex library types than libraries do themselves assuming their unit tests are included at all. I'm not sure what the best way to identify good candidates for these sort of benchmarks is as these projects are notoriously messy and volatile. Potentially just choosing the largest open-source applications to cover the most popular libraries including complex types like Zod, tRPC, Drizzle and Prisma would be a start? I just wrapped up some consulting for cal.com. They're probably a very good candidate for this sort of thing. |
|
I would like to be able to add a repo like you're describing to the standard benchmark suite, it's just a matter of knowing which one we can choose that will demonstrate this issue. If there's a specific tsconfig out in the wild, we can add it. (cal.com might itself be too large of a single project to add, but maybe there's something else?) |
I agree. If adding a cache on a mapper helps performance, it is an indication that something isn't being cached somewhere else. Mappers were never intended to be cache locations, but rather just type-to-type mapping functions. I'm in the middle of JSX porting work in the ts-go code base, but once I come up for air I'll try to take a look at this. |
|
Also, I should add, instantiation count isn't the most accurate account of computational work because it sits before the instantiation caches. IOW, we count instantiations coming out of a cache the same as instantiations that actually create new types. Type count is a much better indicator. |
I understand that - adding this cache, in the first place, felt quite dirty to me but it was the best shot I had at this.
I've examined this quite a bit, both before and after opening this PR, and I just couldn't find any other way to introduce some new caching elsewhere or to fix existing caches. I only found out that maybe That helped a little bit for type instantiations but at the expense of more types being created. That surprised me but I have not followed up on that because the idea has not proved to be sufficient to fix the original perf issues reported~ here.
I'm very eager to see what you can come up with here. As I've mentioned, at the beginning I treated this PR as a cheap shot at this problem. But after looking more and more into this - I just failed to recognize any other caching possibilities. Of course, I have much worse understanding of all the internal intricacies. I'm hopeful that some alternative for this can be found - it's just that I can't find them right now 😅 In the meantime, I've opened an alternative version to this PR here: #61535 . It still helps the original case but not as much. It might perform better when it comes to the consumed memory though.
Right, I only found out that it still takes a lot of work (and thus time) to get to those caches for computations that already happened in the past. In the presented |
|
@jakebailey You could add the AnswersOverflow repo I mentioned in my last response where I originally noticed the issue. @ahejlsberg To be clear, defining e.g. the same ArkType schema twice does not result in extra instantiations. I've tested both metrics fairly extensively in ArkType and across other real world projects and while neither types nor instantiations are linearly correlated with check time, both are granular, deterministic heuristics. Minimizing instantiations is almost always valuable, although some like those in this scenario are cheaper than others. It's also particularly good at identifying the scaling characteristics of a type. For example, the perfect match in the original case for O(3^N) made it very clear why the type gets so slow so fast as you add more operands. |
|
I think I was hoping for a repro via a repo I had heard of previously; AnswerOverflow itself uses some tooling that would make it hard to use as a direct repro, unfortunately. |
|
@jakebailey Unfortunately the end-user monorepos where these type performance issues are most likely to occur tend to be messier and also less likely to be public in general than the libraries that are typically tested. I do think it would still be highly beneficial to try and find some that are relatively stable/reliable to give TS some unique real-world perf insights. That said, I actually have some benchmarks and tests for these scenarios in ArkType (associated issue). For now, I have some of the longer chain/piping tests commented out because tsc chokes, but once this issue is addressed, I will uncomment them and you would see any regressions when typechecking ArkType. |
|
I'm working on the next version of my tRPC alternative Cuple. I can easily run into this type instantiation limit. I happily donate my repo for benchmarking https://github.com/fxdave/ts-deep-type-instantiation-example . It uses zod and express, but those are not requried, you can just copy-paste middlewares until it happens, or you can keep them and it happens right now in the |
|
Our benchmark resources are limited, so we usually go for "large established/continuing projects which exhibit a lot of behavior" rather than specific microbenchmarks; that's mainly why I was asking if there were any real-world projects out there that showed this, e.g. we use mui-docs, angular, xstate, as "somewhat large but interesting" cases. |
|
Wouldn't a popular repository that discovered this behavior as part of development, get rid of it in development/not allow that feature to merge as opposed to keeping it in there and making their userbase suffer it? This seems a bit of a self-fulfilling prophecy that bad performing behavior won't be accepted, so no one will use the bad performing behavior. |
|
You're not wrong, though that "AnswerOverflow" example does imply that there's likely some out there. I'm not trying to be difficult, it's just that every benchmark we add adds a bunch of CI time on a limited set of machines, so we try and pick good benchmarks. I certainly want to add something for this ASAP. |
|
The comment here is a clear indication that caches are ultimately hit to produce the types (since the type counts are the same), but also that the path to those caches is expensive and possibly merits an additional fronting cache. Are we dealing with instantiations of intersection types of increasing complexity here? |
Yes, the involved types are origin-less intersections |
@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.