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.

Sign up for GitHub

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

Jump to bottom

Performance regression in TS 5.4 #57781

Closed
MichaelMitchell-at opened this issue Mar 14, 2024 · 10 comments Β· Fixed by #57849
Closed

Performance regression in TS 5.4 #57781

MichaelMitchell-at opened this issue Mar 14, 2024 · 10 comments Β· Fixed by #57849
Labels
Fix Available A PR has been opened for this issue Needs More Info The issue still hasn't been fully clarified

Comments

@MichaelMitchell-at
Copy link

πŸ”Ž Search Terms

performance regression

πŸ•— Version & Regression Information

  • This changed in commit or PR 4bcbc16

⏯ Playground Link

No response

πŸ’» Code

I cannot share our repository nor can I feasibly pare it down to be shareable.

πŸ™ Actual behavior

Typechecking time in our monorepo takes about 375 seconds on my local machine.

πŸ™‚ Expected behavior

Typechecking time used to be about 315 seconds on my local machine with the same code.

Additional information about the issue

Here's my usage of every-ts to bisect the results. It narrows down to #56087. cc @Andarist

$ every-ts bisect start               
Updating files: 100% (5762/5762), done.
Previous HEAD position was e441420483 Update package-lock.json
HEAD is now at 9684ba6b0d Cherry-pick fix for cross-file inlay hints (#55476) to `release-5.2` and LKG (#55487)
status: waiting for both good and bad commits
Building TypeScript...
TypeScript built successfully!

$ every-ts bisect bad 5.4.2
Resolved 5.4.2 to v5.4.2
status: waiting for good commit(s), bad commit known

$ every-ts bisect good e44142048305d42ec4fc753457aa561f8e247e4f
Bisecting: 66 revisions left to test after this (roughly 6 steps)
remote: Enumerating objects: 125, done.
remote: Counting objects: 100% (101/101), done.
remote: Compressing objects: 100% (65/65), done.
remote: Total 125 (delta 42), reused 36 (delta 36), pack-reused 24
Receiving objects: 100% (125/125), 1.68 MiB | 2.45 MiB/s, done.
Resolving deltas: 100% (57/57), done.
[dad9f17c773f97426f90539dd75a6a5100d342d0] Update package-lock.json
Building TypeScript...
TypeScript built successfully!

$ every-ts bisect bad  # 368.143 s
Bisecting: 32 revisions left to test after this (roughly 5 steps)
remote: Enumerating objects: 29, done.
remote: Counting objects: 100% (25/25), done.
remote: Compressing objects: 100% (24/24), done.
remote: Total 29 (delta 1), reused 1 (delta 1), pack-reused 4
Receiving objects: 100% (29/29), 1.31 MiB | 6.25 MiB/s, done.
Resolving deltas: 100% (1/1), done.
[69f2e2ae651a13ad865727f42af8fac48c64e137] Issue better error when unresolvable package in `--moduleResolution node10` resolves with `--moduleResolution bundler` (#56949)
Building TypeScript...
TypeScript built successfully!

$ every-ts bisect bad  # 371.229 s 
Bisecting: 16 revisions left to test after this (roughly 4 steps)
remote: Enumerating objects: 6, done.
remote: Counting objects: 100% (6/6), done.
remote: Compressing objects: 100% (6/6), done.
remote: Total 6 (delta 0), reused 0 (delta 0), pack-reused 0
Receiving objects: 100% (6/6), 939.99 KiB | 5.08 MiB/s, done.
[81793210e6337ad8d20f2b7e44e9489687c2d29c] Update package-lock.json
Building TypeScript...
TypeScript built successfully!

$ every-ts bisect bad  # 374.109 s
Bisecting: 7 revisions left to test after this (roughly 3 steps)
remote: Enumerating objects: 4, done.
remote: Counting objects: 100% (4/4), done.
remote: Compressing objects: 100% (4/4), done.
remote: Total 4 (delta 0), reused 0 (delta 0), pack-reused 0
Receiving objects: 100% (4/4), 784.99 KiB | 6.04 MiB/s, done.
[1e00399a383d253610ec18d3a77d08b671219fc2] Fixed emit of return statements with parenthesized assertions and comments (#56601)
Building TypeScript...
TypeScript built successfully!

$ every-ts bisect good  # 315.135 s
Bisecting: 3 revisions left to test after this (roughly 2 steps)
remote: Enumerating objects: 1, done.
remote: Counting objects: 100% (1/1), done.
remote: Total 1 (delta 0), reused 0 (delta 0), pack-reused 0
Receiving objects: 100% (1/1), 549.70 KiB | 6.04 MiB/s, done.
[4557e34e7003f5dd11fd33881ae91d10451fb23a] Emit declarations using alternative containing modules for types exported using separate export statements (#56857)
Building TypeScript...
TypeScript built successfully!

$ every-ts bisect good  # 319.945 s
Bisecting: 1 revision left to test after this (roughly 1 step)
[2c14a1c2250d0b891f743c1b6ae58ac951942206] fix(55650): Wrong/missing quick info in JSDoc @implements tag  (#56884)
Building TypeScript...
TypeScript built successfully!

$ every-ts bisect bad  # 364.453 s
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[4bcbc16cff6c65caf349f1c1df4dc7b6809468ae] Use symbols of type aliases when emitting declarations (#56087)
Building TypeScript...
TypeScript built successfully!

$ every-ts bisect bad  # 368.270 s
4bcbc16cff6c65caf349f1c1df4dc7b6809468ae is the first bad commit
commit 4bcbc16cff6c65caf349f1c1df4dc7b6809468ae
Author: Mateusz BurzyΕ„ski <mateuszburzynski@gmail.com>
Date:   Tue Jan 9 22:02:52 2024 +0100

    Use symbols of type aliases when emitting declarations (#56087)

 src/compiler/checker.ts                            |  6 +++
 .../reference/declarationEmitUsingTypeAlias1.js    | 47 ++++++++++++++++++++++
 .../declarationEmitUsingTypeAlias1.symbols         | 46 +++++++++++++++++++++
 .../reference/declarationEmitUsingTypeAlias1.types | 43 ++++++++++++++++++++
 .../compiler/declarationEmitUsingTypeAlias1.ts     | 30 ++++++++++++++
 5 files changed, 172 insertions(+)
 create mode 100644 tests/baselines/reference/declarationEmitUsingTypeAlias1.js
 create mode 100644 tests/baselines/reference/declarationEmitUsingTypeAlias1.symbols
 create mode 100644 tests/baselines/reference/declarationEmitUsingTypeAlias1.types
 create mode 100644 tests/cases/compiler/declarationEmitUsingTypeAlias1.ts
Building TypeScript...
TypeScript built successfully!
@Andarist
Copy link
Contributor

Andarist commented Mar 14, 2024 •
edited
Loading

Would you be able to apply the patch from this PR and test the perf?

@MichaelMitchell-at
Copy link
Author

Would you be able to apply the patch from this PR and test the perf?

Sure, will try it now

@MichaelMitchell-at
Copy link
Author

Would you be able to apply the patch from this PR and test the perf?

I applied the patch and this brought typechecking time down to ~331 s.

@Andarist
Copy link
Contributor

Is there any difference there between when you typecheck + emit dts and when you just typecheck?

@MichaelMitchell-at
Copy link
Author

Is there any difference there between when you typecheck + emit dts and when you just typecheck?

Since this is a composite project, subprojects must emit dts for parent projects to use.

@RyanCavanaugh
Copy link
Member

We need a way to repro this in order to investigate. None of our perf tests showed a significant drop in performance over the prior release, so it's something specific to the code here that we'd need to be able to look at to make further progress.

@RyanCavanaugh RyanCavanaugh added the Needs More Info The issue still hasn't been fully clarified label Mar 15, 2024
@MichaelMitchell-at
Copy link
Author

MichaelMitchell-at commented Mar 15, 2024 •
edited
Loading

I can send over any traces or profiles you want so long as I can redact any actual code/code paths. Since I lack the knowledge or tools to identify what the performance hotspots in the code actually are (a frustration that arises frequently when trying to optimize our typechecking perf in general), there's just no way for me to construct a shareable repo that can illustrate the regression.

@jakebailey
Copy link
Member

If you use https://www.npmjs.com/package/pprof-it on tsc.js and set PPROF_SANITIZE=true, the output profiles should be free of any references to code or even to the paths on your machine. Getting profiles before/after the bad commit would be most helpful.

@MichaelMitchell-at
Copy link
Author

If you use https://www.npmjs.com/package/pprof-it on tsc.js and set PPROF_SANITIZE=true, the output profiles should be free of any references to code or even to the paths on your machine. Getting profiles before/after the bad commit would be most helpful.

Thanks I'll do this over the weekend

@MichaelMitchell-at
Copy link
Author

Here are some profiles from pprof-it

Before regression
pprof-heap-17148.pb.gz
pprof-time-17148.pb.gz

After regression
pprof-heap-22618.pb.gz
pprof-time-22618.pb.gz

@jakebailey jakebailey mentioned this issue Mar 19, 2024
Merged
@typescript-bot typescript-bot added Fix Available A PR has been opened for this issue labels Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Fix Available A PR has been opened for this issue Needs More Info The issue still hasn't been fully clarified
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Revert PR 56087 jakebailey/TypeScript
5 participants
@jakebailey @RyanCavanaugh @Andarist @typescript-bot @MichaelMitchell-at