-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Keep scriptInfo and project alive even after file delete till next file open #57492
Conversation
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
1ec1e98
to
7e9f5f7
Compare
28182a1
to
ba47b73
Compare
ba47b73
to
9c2e8aa
Compare
9c2e8aa
to
bd10117
Compare
@typescript-bot perf test this |
@jakebailey Here are the results of running the user tests comparing Everything looks good! |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine to me (though I don't know how to physically test it), though I think my main concern are API users who expect that closing a file actually frees its resources. I'm not sure if ts-eslint
actually closes any files, though.
bd10117
to
e7d83a9
Compare
e7d83a9
to
c6bfa10
Compare
Some times when updating branches or doing
npm ci
, instead of file change event, we may get file delete followed by file create event.This was previously handled not so efficiently as: as soon as we get delete event say on configFile for the project, we would remove the whole project from memory and schedule update for open files, only to get file create so end up creating project again.
With this change:
defer delete
option. The deferred delete marked project is kept alive and acts as orphan project until next file open which is when we normally dogc
on all projects and scriptInfos. Note we do something similar for inferred project so we can reuse it so its just extending that concept to configured projectnpm ci
can replace file with same text and this helps with file usage. Again these deferred removed on next file open.Other two commits in the PR are tests before the change to cover for scenarios of file delete, recreate etc and refactoring
This should also help with #57196 when we would open more projects and configs to search for your default project
Without proper repro to measure perf reliably but in my dogfooding and from tests it seems to really help with the perf in these kind of scenarios. (No guarantee when timeout happens and how things will unfold to see if delete, create happen or not)