-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
TS compilation perf: faster objectUtil.addQuestionMarks #2845
TS compilation perf: faster objectUtil.addQuestionMarks #2845
Conversation
✅ Deploy Preview for guileless-rolypoly-866f8a ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
interesting, how did you do this benchmark? how can I reproduce? |
I'm basically just benchmarking it against one of the TS APIs at work that uses Zod quite heavily. Would probably be good to have some public I'm having trouble getting any perf difference between these two implementations on just a few ZodObjects, but in my real world project it's quite clear as you can tell from the numbers cited in the OP |
Script to generate a bunch of zod object schemas and .omit() & .extend() modifications for each:
Generate a file, then Importantly: a meaningful amount of perf degradation only occurs when using .omit() and .extend(), but when you do, the difference is stark. |
Turns out it was even possible to remove the Beyond this PR, it might be a good idea to build a separate compilation performance regression test suite for Zod. Something like the type generation script above (albeit cleaned up) might serve as a starting point. |
I made a really braindead-simple benchmarking repo, some test runs here: |
On zod 3.22.4, this patch breaks a ton of types. |
Could you provide some example types so I can modify the PR accordingly? Those examples could be added as type level regression tests. |
Sure. The issues are in schemas composed of a |
I apologize, your PR works fine. I cherry-picked your commits onto upstream master and used that build successfully. My problem was that I patched the build artefact directly with |
Was able to reduce the # of type instantiations some more by baking in flattening to |
Although apparently it fails on newer TS versions... the project version is 4.5.x EDIT: reverted. |
076e70a
to
29e0bc8
Compare
I'm not sure what the precise reasons for this being faster are, but consistently benchmarking in my project about 50% more type instantiations with the original version vs. the one proposed in this commit; plus the compilation time is 20% longer in the original.
The previous commit contained some pseudo-optimizations that had no realworld impact.
Turns out requiredKeys is not necessary and filtering out optional keys in the mapped type's key filtering does the same thing. This also further reduces the amount of type instantiations which slightly improves performance.
29e0bc8
to
50dcc45
Compare
Thanks! I'd added some additional tests in I also threw a simplified version of Amazing stuff @jussisaurio!!! |
This has landed in Zod 3.23. https://github.com/colinhacks/zod/releases/tag/v3.23.0 |
And...it broke JSDoc #3437 Here's where I'm at in my investigations: // slow (60% more instantiations), but preserves JSDoc
type extendShape2<A extends object, B extends object> = Pick<
A,
Exclude<keyof A, keyof B>
> & B;
// fast, but JSDoc is lost
type extendShape1<A extends object, B extends object> = {
[K in keyof A | keyof B]: K extends keyof B
? B[K]
: K extends keyof A
? A[K]
: never;
};
// fast & preserves JSDoc! doesn't reduce object to simplest form
type extendShape3<A extends object, B extends object> = {
[K in Exclude<keyof A, keyof B>]: A[K];
} & B; |
Iteration 1: export type extendShape<A extends object, B extends object> = {
[K in keyof A]: K extends keyof B ? never : A[K];
} & {
[K in keyof B]: B[K];
}; This seems to preserve the JSDocs at least with the example given in #3437, and also seems to create (slightly) less type instantiations as well. However, it fails a few existing tests, eg. test("test inferred merged type", async () => {
const asdf = z.object({ a: z.string() }).merge(z.object({ a: z.number() }));
type asdf = z.infer<typeof asdf>;
util.assertEqual<asdf, { a: number }>(true); // fail
}); Iteration 2: export type extendShape<A extends object, B extends object> = {
[K in keyof A as K extends keyof B ? never : K]: A[K];
} & {
[K in keyof B]: B[K];
}; This passes all existing tests, however increases instantiations by 8.5% (380k vs 350k in my test-case using zod-ts-perftest). This may be a tolerable compromise? |
I'm not sure what the precise reasons for this being faster are, but consistently benchmarking in my project about 50% more type instantiations with the original version vs. the one proposed in this commit; plus the compilation time is 20% longer in the original.
EDIT: check out this issue comment from a
tsc
maintainerBefore:
After: