SqrTT / prophet

Prophet Debugger (SFCC sandboxes via SDAPI 2.0) extension for VS Code
https://marketplace.visualstudio.com/items?itemName=SqrTT.prophet
Other
144 stars 59 forks source link

Reduce CPU consumption of the file watching #51

Closed SGD1953 closed 6 years ago

SGD1953 commented 7 years ago

CPU consumption is very high (at least on MacOS X) and goes back to normal when uploading is disabled. I tracked this down to chokidar which apparently has an issue (paulmillr/chokidar#412) and replaced it with a native solution. It is a bit workaround-like due to API contraints but does the job at much lower CPU load.

SqrTT commented 7 years ago

I've disabled pooling in 0.9.5 realise. Does it have some effect on CPU?

SGD1953 commented 7 years ago

No, still bad. Using createFileSystemWatcher() it just stays completely normal.

image

ghost commented 7 years ago

I'm agree with @SGD1953, FileSystemWatcher is the best way to watch files in VSCode

SqrTT commented 7 years ago

Using FileSystemWatcher we loose availability to exclude files, ie. ('node_nodules', client side files, etc)

SGD1953 commented 7 years ago

We don't, https://github.com/SqrTT/prophet/pull/47 includes poor-man's exclusions. Could probably be polished a bit more to allow for proper globbing by using a globbing lib but as the exclusions were hard-coded it gives the same result as before (I even tested it ;)).

ghost commented 7 years ago

@SqrTT Nope. Workspace -> createFileSystemWatcher. You can use glob patterns

SqrTT commented 7 years ago

@SGD1953 I saw your solution, you attach handlers for all files but ignore events if it is fired for "ignored file". Watching a node_module folder might consume a lot of memory. I dislike a code which does some job but the result of this job is ignored afterward. :(

Probably we should consider similar config.

SqrTT commented 7 years ago

@ufnd create FileSystemWatcher does not allow exclusions :(

SGD1953 commented 7 years ago

I'm not in love with the solution to be clear, it took me a while to figure out that the globbingPattern is basically a single-path glob with no exclusions. However it leaves my machine idle with upload turned on while with chokidar I have to tape it to the desk so it does not take off. We could emulate some better APIs by wrapping the watch stuff into a separate class, but it simply produces the best result at the moment.

ghost commented 7 years ago

@SqrTT @SGD1953 Maybe we can use multiple fileWatchers, for every cartridge and manage them? Or we will need to use fsEvents for Mac

SqrTT commented 7 years ago

The issue is that fsevents are not used by chokidar (chokidar is actually a wrapper around native watchers). I've added message about this in output... but I have no chance to find out why it fsevents is disabled :(

SqrTT commented 7 years ago

I meant chokidar must use fsevents for Mac OS. But by some reason, it uses pooling as a fallback.

SGD1953 commented 7 years ago

I you feel better I could refactor it in a way that we can select which way the files are watched

SqrTT commented 7 years ago

Unfortunately, I didn't found a way to fix chokidar for vscode, basically, the problem is described here and in QA section. fsevents use native modules hence no way to compile it for all MacOS targets and versions of nodejs. So this means we are getting rid of chokidar.

@SGD1953 If you feel better I could refactor it in a way that we can select which way the files are watched

How would you achieve that?

SGD1953 commented 7 years ago

I could wrap it into some chokidar-like API so that you can just use it the same way, hiding the ugliness away into its own module which handles the multiple watchers, poor-man's excludes etc.

SqrTT commented 7 years ago

@SGD1953 please confirm if this works for you.

Also, there is possibility to exclude files from watching via config files.watcherExclude for workspace

SGD1953 commented 7 years ago

Nice one, I agree that this would be the cleanest approach. We can then simply document some recommended excludes in case folks are not aware.

SGD1953 commented 7 years ago

This would include all watching on those locations, currently we have excluded

            'node_modules/',
            '.git/',
            `cartridge/js/`,
            `cartridge/client/`

Which would give us a slightly different result.

I guess the first 2 make sense to be excluded from watching but the latter ones maybe not so much as another extension might want to check those folder in order to compile the JS.

SqrTT commented 6 years ago

can this be closed?

SGD1953 commented 6 years ago

Current solution works like a charm for me, so yes.