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

Only start one tsserver, not one per tsconfig #256

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sandersn
Copy link
Contributor

This is how tsserver is intended to be used, according to Typescript team members who work on it. This improves the usability of a long-lived emacs instance after opening many different projects.

I'm testing the change right now and so far performance is better. I don't know whether there are deep-seated reasons to have one tsserver per project root, so I thought that we could discuss that with this PR.

I'm not sure whether initialising tide-server with defvar to nil and updating it with setq are right since I don't know elisp that well.

@josteink
Copy link
Collaborator

josteink commented May 18, 2018

Historically tide only supported working against one particular version of tsserver (shipped with tide or overridden, custom path set by the user). In such circumstances, this change would make sense.

Now however, tide defaults to using the same tsserver as provided by the typescript npm package installed in each and every project it works with.

This ensures tide handles things consistently with the projects specific version of tsc, the typescript compiler, automatically.

This was a “big” (although fairly silent) feature with lots of discussion and code-review in the PRs.

Having only one instance of tsserver would directly invalidate all that work invested in that undertaking, so that’s not immediately desired, at least not for all users.

While I can see the argument for preserving resources, it would have to be under very specific constraints, like always using a globally installed tsserver as opposed to the projects’ own.

@sandersn
Copy link
Contributor Author

What about one tsserver per tsserver location instead of one per project? tide-servers could use tide-locate-tsserver-executable to get the hash key instead of tide-project-name.

@lddubeau
Copy link
Collaborator

I have some repos that have multiple tsconfig.json files. Often, there's one for the application proper, and one for running the test suite. Something like

src/foo.ts
src/tsconfig.json
test/foo.spec.ts
test/tsconfig.json

Way back when, tsserver did not play well with this kind of setup. If I started editing src/foo.ts and then moved to edit test/foo.spec.ts, then I would get behavior out of tsserver that was consistent with it incorrectly using src/tsconfig.json for processing test/foo.spec.ts instead of correctly using test/tsconfig.json. And if I opened the files in the reverse order then src/foo.ts was not handled properly.

This, in combination with issue reports like this one (this comment, specifically), gave me the impression that tsserver is not designed to handle multiple tsconfig.json files at the same time.

@sandersn
Copy link
Contributor Author

@lddubeau Are you saying the current behaviour of hashing tsservers on tide-project-name is better or worse than hashing on tide-locate-tsserver-executable? Or would the bug you linked be the same no matter how many tsservers you start?

@lddubeau
Copy link
Collaborator

@sandersn The current behavior of hashing on tide-project-name is better than hashing on tide-locate-tsserver-executable. Hashing on the location of tsserver in the scenario I gave above would result in a single tsserver handling source files that are governed by different tsconfig.json files, and in my experience, that did not work.

Hashing on tide-project-name means one instance of tsserver per tsconfig.json. tide-project-name looks up the file system for a tsconfig.json and then takes the basename of the directory that contains the tsconfig.json and appends a hash to it to work around clashes. In the example I gave above, there would be two project names: src-[some hash] and test-[some other hash], and two instances of tsserver.

@sandersn
Copy link
Contributor Author

Assuming that tsserver still has the same problem from microsoft/TypeScript#5828, maybe we should wait until Typescript supports dependencies between tsconfigs, which should be coming in 3.0 or 3.1. That feature exists to make src and test relate correctly.

To recap, it seems like there are two reasons to start one tsserver per tsconfig.

  1. Support local installs of typescript.
  2. Work around src-vs-test-interaction bugs in tsserver.

If/when (2) is fixed, then I think hashing on tide-locate-tsserver-executable would be more efficient and therefore strictly more desirable, right?

@lddubeau
Copy link
Collaborator

@sandersn Yep, when the 2nd point is fixed it will be better to hash per location to avoid starting so many instances.

@sandersn
Copy link
Contributor Author

@lddubeau Also, if it's been a long time since you observed the bug, it may have been fixed. You can test it by using a constant hash key like "DUMMY" in the gethash line in tide-current-server and the puthash line in tide-start-server.

I realise it's an imposition to ask you to test whether a bug has been fixed, but I'd like to solve real bugs now (for me, running out of file handles when running VS code at the same time as Emacs) instead of avoiding old bugs that have been fixed.

@ananthakumaran
Copy link
Owner

ananthakumaran commented May 19, 2018

We could introduce a config tide-reuse-tsserver and set it to nil by default until all the bugs are sorted out in tsserver. If enabled, it would search for existing tsserver process with same path and reuse it.

Codewise, we assume one server would be used by one project in some places like tide-restart-server, tide-net-sentinel. We also store the project-name in process.

(process-put process 'project-name (tide-project-name))

I guess we could convert it into project-names and also add another process property tsserver-path which could be used to find server for reuse.

@lddubeau
Copy link
Collaborator

@sandersn I tried it just now. It is still buggy. Initially, it looks like it is working but I soon get bizarre behavior, like hitting 100% CPU on tsserver until I kill it. It all goes away when I revert to tide's stock behavior.

@ananthakumaran Yes, I was thinking it should be configurable. It is very likely that users of tide won't be able to move all their projects at once to the new version of TS that has a tsserver that handles multiple projects without error. These users will want to use the current method of starting tsserver until they no longer have to deal with the old versions.

@sandersn
Copy link
Contributor Author

Further experimentation with this branch revealed bad behaviour for my Typescript-compiler-development scenario as well (details below). I don't have time for investigation now, so I guess I'll live with the resource usage.

  1. Point tsserver to require("~/ts/built/local/tsserver.js")
  2. Open two projects: ~/ts/src/compiler and ~/test. The locally-built tsserver starts once, for the first project.
  3. Rebuild the tsserver at ~/ts/built/local/tsserver.
  4. Switch to a source file in ~/ts/src/compiler and request the type of something. The minibuffer shows part of a stack trace.
  5. tide-restart-server. Now tsserver is working in ~/ts/src/compiler.
  6. Switch to a source file in ~/test and request the type of something. The minibuffer again shows part of a stack trace.
  7. tide-restart-server. No tsserver is working in ~/test.
  8. Goto 4 or give up.

elibarzilay added a commit to elibarzilay/tide that referenced this pull request Oct 30, 2019
There are two things here that are both aimed at reducing the amount of
resource clutter:

1. I've seen many tmp files left.  Looking at the code, it seems to me
   like a reasonable thing to get rid of them when a file is saved.  I
   did this via adding `tide-remove-tmp-file` on `after-save-hook`.

2. I also saw that there is a general tendency to accumulate many
   servers since they're never removed (actually, more than just the
   servers -- the whole project resources are kept, but the server is
   the main problem wrt resources).  This is also mentioned in ananthakumaran#256.

   So I implemented a function that scans all buffers and cleanup all
   projects that have no live buffers.  It looks to me like a good idea
   to do this, since you can just kill old buffers to reduce resource
   usage.  (And killing old buffers is more obvious than explicitly
   openning the server list to kill old ones, especially since there's
   no way to tell if a server is used by some buffer or not.)
elibarzilay added a commit to elibarzilay/tide that referenced this pull request Oct 30, 2019
There are two things here that are both aimed at reducing the amount of
resource clutter:

1. I've seen many tmp files left.  Looking at the code, it seems to me
   like a reasonable thing to get rid of them when a file is saved.  I
   did this via adding `tide-remove-tmp-file` on `after-save-hook`.

2. I also saw that there is a general tendency to accumulate many
   servers since they're never removed (actually, more than just the
   servers -- the whole project resources are kept, but the server is
   the main problem wrt resources).  This is also mentioned in ananthakumaran#256.

   So I implemented a function that scans all buffers and cleanup all
   projects that have no live buffers.  It looks to me like a good idea
   to do this, since you can just kill old buffers to reduce resource
   usage.  (And killing old buffers is more obvious than explicitly
   openning the server list to kill old ones, especially since there's
   no way to tell if a server is used by some buffer or not.)

   (I added this function onto `kill-buffer-hook`, but if that's too
   extreme, then a more mild option is to not do that and just let
   people add it themselves.)
elibarzilay added a commit to elibarzilay/tide that referenced this pull request Oct 30, 2019
There are two things here that are both aimed at reducing the amount of
resource clutter:

1. I've seen many tmp files left.  Looking at the code, it seems to me
   like a reasonable thing to get rid of them when a file is saved.  I
   did this via adding `tide-remove-tmp-file` on `after-save-hook`.

2. I also saw that there is a general tendency to accumulate many
   servers since they're never removed (actually, more than just the
   servers -- the whole project resources are kept, but the server is
   the main problem wrt resources).  This is also mentioned in ananthakumaran#256.

   So I implemented a function that scans all buffers and cleanup all
   projects that have no live buffers.  It looks to me like a good idea
   to do this, since you can just kill old buffers to reduce resource
   usage.  (And killing old buffers is more obvious than explicitly
   openning the server list to kill old ones, especially since there's
   no way to tell if a server is used by some buffer or not.)

   (I added this function onto `kill-buffer-hook`, but if that's too
   extreme, then a more mild option is to not do that and just let
   people add it themselves.)
elibarzilay added a commit to elibarzilay/tide that referenced this pull request Oct 30, 2019
There is a general tendency to accumulate many servers since they're
never removed (actually, more than just the servers -- the whole project
resources are kept, but the server is the main problem wrt resources).
This is also mentioned in ananthakumaran#256.

So I implemented a function that scans all buffers and cleanup all
projects that have no live buffers.  It looks to me like a good idea to
do this, since you can just kill old buffers to reduce resource
usage.  (And killing old buffers is more obvious than explicitly
openning the server list to kill old ones, especially since there's no
way to tell if a server is used by some buffer or not.)

(I added this function onto `kill-buffer-hook`, but if that's too
extreme, then a more mild option is to not do that and just let people
add it themselves.)
elibarzilay added a commit to elibarzilay/tide that referenced this pull request Nov 23, 2019
There is a general tendency to accumulate many servers since they're
never removed (actually, more than just the servers -- the whole project
resources are kept, but the server is the main problem wrt resources).
This is also mentioned in ananthakumaran#256.

So I implemented a function that scans all buffers and cleanup all
projects that have no live buffers.  It looks to me like a good idea to
do this, since you can just kill old buffers to reduce resource
usage.  (And killing old buffers is more obvious than explicitly
openning the server list to kill old ones, especially since there's no
way to tell if a server is used by some buffer or not.)

(I added this function onto `kill-buffer-hook`, but if that's too
extreme, then a more mild option is to not do that and just let people
add it themselves.)
elibarzilay added a commit to elibarzilay/tide that referenced this pull request Nov 29, 2019
There is a general tendency to accumulate many servers since they're
never removed (actually, more than just the servers -- the whole project
resources are kept, but the server is the main problem wrt resources).
This is also mentioned in ananthakumaran#256.

So I implemented a function that scans all buffers and cleanup all
projects that have no live buffers.  It looks to me like a good idea to
do this, since you can just kill old buffers to reduce resource
usage.  (And killing old buffers is more obvious than explicitly
openning the server list to kill old ones, especially since there's no
way to tell if a server is used by some buffer or not.)

(I added this function onto `kill-buffer-hook`, but if that's too
extreme, then a more mild option is to not do that and just let people
add it themselves.)
elibarzilay added a commit to elibarzilay/tide that referenced this pull request Nov 29, 2019
There is a general tendency to accumulate many servers since they're
never removed (actually, more than just the servers -- the whole project
resources are kept, but the server is the main problem wrt resources).
This is also mentioned in ananthakumaran#256.

So I implemented a function that scans all buffers and cleanup all
projects that have no live buffers.  It looks to me like a good idea to
do this, since you can just kill old buffers to reduce resource
usage.  (And killing old buffers is more obvious than explicitly
openning the server list to kill old ones, especially since there's no
way to tell if a server is used by some buffer or not.)

(I added this function onto `kill-buffer-hook`, but if that's too
extreme, then a more mild option is to not do that and just let people
add it themselves.)
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.

4 participants