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

Regression performance compiling types in version 5.4.2 #57710

Open
georgekaran opened this issue Mar 9, 2024 · 18 comments
Open

Regression performance compiling types in version 5.4.2 #57710

georgekaran opened this issue Mar 9, 2024 · 18 comments
Labels
Needs More Info The issue still hasn't been fully clarified

Comments

@georgekaran
Copy link

πŸ”Ž Search Terms

"regression", "5.4.2", "performance"

πŸ•— Version & Regression Information

  • This changed between versions 5.3.3 and 5.4.2

⏯ Playground Link

No response

πŸ’» Code

Extended diagnostics in version 5.4.2

Files:                          4294
Lines of Library:              40640
Lines of Definitions:         180807
Lines of TypeScript:          430513
Lines of JavaScript:               0
Lines of JSON:                     0
Lines of Other:                    0
Identifiers:                  591567
Symbols:                     1232510
Types:                       4677381
Instantiations:            395496734
Memory used:                2330550K
Assignability cache size:    3974893
Identity cache size:           14482
Subtype cache size:            37794
Strict subtype cache size:      9374
I/O Read time:                 0.70s
Parse time:                    0.96s
ResolveModule time:            0.58s
ResolveTypeReference time:     0.01s
ResolveLibrary time:           0.01s
Program time:                  2.51s
Bind time:                     0.62s
Check time:                  367.45s
printTime time:                0.00s
Emit time:                     0.00s
Total time:                  370.58s

Extended diagnostics in version 5.3.3

Files:                         4290
Lines of Library:             40241
Lines of Definitions:        180807
Lines of TypeScript:         430513
Lines of JavaScript:              0
Lines of JSON:                    0
Lines of Other:                   0
Identifiers:                 591320
Symbols:                    1234773
Types:                       699200
Instantiations:             2247099
Memory used:               1308962K
Assignability cache size:    223823
Identity cache size:          14479
Subtype cache size:           28353
Strict subtype cache size:     9128
I/O Read time:                0.73s
Parse time:                   1.11s
ResolveModule time:           0.44s
ResolveTypeReference time:    0.01s
ResolveLibrary time:          0.02s
Program time:                 2.60s
Bind time:                    0.76s
Check time:                  13.75s
printTime time:               0.00s
Emit time:                    0.00s
Total time:                  17.11s

πŸ™ Actual behavior

The type validation using tsc --noEmit increase more than 20 times using the version 5.4.2 comparing with the version 5.3.3.

In the CI is always hitting the heap memory limit:
image

πŸ™‚ Expected behavior

I expected the compilation to be equivalent in both versions, but it was more than 20 times slower.

Additional information about the issue

No response

@Andarist
Copy link
Contributor

Andarist commented Mar 9, 2024

Could u share a repro case of this problem?

@fatcerberus
Copy link

The number of instantiations went up over a hundredfold 😱

@jakebailey
Copy link
Member

If you can reproduce this consistently, consider bisecting using https://www.npmjs.com/package/every-ts.

@georgekaran
Copy link
Author

If you can reproduce this consistently, consider bisecting using https://www.npmjs.com/package/every-ts.

I was able to pinpoint the exact commit which the performance problem was started.

8d1fa44 is the first bad commit
commit 8d1fa44
Author: Mateusz BurzyΕ„ski mateuszburzynski@gmail.com
Date: Thu Nov 30 20:24:04 2023 +0100

