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

optim(watch): don't reset DocumentRegistry b/t watch cycles #388

Merged
merged 1 commit into from
Aug 8, 2022

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Jul 22, 2022

Summary

Keep the DocumentRegistry b/t watch cycles instead of resetting it

Details

  • we can re-use the same one during watch instead of resetting it in the options hook
    • should make the LS a bit faster / more efficient in watch mode as most source files are shared between watch cycles

Potential Further Optimizations

  1. DocumentRegistry can actually be shared between multiple LSes for Recommendation for multi-module setup? Memory leak with 18+ Rollup watchers #177. Per the TS LS Wiki, sharing DocumentRegistry is actually recommended, as, minimally, lib.d.ts could be shared between projects.
    • But to do that we'd probably have to expose an option for it and users would have to opt-in. On second thought, this might not even be useful for Recommendation for multi-module setup? Memory leak with 18+ Rollup watchers #177 since those are separate processes that don't share variables 😕
      • Thinking about it one step further... we could actually create the DocumentRegistry as a top-level var, before the plugin is instantiated... that would make DocumentRegistry shared between all imports of rpt2... but it would still be limited to only the same Node process... so guess that's probably not useful either in this case 😕
        • And the top-level instantiation could be a bit hacky since tsModule isn't set till plugin instantiation (as visible in this PR, where the DocumentRegistry is only instantiated after tsModule is set)...
      • Leaving it as is here, as a result then
  2. It's possible we can avoid recreating the LS itself between watch cycles. That would be optimal. I believe we'd be able to do that if the parsedConfig hasn't changed between watch cycles, which is probably the most common usage of watch anyway, i.e. changing source files and not config

- we can re-use the same one during watch instead of resetting it in the `options` hook
  - should make the LS a bit faster / more efficient in watch mode as most source files are shared between watch cycles
@agilgur5 agilgur5 added kind: optimization Performance, space, size, etc improvement scope: watch mode Related to Rollup's watch mode labels Jul 22, 2022
@agilgur5 agilgur5 added the topic: TS Compiler API Docs Related to the severely lacking TS Compiler API Docs label Jul 22, 2022
@ezolenko ezolenko merged commit e75d97a into ezolenko:master Aug 8, 2022
@agilgur5 agilgur5 deleted the optim-document-registry branch June 30, 2023 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: optimization Performance, space, size, etc improvement scope: watch mode Related to Rollup's watch mode topic: TS Compiler API Docs Related to the severely lacking TS Compiler API Docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants