Skip to content

[Performance regression] Performance regression in Typescript 5.1 on large files #60353

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

Closed
moufmouf opened this issue Oct 27, 2024 · 5 comments
Closed
Labels
Not a Defect This behavior is one of several equally-correct options

Comments

@moufmouf
Copy link

🔎 Search Terms

performance regression, Typescript 5.1

🕗 Version & Regression Information

  • This changed between versions 5.0.4 and 5.1
  • This changed in commit 511921e (detected via every-ts)
  • This is the behavior in every version I tried after 5.1, including the latest 5.6 version

⏯ Playground Link

https://github.com/moufmouf/typescript-perf-issue

💻 Code

See Github repo for the code sample (the issue is that very big files take wayyyyy longer to be analyzed)

🙁 Actual behavior

The issue is that the compilation time of some very large files (22k lines) has increased significantly between v5.0 and v5.1 (and is still very long in the latest 5.6 release)

The file I'm trying to compile is 22k lines long.

Compilation time in v5.0: 6s.
Compilation time in v5.1: 110s.

This is a 18x increase.

I generated trace files but I don't know how to interpret them.

Tracefile for 5.0: traceDir_5.0.log
Tracefile for 5.1: traceDir_5.1.log

🙂 Expected behavior

The analysis time would not greatly differ between 5.0 and recent releases.

Additional information about the issue

The file that is suddenly taking very long to analyze was generated using ts-proto.

I could perfectly disable the analysis for this file (since it was autogenerated) but there is probably an underlying issue.

For the record, the original file comes from the WorkAdventure project (https://github.com/workadventure/workadventure)

@s3xysteak
Copy link

s3xysteak commented Nov 1, 2024

After updating to [email protected], the Typescript IntelliSense is so slow😥. I'm not sure is that related to this issue.

@jakebailey
Copy link
Member

@s3xysteak TypeScript 5.1 was ages ago. If things got slower when updating to this month's VS Code version, your issue is definitely not this issue.

@RyanCavanaugh
Copy link
Member

Bisects to #53435

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Nov 8, 2024

We're spending huge amounts of time analyzing functions like this one https://github.com/moufmouf/typescript-perf-issue/blob/master/src/messages.ts#L6122 which have extremely deep control flow graphs with possible narrowing effects due to the conditional assignments.

If this is generated code, it's a huge waste of time to be typechecking these bodies (which would almost certainly be written differently by a human, i.e. in a way that is more amenable to typechecking). It should really be using any in all function bodies, since any is effectively free from a perf perspective. I would also question why it's not using switch, e.g. here it would be much shorter and also efficient both in terms of runtime and typechecking perf

    if (message.message?.$case === "batchMessage") {
      obj.batchMessage = BatchMessage.toJSON(message.message.batchMessage);
    }
    if (message.message?.$case === "errorMessage") {
      obj.errorMessage = ErrorMessage.toJSON(message.message.errorMessage);
    }
    if (message.message?.$case === "roomJoinedMessage") {
      obj.roomJoinedMessage = RoomJoinedMessage.toJSON(message.message.roomJoinedMessage);
    }
    if (message.message?.$case === "webRtcStartMessage") {
      obj.webRtcStartMessage = WebRtcStartMessage.toJSON(message.message.webRtcStartMessage);
    }
    if (message.message?.$case === "webRtcSignalToClientMessage") {
      obj.webRtcSignalToClientMessage = WebRtcSignalToClientMessage.toJSON(message.message.webRtcSignalToClientMessage);
    }

It's possible there's a fix here but it's always going to be possible (but unlikely for a human) to write over-deep CFA graphs like this.

I'm open to a PR on this but overall I would say this is taking an expectable amount of time for the amount of CFA graph complexity the code is presenting; the 5.0 behavior was fast because it was also wrong in many cases.

@RyanCavanaugh RyanCavanaugh added the Not a Defect This behavior is one of several equally-correct options label Nov 8, 2024
@typescript-bot
Copy link
Collaborator

This issue has been marked as "Not a Defect" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not a Defect This behavior is one of several equally-correct options
Projects
None yet
Development

No branches or pull requests

5 participants