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

[Bug]: Performance regression in 14.2.0 and 14.2.1 #2886

Closed
mattlewis92 opened this issue Dec 2, 2024 · 2 comments · Fixed by #2888
Closed

[Bug]: Performance regression in 14.2.0 and 14.2.1 #2886

mattlewis92 opened this issue Dec 2, 2024 · 2 comments · Fixed by #2888
Labels
🐛 Bug Confirmed Bug is confirmed

Comments

@mattlewis92
Copy link

Version

14.4.1

Steps to reproduce

As this is a performance issue, it's really difficult to reproduce this outside of our repo as you need a large amount of source files and tests to see the problem.

Expected behavior

Performance does not change between versions

Actual behavior

After upgrading from 14.1.1 to 14.4.1 we noticed a massive performance hit in our unit test times, where everything got more than twice as slow.

We managed to track down the source of the slow performance to these commits:

  • 31e78b1 (this makes our tests take 2.5x times as long)
  • ad7a297 (after locally reverting the previous commit, this makes our tests take ~20% longer, presumably due to the overhead of creating multiple typescript programs instead of 1 as we were before)

Once we reverted both of these commits locally, our test times returned to what they were on the previous version.

As these commits were just refactors, would it be possible to just revert these 2 commits and restore the previous performance? The only test that fails when I did this locally, was this one but I don't think we need to access the type at runtime as we are using an injection token, so I think it's fine for that to be compiled to undefined.

Additional context

We are using the isolatedModules: true flag in our jest config to disable type-checking

Environment

System:
  OS: macOS 14.7
  CPU: (16) arm64 Apple M3 Max
Binaries:
  Node: 22.11.0 - ~/Library/pnpm/nodejs/22.11.0/bin/node
  Yarn: 1.22.22 - ~/Library/pnpm/yarn
  npm: 10.7.0 - ~/.nvm/versions/node/v20.15.0/bin/npm
  pnpm: 9.14.4 - ~/.nvm/versions/node/v20.15.0/bin/pnpm
npmPackages:
  jest: 29.7.0 => 29.7.0
@ahnpnl
Copy link
Collaborator

ahnpnl commented Dec 3, 2024

Thanks, indeed I could see the issue. I added a perf test to the fix PR which should be able to guard us from future changes which impacts performance.

ahnpnl added a commit that referenced this issue Dec 3, 2024
As reported, the perf issue is caused by the introduction of an extra `Program` instance to fetch to AST transformers next to the hidden `Program` behind `ts.transpileModule`. We should only use one instance of `Program` therefore we have to manually copy the codes of `ts.transpileModule` to use for our case.

Fixes #2886
ahnpnl added a commit that referenced this issue Dec 3, 2024
As reported, the perf issue is caused by the introduction of an extra `Program` instance to fetch to AST transformers next to the hidden `Program` behind `ts.transpileModule`. We should only use one instance of `Program` therefore we have to manually copy the codes of `ts.transpileModule` to use for our case.

Fixes #2886
ahnpnl added a commit that referenced this issue Dec 3, 2024
As reported, the perf issue is caused by the introduction of an extra `Program` instance to fetch to AST transformers next to the hidden `Program` behind `ts.transpileModule`. We should only use one instance of `Program` therefore we have to manually copy the codes of `ts.transpileModule` to use for our case.

Fixes #2886
@ahnpnl ahnpnl closed this as completed in b19cd6e Dec 3, 2024
@mattlewis92
Copy link
Author

Thank you so much!! It's very much appreciated 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug Confirmed Bug is confirmed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants