-
Notifications
You must be signed in to change notification settings - Fork 455
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
chore(compiler): improve performance for isolatedModules: false #1549
Conversation
@kulshekhar I tested with my company project and it improved almost 30% test speed. Can you please help testing this against your real repo to see if this approach causes any issues and how it improves ? |
Sure, I'll test and post the results here. Although none of my repos are
big enough that the change will be noticeable!
Instead of an environment variable, we could publish an alpha/beta version
- what do you think? The reason is that there won't be a lot of switching
between these versions. If one works for one project, that project will
keep using that version..
…On Mon, Apr 20, 2020 at 9:18 PM Ahn ***@***.***> wrote:
@kulshekhar <https://github.com/kulshekhar> I have a problem with the
approach of this PR. Our internal tests don't accept this change while I
tested with my company project and it improved *almost 30%* test speed.
Can you please help on this:
- Can you please test the build of this branch against your real repo ?
- I'm thinking about creating a flag based on environment variable to
toggle between old codes
<https://github.com/kulshekhar/ts-jest/blob/ef4bb76abbb5d3bce41a8f135390f1a44689b66b/src/compiler/instance.ts#L178>
vs new codes
<https://github.com/ahnpnl/ts-jest/blob/5671bb6c6a0dc11d0aefb5604e9bee49fe4deda6/src/compiler/instance.ts#L183>.
So basically I want to add an if to toggle running either the old codes or
the new codes. What do you think ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1549 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACSH7UF5Q4QPXELACVGEQULRNRVFFANCNFSM4MMRRUHA>
.
|
Ye I think let’s try with a alpha/beta version and wait for feedbacks. We can use RFC issue to gather feedbacks for that unreleased version. UPDATE: I think I found the way to fix the internal tests. I’m more curious about how this behaves for a real repo. |
Pull Request Test Coverage Report for Build 4495
💛 - Coveralls |
ok this branch is ready for alpha/beta release. I think we shouldn't merge into master yet but wait for feedbacks and decide later. |
@ahnpnl as I suspected, my repos aren't big enough to show a noticeable difference. I'll get to publishing an alpha version after work today |
made a final push before releasing alpha version. I've tested on Windows with my company project, also see a reduction around 30%, some cases even 40% |
actually alpha-2 is still not the correct version... I made small mistake so the latest commit e75ff6b should be the one we need to use for testing. |
Summary
Improve performance for
isolatedModules: false
by only creatingLanguageService
instance with limited amount of initial files.Because when creating
LanguageService
instance withfileNames
fromtsconfig
, often thisfileNames
includes all the files in the project which makes internalTypeScript
performs scanning and reading the unnecessary files. This is not optimal because it will create lots of I/O threads which then impact heavily on the performance.This PR should help #1115 and all related performance issues by somewhere around 30% improvement in speed.
Test plan
Does this PR introduce a breaking change?
Other information