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

Improve the performance of isolatedDeclarations quickfix #58722

Merged
merged 3 commits into from
May 31, 2024

Conversation

blickly
Copy link
Contributor

@blickly blickly commented May 30, 2024

It turns out that typeChecker.getEmitResolver() can be expensive, so avoid calling it unless necessary.

Hopefully helps #58426, in particularly the performance issue (bullet 1)

It turns out that `typeChecker.getEmitResolver()` can be expensive,
so avoid calling it unless necessary.

Hopefully this fixes the performance issue (bullet microsoft#1) of
microsoft#58426
These are not related to the perf issue, but I noticed them while editing this file
@blickly
Copy link
Contributor Author

blickly commented May 30, 2024

@microsoft-github-policy-service agree company="Google"

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 30, 2024
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@jakebailey
Copy link
Member

Weird, how does this end up being expensive? Is it recalculating diagnostics? Something else? Do you have a profile before/after?

You'll want to resend that CLA comment again, I don't think it understands edits. I also modified your PR description as the language it contained would have closed the linked issue (when it shouldn't in full yet).

@blickly
Copy link
Contributor Author

blickly commented May 30, 2024

Yeah, I assume it's recalculating diagnostics, as the method called inside getEmitResolver is called getDiagnostics2.

The full before profile is vscode-profile-2024-05-29-16-18-13.cpuprofile

After this fix, the quickfix in my example projects didn't have perceptible latency, so I didn't try to grab an after profile.

@microsoft-github-policy-service agree company="Google"

@blickly
Copy link
Contributor Author

blickly commented May 30, 2024

Oops, I think I uploaded the wrong before profile. Darn reverse chronological. The useful ones are these three:

@jakebailey jakebailey merged commit fc42002 into microsoft:main May 31, 2024
28 checks passed
@robpalme
Copy link

Thank you @blickly for jumping on this 👍

skeate added a commit to skeate/TypeScript that referenced this pull request Jun 1, 2024
* upstream/main: (37 commits)
  Added NoTruncation flag to completions (microsoft#58719)
  Clone node to remove location even when it has been modified if needed (microsoft#58706)
  Properly account for `this` argument in intersection apparent type caching (microsoft#58677)
  Fix: Include Values of Script Extensions for Unicode Property Value Expressions in Regular Expressions (microsoft#58615)
  In `reScanSlashToken` use `charCodeChecked` not `codePointChecked` (microsoft#58727)
  Shorten error spans for errors reported on constructor declarations (microsoft#58061)
  Mark file as skips typechecking if it contains ts-nocheck (microsoft#58593)
  Fixed an issue with broken `await using` declarations in `for of` loops (microsoft#56466)
  Do not expand type references in keyof and index access (microsoft#58715)
  Improve the performance of isolatedDeclarations quickfix  (microsoft#58722)
  Unwrap `NoInfer` types when narrowing (microsoft#58292)
  Recover from type reuse errors by falling back to inferred type printing (microsoft#58720)
  Fixing self import (microsoft#58718)
  Enable JS emit for noCheck and noCheck for transpileModule (microsoft#58364)
  Revert PR 55371 (microsoft#58702)
  Update dependencies (microsoft#58639)
  Fix baselines after PR 58621 (microsoft#58705)
  Do not infer `yield*` type from contextual `TReturn` (microsoft#58621)
  `await using` normative changes (microsoft#58624)
  Handling statements from a known source file (microsoft#58679)
  ...
@blickly
Copy link
Contributor Author

blickly commented Jun 3, 2024

Thanks! I also wanted to to thank @h-joo for helping to answer many of my questions and get me up to speed on the codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants