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

Use of intersection type leads to extremely slow typecheck times #58559

Open
xixixao opened this issue May 17, 2024 · 9 comments
Open

Use of intersection type leads to extremely slow typecheck times #58559

xixixao opened this issue May 17, 2024 · 9 comments
Labels
Needs More Info The issue still hasn't been fully clarified

Comments

@xixixao
Copy link

xixixao commented May 17, 2024

πŸ”Ž Search Terms

"intersection", "performance", "typecheck", "subtype", "time"

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about performance

⏯ Playground Link

No response

πŸ’» Code

// Your code here

πŸ™ Actual behavior

I have a pretty complicated library: https://github.com/xixixao/convex-ents (it's similar to Prisma)

(I'm gonna describe the issue in-situ, because essentially the whole library is involved to get the times I'm observing)

All the code I'll mention is in this file: https://github.com/xixixao/convex-ents/blob/edgebitfaster-more-deduped/src/functions.ts

In it I have two intersection types, Ent and EntWriter. I have to use an intersection, because I want to add methods to a POJO (I do this via an intersection between a class and a Record type).

One type should be a subtype of the other (EntWriter should be a subtype of Ent).

The types have a method edge (from the class in the intersection), which returns a conditional type (PromiseEdge and PromiseEdgeWriter respectively), which results in other complicated types, but the subtyping relationship holds.

The library code typechecks fine, but the issue arises when the user of the library tries to assert that EntWriter<> is indeed a subtype of Ent:

const x: Ent<"sometable"> = someEntWriter;

Now TypeScript goes and tries to check the subtyping relationship, starting with the intersection, walking to the conditionals, and to all their branches, and to all the methods of those types, etc. It took minutes.

I can observe the slowdown by reverting PromiseEdgeWriter to match PromiseEdge exactly, and then changing one branch at a time.

After reading the performance wiki and getting a trace and going through the types, I have optimized this somewhat. I have pulled out the conditional logic into a type that is reused (see PromiseEdgeResult). This helped a lot, but the typechecking still takes 12 seconds instead of sub-second without this edge type change.

I tried pulling out the intersection to a helper type as well (EntBase) but this had no impact. It seems that the intersection is "inlined"?

πŸ™‚ Expected behavior

What I think would help me is if:

A subtype of B
X' = X<A, C>
X'' = X<B, C>
implied that X' subtype of X''
even when X<T, U> = T & U

But this doesn't seem to be what the typechecker actually assumes? (probably because intersection merged properties as per wiki)

But could I hint somehow to the compiler to not go through this whole huge reasoning chain? I know that EntWriter is subtype of Ent when their generic arguments are the same.

The intersection types themselves are ok, but it's the interplay with the two classes. The slowdown is from the edge method being checked. So alternatively I'd like to tell TypeScript: "Hey, this method does correctly override the parent class's method, you don't need to check that if the generic arguments to it are the same"

Additional information about the issue

No response

@fatcerberus
Copy link

A subtype of B
X' = X<A, C>
X'' = X<B, C>
implied that X' subtype of X''
even when X<T, U> = T & U

This is not a valid assumption for arbitrary X. For example, T might be contravariant, or X might even be a conditional type in which case the resulting type relationship is entirely arbitrary.

@ahejlsberg
Copy link
Member

It's hard to diagnose an issue without a specific example. Ideally one that fits in the playground, but if not, a repo that can be easily cloned.

@ahejlsberg ahejlsberg added the Needs More Info The issue still hasn't been fully clarified label May 18, 2024
@xixixao
Copy link
Author

xixixao commented May 19, 2024

@ahejlsberg appreciate you having a look! This should do the trick in a few seconds:

git clone https://github.com/xixixao/convex-ents --depth 2 -b edgebitfaster repro-convex-ents
cd repro-convex-ents
npm i
cd test
npm i
npx tsc

@ahejlsberg
Copy link
Member

ahejlsberg commented May 20, 2024

@xixixao Thanks. With npx tsc -diagnostics I see the following:

Files:              289
Lines:            89160
Identifiers:      81591
Symbols:         214861
Types:           207767
Instantiations: 8903666
Memory used:    314029K
I/O read:         0.11s
I/O write:        0.00s
Parse time:       0.53s
Bind time:        0.12s
Check time:      14.37s
Emit time:        0.00s
Total time:      15.03s

The number that catches my eye here is the ~10M type instantiations. That is a very high number, indicative of some expensive type operations in the code base.

Unfortunately, when I compile the code base with the nightly build of the compiler (or the 5.5 beta release) I see a long list of type check errors (but not the long compile times), so I can't really debug the issue further. Looks to me like a conditional type is making a different choice with 5.5, which subsequently causes a lot of tests to error.

