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

perf(lsp): improve some tsc op hot paths #13473

Merged
merged 2 commits into from
Jan 24, 2022
Merged

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Jan 23, 2022

Chasing down another issues I realised that while individual op_exists and op_script_version are not performance issues themselves, they are often called 1000s of time per document and we should really try to be as efficient with them as possible.

@kitsonk kitsonk requested a review from dsherret January 23, 2022 23:51
@kitsonk kitsonk merged commit 1a3983a into denoland:main Jan 24, 2022
@kitsonk kitsonk deleted the perf_tsc branch January 24, 2022 08:01
@@ -949,6 +949,22 @@ impl Documents {
}
}

/// Used by the tsc op_exists to shortcut trying to load a document to provide
/// information to CLI without allocating a document.
pub(crate) fn exists(&self, specifier: &ModuleSpecifier) -> bool {
Copy link
Member

@dsherret dsherret Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little strange that we now have exists and contains_specifier, which both do the same thing. I don't think we need two methods. What's the performance impact of using contains_specifier vs exist? When TypeScript does an exists call it will then do a second call load the file, so is it a big deal to just load it when doing .contains_specifier() as it will just be loaded right after?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #13479

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

Successfully merging this pull request may close these issues.

None yet

3 participants