Defer processing of nested generic calls that return constructor types (#54813)

Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>

@georgekaran
Copy link
Author

Could u share a repro case of this problem?

Unfortunately, I cannot share a repro for this problem as this is an internal closed-source project from my company.

@fatcerberus
Copy link

@Andarist It’s one of your PRs: #54813

@Andarist
Copy link
Contributor

guy saying not again

Unfortunately, I cannot share a repro for this problem as this is an internal closed-source project from my company.

Understandably, u might not be able to share it as-is. To investigate this I'd need some kind of a repro case though - I can't chase a wild goose

@georgekaran
Copy link
Author

Understandably, u might not be able to share it as-is. To investigate this I'd need some kind of a repro case though - I can't chase a wild goose

Sure thing, I will work on creating a reproducible for this issue. It may take a while as our codebase is quite large 😞

@sandersn
Copy link
Member

I sent a PR to revert the change, but it's likely that the underlying bug was unrelated, and exposed by this change. @weswigham believes it is possible to construct a failure that relies exclusively on functions, not on constructors. Long-term, we should isolate and fix the underlying bug and re-apply this change.

@sandersn
Copy link
Member

@georgekaran would one of the TS team at Microsoft be able to sign an NDA to get access to your code? Then we could isolate the problem and produce a repro that's not linked to your code.

@jakebailey
Copy link
Member

Even if you can't share the code for now, can you try running tsc via https://www.npmjs.com/package/pprof-it? The profile won't include any code, and if you enable PPROF_SANITIZE=true, shouldn't include any paths or other info from your machine. (But I would verify the output file.)

@RyanCavanaugh RyanCavanaugh added the Needs More Info The issue still hasn't been fully clarified label Mar 12, 2024
@reduckted
Copy link

I just want to chime in and say that I think I'm encountering the same issue, however updating to 5.4.3 hasn't fixed the problem. I'm still seeing the error:

FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory

I'm actually seeing this when running ESLint, but I've been able to track it down to something related to the TypeScript config that's being used. Using the same config that worked fine with TS 5.3 will cause the OOM errors in TS 5.4. Changing that config to exclude certain directories (and hence reducing the overall size of the project) avoids the OOM errors.

Unfortunately, I also cannot share a repro for the same reasons (closed-source, company project, etc), and there's next to no chance of an NDA being allowed. I'll see what I can do to get a simpler reproduction though.

@jakebailey
Copy link
Member

I would definitely recommend using every-ts and linking it in to bisect, if it's reproducible. But I would doubt that your issue is related; typically bugs like this aren't, but a bisect would say .

@reduckted
Copy link

reduckted commented Mar 25, 2024

@jakebailey Looks like you're right about what I'm seeing being a different bug. Commit 8d1fa44 doesn't exhibit the behavior that I'm seeing.

every-ts is awesome by the way! ❀️

I had a bit of trouble using every-ts bisect to pinpoint when the bug that I'm seeing was introduced. Bisecting 5.3.3 and 5.4.2 led me to the problem commit being one that only changed files in the .github/ folder πŸ˜†. I have a feeling that might be because the bug was introduced at some point, then fixed or reverted, then introduced again.
Edit: I must have made a mistake on multiple bisect runs, because I just tried again and ended up with the correct commit that's causing my error. πŸ€¦β€β™‚οΈ

I was able to successfully use every-ts bisect to pinpoint the commit where the bug I'm seeing was introduced. πŸŽ‰ I'll create a new issue about it.

Edit: #58011 is the new issue.

@Katli95
Copy link

Katli95 commented Apr 5, 2024

Sorry, I commented on a couple of other old issues before I read some comments about trying to group things by at least versions (in my defense, there's a lot of OOM issues open πŸ˜…)

--
I'm running into the same on GitHub Actions Runner (ubuntu-latest) but not on local (M1 Pro).
Please ask for more info that might be helpful and I'll examine if there's something I'm at liberty to share.

Some basic relative info:
Next App, large-ish project.
Running into this error on typescript@5.4.4 but working on typescript@5.2.2 in Github Actions, always works on local.
using "zod": "^3.20.3"(seemed relative to another OOM issue I found)

@jakebailey
Copy link
Member

Sorry, I commented on a couple of other old issues before I read some comments about trying to group things by at least versions (in my defense, there's a lot of OOM issues open πŸ˜…)

Not sure what you mean; we want new issues for different OOMs because they're very likely unrelated, unless you've bisected the problem to the same thing as this particular issue.

@Katli95
Copy link

Katli95 commented Apr 5, 2024

unless you've bisected the problem to the same thing as this particular issue.

Right, gotcha, well that's kinda difficult in my case as it's only happening on GH Actions :/
What would be the best way to try and spot it?
Still testing manually on local (with every-ts) and checking for a memory spike?
And hoping that it's platform (Mac vs. Ubuntu) independent?

(thx for the quick reply btw!)

@jakebailey
Copy link
Member

--extendedDiagnostics should show memory usage, or you could artificially lower the max memory with Node's max-old-space-size. That or provide a repro.

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

8 participants