Running heavy filesystem operations in separate processes #899

akosyakov · 2017-11-28T14:02:52Z

No description provided.

svenefftinge

It sounds like it would be a good idea in general to run watching processes in separated threads. Could you generalize the forwarding so that we can reuse it more easily for the git watcher, too?

svenefftinge · 2017-11-28T16:00:02Z

packages/filesystem/src/common/filesystem-watcher-protocol.ts

-        this.proxy.onDidOpenConnection(() => this.reconnect());
+        const onInitialized = this.proxy.onDidOpenConnection(() => {
+            onInitialized.dispose();
+            this.proxy.onDidOpenConnection(() => this.reconnect());


Could you explain why this?

If the backend is restarted then the frontend should reconnect to it.
If the chokidar watcher process is restarted then the backend should reconnect to it.

Here it skips the first connection and on following connections does reconnection. We used to have duplicate Started watching because the first connection was not skipped.

ok, thanks. A comment in code could help me next time I need to understand it :)

akosyakov · 2017-11-29T16:14:34Z

It sounds like it would be a good idea in general to run watching processes in separated threads. Could you generalize the forwarding so that we can reuse it more easily for the git watcher, too?

Watching for git is cheap, locating of repositories is expensive.

I've extracted the common ipc json-rpc connection provider and used it for filesystem watching and running repositories look up in separate processes.

Also, repositories look up now consists of 2 phases:

discovering the containing repo - fast look up
discovering all contained repos - expensive, executed in the separate process

akosyakov · 2017-11-29T16:15:30Z

Please review.

@hexa00 Could you test that the app is now responsive even with a user home as a workspace root?

kittaakos · 2017-11-29T16:22:43Z

I'll take care of the Windows verification.

akosyakov · 2017-11-29T16:24:24Z

@kittaakos if you can have a look that there are no dangling processes on the server shutdown on windows, it would be nice.

kittaakos · 2017-11-29T16:36:53Z

packages/core/src/node/messaging/ipc-connection-provider.ts

+            silent: true,
+            env: {
+                ...process.env
+            },


Just asking to squeeze some additional knowledge from you: it is the same as env: process.env, right?

no, it copies properties to a new object, later 2 more properties added to the new object but not to process.env

svenefftinge · 2017-11-29T20:38:58Z

packages/git/src/common/git-model.ts

+        if (repository && repository2) {
+            return repository.localUri === repository2.localUri;
+        }
+        return repository === repository2;


That is undefined === undefined. Maybe the first arg doesn't need to be optional?

there is a client which can pass both undefined: https://github.com/theia-ide/theia/blob/a8389fa95a6132ef6af13c0c0f7de9f5846ca05b/packages/git/src/browser/git-repository-provider.ts#L73

svenefftinge · 2017-11-29T20:42:44Z

packages/git/src/browser/git-repository-provider.ts

-     *  - If no repositories are available, leaves the selected one as `undefined`.
-     *  - If the previously selected one, does not exist among the most recently discovered one, selects the first one.
-     *  - This method blocks, if the workspace root is not yet set.
-     */
    async refresh(): Promise<void> {


Wouldn't it a bit simpler to have a limit option. Which we set to 1 only when initially called from constructor and later always fetch all repos (replacing everything). I think we wouldn't need all the logic below nor the notion of a containingRepository.

What should be returned if there is no containing repo, the first contained repo?

And I think the logic will be needed anyway, otherwise it will flicker from one repository to several

It should just return the first n results and then stop. Why would it flicker?

It shows several repositories, you press refresh, it shows one repository, then it shows several repositories again.

hexa00 · 2017-11-29T20:48:32Z

The UI is responsive !:)

I get these errors in the browser console however:

messages.js:46 Uncaught (in promise) Error: Request locate failed with message: EACCES: permission denied, scandir '/lost+found'
    at new ResponseError (messages.js:46)
    at handleResponse (main.js:421)
    at processMessageQueue (main.js:249)
    at main.js:233
    at run (setImmediate.js:40)
    at runIfPresent (setImmediate.js:69)
    at onGlobalMessage (setImmediate.js:109)

Strangly I get much more errors like this in the backend but these find their way to the frontend

hexa00 · 2017-11-29T20:55:47Z

However once the nodejs memory limit is reached the other process is at 100% cpu for minutes now...

But that's another problem I guess.. still it's a very good step forward.

hexa00

I'll continue to review the code tomorrow, I'm out of time today

hexa00 · 2017-11-29T21:03:07Z

packages/core/src/common/messaging/connection-error-handler.ts

+    }
+
+    protected readonly restarts: number[] = [];
+    shouldRestart(): string | undefined {


Seems this should be shouldRestart ( { reason: string }): boolean ?

Or I'm thinking in C too much and it should be shouldResart() : { success: boolean, reason: string }

I've changed with logging in the error handler.

akosyakov · 2017-11-30T05:35:45Z

Strangly I get much more errors like this in the backend but these find their way to the frontend

@hexa00 I think they are from our filesystem, not from the watcher. Could you file an issue that the filesystem should handle EACCES please?

akosyakov · 2017-11-30T05:39:05Z

However once the nodejs memory limit is reached the other process is at 100% cpu for minutes now...

Ideas what we can do about it? Should we have timeout and stop the operation? Like if watching cannot be started during 5mins?

akosyakov · 2017-11-30T10:29:31Z

We've discussed to limit deep of git repositories look up and time out the startup of file watching.

hexa00 · 2017-11-30T12:34:11Z

@hexa00 I think they are from our filesystem, not from the watcher. Could you file an issue that the filesystem should handle EACCES please?

OK #915

hexa00 · 2017-11-30T12:36:36Z

Ideas what we can do about it? Should we have timeout and stop the operation? Like if watching cannot be started during 5mins?

I think best would be a max number of watches, if we can determine the memory taken by a watch we could limit it so that we don't use >70% of the available node memory for that?

@simark since you've checked this before would you have an idea on those numbres (memory used per watch ? / max number of watch to allow)

hexa00 · 2017-11-30T12:38:15Z

Actually I think watching a directory can be fast it's just when the memory limit is reached everything is slow in nodejs as it tries to free memory to allocate memory all the time. Feels like the equivalent of a system using it's swap.

We should guard against that in general in theia I think otherwise just opening a big file in the editor may have the same result.

akosyakov · 2017-11-30T12:44:17Z

I think best would be a max number of watches, if we can determine the memory taken by a watch we could limit it so that we don't use >70% of the available node memory for that?

I don't get it, there is only one watcher for the workspace root per a window.

Actually I think watching a directory can be fast it's just when the memory limit is reached everything is slow in nodejs as it tries to free memory to allocate memory all the time. Feels like the equivalent of a system using it's swap.

It takes 10s to start up for Theia, i meant if it cannot start up for example for 1 min then timeout the process.

We should guard against that in general in theia I think otherwise just opening a big file in the editor may have the same result.

It is different contexts, Monaco can handle big files, e.g. average of the file in TS repo 20 LOC. I would like to go with the simple approach now and if it does not work well, reconsider it later.

akosyakov · 2017-11-30T12:54:35Z

There are also watchers for preferences and package.json files, but they are cheap. We can optimize by watching single files by one watcher or watching the same repo by one watcher for all clients. But I would do it in separate PRs as we experience issues.

akosyakov · 2017-12-01T07:21:34Z

I've removed a constraint by depth on git and used https://github.com/implausible/find-git-repositories. It does not burn CPU time and I need about 3 mins to discover all git repositories on my machine (250G of data).

Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

separate process Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

kittaakos · 2017-12-01T08:47:50Z

I am trying it out on Windows now. Please note, I have only two cores for the image, so that will be a real stress-test.

kittaakos · 2017-12-01T10:09:39Z

Performance: awesome.
Stale processes after application close: none.
File change notifications: did not check, we have test failures.
Problems:
- Cannot open deleted files in "diff" view. (I believe, this is unrelated to you change.)
- Select a repository in the GIt tab, delete the repository from the FS. Git tab does not get updated, the non-existing git repository is still in the drop-down. It used to work. (Note, the error logging is fine as expected. This entry is rather about the update. But I am more than happy with this minor thing, performance is really great.): (See follow-up task: [git] Selected git repository does not get updated on repository deletion #927)

The command `git status --untracked-files=all --branch --porcelain=2 -z` exited with an unexpected code: 128. The caller should either handle this error, or expect that exit code.

fatal: Not a git repository (or any of the parent directories): .git

(The error was parsed as 27: This is not a git repository.)

[2017-12-01T10:04:33.666Z] ERROR: Theia/756 on DESKTOP-VJRAMKC:
    Error occurred while synchronizing the status of the repository. [ 'file:///c%3A/Users/kittaakos/dev/tmp-git/sadlos2',
      { GitError: This is not a git repository.
          at new GitError (C:\Users\kittaakos\dev\theia\node_modules\dugite-extra\lib\core\git.js:79:28)
          at Object.<anonymous> (C:\Users\kittaakos\dev\theia\node_modules\dugite-extra\lib\core\git.js:155:27)
          at step (C:\Users\kittaakos\dev\theia\node_modules\dugite-extra\lib\core\git.js:50:23)
          at Object.next (C:\Users\kittaakos\dev\theia\node_modules\dugite-extra\lib\core\git.js:31:53)
          at fulfilled (C:\Users\kittaakos\dev\theia\node_modules\dugite-extra\lib\core\git.js:22:58)
          at <anonymous>
        name: 'GitError',
        result:
         { stdout: '',
           stderr: 'fatal: Not a git repository (or any of the parent directories): .git\n',
           exitCode: 128,
           gitError: 27,
           gitErrorDescription: 'This is not a git repository.' },
        args:
         [ 'status',
           '--untracked-files=all',
           '--branch',
           '--porcelain=2',
           '-z' ] } ]

kittaakos · 2017-12-01T12:24:08Z

I have tried out nsfw outside from Theia, and there is an odd thing on Windows. When I create a file with touch xxx, I receive an array of events; the first item is ADDED and twice MODIFIED. (Just wanted to share.)

Update: it does not happen if I use fs.createFileSync. This could be due to touch.

kittaakos · 2017-12-01T12:44:19Z

packages/filesystem/src/node/node-filesystem.spec.ts

-import { ChokidarFileSystemWatcherServer } from './chokidar-filesystem-watcher';
+import { NsfwFileSystemWatcherServer } from './nsfw-watcher/nsfw-filesystem-watcher';
+
+// tslint:disable:no-unused-expression


Since you have already cleaned up the test setup, perhaps, you could get rid of chai-as-promised too, and use async, can you? (First, let's have green tests on the CI.)

Follow-up: #928

akosyakov · 2017-12-01T13:55:53Z

There are some quirks with nsfw, like it cannot watch symlinked directories, but it is going to be fixed and pushed by vscode developers. I would like to go with nsfw anyway, it is quite an improvement compared to chokidar.

Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

…nig by depth Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

akosyakov · 2017-12-01T14:46:14Z

Builds are green! Objections to merge? @svenefftinge I've changed quite a lot since your last review.

hexa00 · 2017-12-01T16:18:25Z

Good with me.

Only detail could we document somewhere that the symlinks don't work ?

hexa00 · 2017-12-01T16:18:51Z

Or report it to the user ?

akosyakov · 2017-12-01T16:29:00Z

Or report it to the user ?

I will wait till someone hits it. There are also other issues, you can look at https://github.com/Axosoft/nsfw/issues. It is big effort trying to prevent all of them while Theia is not used extensively. if we will often stumble on them I am fine to start thinking about it.

Also, I am not sure that we can detect it since all important code is native, not js.

hexa00 · 2017-12-01T18:23:15Z

OK fine with me with luck it will be fixed by the time it's really an issue.

akosyakov requested a review from svenefftinge November 28, 2017 14:20

svenefftinge reviewed Nov 28, 2017

View reviewed changes

akosyakov force-pushed the ak/watching_in_separate_process branch from 84cfb48 to 6e36d97 Compare November 29, 2017 16:10

akosyakov requested review from hexa00 and kittaakos November 29, 2017 16:15

akosyakov changed the title ~~[filesystem] run watching in the separate process~~ Running heavy filesystem operations in separate processes Nov 29, 2017

akosyakov force-pushed the ak/watching_in_separate_process branch from 6e36d97 to 8b9d5be Compare November 29, 2017 16:34

kittaakos reviewed Nov 29, 2017

View reviewed changes

akosyakov force-pushed the ak/watching_in_separate_process branch 4 times, most recently from 29e17e5 to a8389fa Compare November 29, 2017 16:46

svenefftinge reviewed Nov 29, 2017

View reviewed changes

hexa00 reviewed Nov 29, 2017

View reviewed changes

akosyakov force-pushed the ak/watching_in_separate_process branch from a8389fa to a29b806 Compare November 30, 2017 06:55

hexa00 mentioned this pull request Nov 30, 2017

Handle EACCESS in the filesystem #915

Open

akosyakov force-pushed the ak/watching_in_separate_process branch 2 times, most recently from e29262b to 1330067 Compare December 1, 2017 07:57

akosyakov added 3 commits December 1, 2017 09:13

[filesystem] run watching in the separate process

8ba39dc

Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

[core] extracted the ipc json-rpc connection provider

102e9e4

Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

[git] locate repositories in 2 phases + execute 2nd phase in the

ad82299

separate process Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

akosyakov force-pushed the ak/watching_in_separate_process branch 3 times, most recently from b42dfaa to 2bd7958 Compare December 1, 2017 08:36

kittaakos reviewed Dec 1, 2017

View reviewed changes

hexa00 mentioned this pull request Dec 1, 2017

[editor] Renaming a file should rename a corresponding editor #230

Closed

akosyakov force-pushed the ak/watching_in_separate_process branch from 2bd7958 to 5f7a52f Compare December 1, 2017 13:52

kittaakos mentioned this pull request Dec 1, 2017

[git] Selected git repository does not get updated on repository deletion #927

Closed

akosyakov added 2 commits December 1, 2017 15:30

[filesystem] switched from chokidar to nsfw for watching

95f2e9f

Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

[git] use native modules to find git repositories instead of constrai…

50274d5

…nig by depth Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

akosyakov force-pushed the ak/watching_in_separate_process branch from 5f7a52f to 50274d5 Compare December 1, 2017 14:32

svenefftinge approved these changes Dec 2, 2017

View reviewed changes

akosyakov merged commit 9f60988 into master Dec 2, 2017

akosyakov deleted the ak/watching_in_separate_process branch December 2, 2017 17:16

This was referenced Dec 2, 2017

Opening a large workspace (like a home directory) makes Theia unresponsive and can crash it #671

Closed

Settings files are watched several more times on startup (3 times more than they should) #748

Closed

Running heavy filesystem operations in separate processes #899

Running heavy filesystem operations in separate processes #899

Conversation

akosyakov commented Nov 28, 2017

svenefftinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

svenefftinge Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

akosyakov commented Nov 29, 2017 • edited Loading

akosyakov commented Nov 29, 2017 • edited Loading

kittaakos commented Nov 29, 2017

akosyakov commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hexa00 commented Nov 29, 2017 • edited Loading

hexa00 commented Nov 29, 2017

hexa00 left a comment

Choose a reason for hiding this comment

hexa00 Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

hexa00 Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov commented Nov 30, 2017

akosyakov commented Nov 30, 2017

akosyakov commented Nov 30, 2017

hexa00 commented Nov 30, 2017

hexa00 commented Nov 30, 2017

hexa00 commented Nov 30, 2017 • edited Loading

akosyakov commented Nov 30, 2017

akosyakov commented Nov 30, 2017

akosyakov commented Dec 1, 2017

kittaakos commented Dec 1, 2017

kittaakos commented Dec 1, 2017 • edited Loading

kittaakos commented Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov commented Dec 1, 2017

akosyakov commented Dec 1, 2017 • edited Loading

hexa00 commented Dec 1, 2017

hexa00 commented Dec 1, 2017

akosyakov commented Dec 1, 2017 • edited Loading

hexa00 commented Dec 1, 2017

akosyakov Nov 28, 2017 •

edited

Loading

svenefftinge Nov 28, 2017 •

edited

Loading

akosyakov commented Nov 29, 2017 •

edited

Loading

akosyakov commented Nov 29, 2017 •

edited

Loading

hexa00 commented Nov 29, 2017 •

edited

Loading

hexa00 Nov 29, 2017 •

edited

Loading

hexa00 Nov 29, 2017 •

edited

Loading

hexa00 commented Nov 30, 2017 •

edited

Loading

kittaakos commented Dec 1, 2017 •

edited

Loading

kittaakos commented Dec 1, 2017 •

edited

Loading

akosyakov commented Dec 1, 2017 •

edited

Loading

akosyakov commented Dec 1, 2017 •

edited

Loading