microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.04k stars 12.49k forks source link

[Build Performance] Including `yup` adds 3 seconds to lib type-checking #46856

Open berickson1 opened 2 years ago

berickson1 commented 2 years ago

Bug Report

In our (private) typescript project that utilizes composite projects, we've been struggling with increasing compile times as more projects have been added. I took a stab at investigating this based on the Performance Tracing Docs. Ultimately I found that consuming yup adds ~3s to each package build (on an M1 MBP). Given we have 100+ composite projects in the repo (not all of which are impacted, since not all import yup), this starts to stack up as a non-trivial increase to build times.

This can be mitigated by setting skipLibCheck to true, but ideally this would be false for complete validation of downstream types.

🔎 Search Terms

performance yup lib type checking

🕗 Version & Regression Information

⏯ Playground Link

Playground link with relevant code The above playground link isn't overly useful given this is a performance bug, I've also prepared a minimal repro. https://github.com/berickson1/Playground/tree/master/type-checking-perf

💻 Code

import yup from 'yup'

export function testFunction(): void {
    const yupString = yup.string()
    console.log(yupString);
}

🙁 Actual behavior

Project compile time went from sub-second to over 3s on an M1 MBP to a single project. When using composite projects, this has a larger impact for every package that includes yup. trace json (2 MB) - Perfetto UI 2021-11-18 at 10 20 26 AM

Trace ZIP: trace.json.zip

🙂 Expected behavior

Project compile sees a trivial increase when including yup. This could either be managed by 1) improving peformance of type-checking or 2) leveraging a shared in-memory cache for node_module lib types across composite builds

RyanCavanaugh commented 2 years ago

I'm not quite sure how to turn this into something actionable. The types are just legitimately extremely complex. Here's a snippet from date.d.ts

export interface RequiredDateSchema<TType extends Maybe<Date>, TContext extends AnyObject = AnyObject> extends DateSchema<TType, TContext, NonNullable<TType>> {
    // Cheap half
    defined(msg?: MixedLocale['defined']): DefinedDateSchema<TType, TContext>;
    required(msg?: MixedLocale['required']): RequiredDateSchema<TType, TContext>;
    optional(): DateSchema<TType, TContext>;
    notRequired(): DateSchema<TType, TContext>;

    // Expensive half
    default<D extends Maybe<TType>>(def: Thunk<D>): If<D, RequiredDateSchema<TType | undefined, TContext>, RequiredDateSchema<Defined<TType>, TContext>>;
    nullable(isNullable?: true): RequiredDateSchema<TType | null, TContext>;
    nullable(isNullable: false): RequiredDateSchema<Exclude<TType, null>, TContext>;
}

Commenting out the "expensive half" saves about 0.4s (!!), but it's easy to see why -- validating that all of these instantiations are legal is a huge amount of work. We might be able to make this a little faster, but we're not going to be able to check this fast enough that it won't have a measurable impact on your overall build perf. Even a 10x improvement from 3s to 0.3s is still going to add up to a lot over multiple builds.

It's not possible for us to reuse validation results between builds because each project in a composite build might have different settings or pull in different types (in fact, the latter is almost certainly the case in most builds), both of which can affect whether or not an arbitrary lib typechecks successfully or not.

If you're pulling in lib files someone else published to npm, skipLibCheck is just a very very safe thing to turn on and we recommend it universally for exactly this reason.

berickson1 commented 2 years ago

I've personally seen a handful of type breaks of downstream types as package upgrades land, so had left this disabled for safety.

We also have some complex recursive types in our repo - so improvement here could potentially have downstream benefits to others, but it's hard to directly enumerate. I'm starting to do some performance profiling of our project to identify and improve hotspots. That being said, I'll move forward with setting "skipLibCheck": true to realize some gains here on our builds.

RyanCavanaugh commented 2 years ago

Ideas that @amcasey and I had after spitballing on this for a while:

berickson1 commented 2 years ago

I think either of those would help our scenario - our setup basically templated tsconfig files (see below) so there shouldn't be any additional types included. Of the above, I do like the idea of grouped cohorts - that way it's still very explicit that behaviour is tied together (perhaps a simple checker could be put in place to validate that there's consistency in tsconfig.json files in the same cohort)

{
  "extends": "../../../tsconfig.base.json",
  "compilerOptions": { "rootDir": "./src", "outDir": "./dist" },
  "include": ["src/**/*", "src/**/*.json"],
  "references": [
/* referencePathsHere */
  ]
}
ahejlsberg commented 2 years ago

FWIW, in 4.6 we're doing a lot better on yup because of #46599. The check time for the yup definitely typed package is 5.0s with 4.5, but only 1.5s with 4.6. That's more than a 3x reduction.

berickson1 commented 2 years ago

FWIW, in 4.6 we're doing a lot better on yup because of #46599. The check time for the yup definitely typed package is 5.0s with 4.5, but only 1.5s with 4.6. That's more than a 3x reduction.

I can confirm that setting skipLibCheck: false takes my measured sample (~3s) of yup processing time down to ~600ms for the 3 highlighted expensive .d.ts files utilizing "typescript": "4.6.0-dev.20211120",.