If you can update your repo to reproduce the issue with 5.5 beta or the nightly builds I can try to look further.

@xixixao
Copy link
Author

xixixao commented May 20, 2024

@ahejlsberg that looks to be caused by a potential bug in 5.5. I can't again get it to simplify, as it doesn't repro inside an individual module. But the main issue is in setup.testing.ts:

Screenshot 2024-05-20 at 17 19 44

This is caused by EntDefinition not being accepted as a subtype of TableDefinition (which comes from another npm package, and if I inline it the issue goes away):

You can replace setup.testing.ts with:

import {
  TableDefinition,
} from "convex/server";
import { EntDefinition } from "../../src";

const x: EntDefinition = null as any;

function foo(t: TableDefinition) {

}

foo(x);

And you will see the error:
Screenshot 2024-05-20 at 17 09 09

I don't know how to fix this error. TableDefinition is defined as a class, exported as a type. EntDefinition is right now defined as an interface extending TableDefinition. If change it to declare class the error stays the same.

@ahejlsberg
Copy link
Member

ahejlsberg commented May 20, 2024

Looks like you have two competing declarations of TableDefinition with the following paths (on my machine):

C:\tc\repro-convex-ents\test\node_modules\convex\src\server\schema.ts
C:\tc\repro-convex-ents\src\schema.ts

The compiler is pointing this out. We've made some fixes to module resolution in 5.5 so likely that's the cause. I would trust that 5.5 is getting it right, so you probably want to look into that.

@xixixao
Copy link
Author

xixixao commented May 21, 2024

@ahejlsberg So that's because of my funky importing from within test from the parent project, which 5.5 resolution seems to choke on (since I have the dependency installed twice). So fixed that via linking but then the issue no longer repros inside the repo.

But I can still repro it in a separate repo (set up with TS5.5 and the "broken" version of the lib):

git clone https://github.com/xixixao/saas-starter --depth 2 -b repro-ts-issue repro-convex-saas
cd repro-convex-saas
npm i -f
npx tsc -diagnostics

Gives:

Files:              1200
Lines:            197780
Identifiers:      208367
Symbols:          471886
Types:            657241
Instantiations: 17862935
Memory used:     808594K
I/O read:          0.04s
I/O write:         0.00s
Parse time:        0.69s
Bind time:         0.19s
Check time:       13.88s
Emit time:         0.02s
Total time:       14.77s

So back to the high instantiation count.

And from the trace I can see that it's the EntWriter < Ent typecheck in several places that causes this.

The only other thing I noticed is that TS caches the results in tsconfig.tsbuildinfo so it needs to be deleted to observe the full typecheck.

The library source code is on branch stillslowinstarter (vs the much faster version on the parent commit), the diff between them is:

--- a/src/functions.ts
+++ b/src/functions.ts
@@ -2375,7 +2375,15 @@ class PromiseEntWriterImpl<
 declare class EntWriterInstance<
   EntsDataModel extends GenericEntsDataModel,
   Table extends TableNamesInDataModel<EntsDataModel>,
-> extends EntInstance<EntsDataModel, Table> {
+> {
+  edge<Edge extends keyof EntsDataModel[Table]["edges"]>(
+    edge: Edge,
+  ): PromiseEdgeWriter<EntsDataModel, Table, Edge>;
+  edgeX<Edge extends keyof EntsDataModel[Table]["edges"]>(
+    edge: Edge,
+  ): PromiseEdgeWriterOrThrow<EntsDataModel, Table, Edge>;
+  doc(): DocumentByName<EntsDataModel, Table>;
+

(leaving the extends in doesn't help, TS still goes and checks that the edge and edgeX are compatible with the parent class)

@ahejlsberg
Copy link
Member

So fixed that via linking but then the issue no longer repros inside the repo.

That may indeed be the fix. I have a hunch your issue is caused by excessive work resulting from structural comparisons of very similar (if not structurally identical) types. That can happen when you have multiple copies of the same library in a single compilation.

But I can still repro it in a separate repo (set up with TS5.5 and the "broken" version of the lib)

What is it that is "broken" here? If it is because you have multiple copies of the same types (and thus excessive work from structural comparisons), then I don't think there's much we can do.

@xixixao
Copy link
Author

xixixao commented May 21, 2024

@ahejlsberg no, I don't I have multiple copies of the same library in the second repro I shared. There's a single flat node_modules with only one copy of convex and convex-ents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs More Info The issue still hasn't been fully clarified
Projects
None yet
Development

No branches or pull requests

3 participants