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

Different watchFile and watchDirectory options through environment variable #21243

Merged
merged 30 commits into from
Mar 8, 2018

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Jan 17, 2018

fs.watch and fs.watchFile provided by node, both have pros and cons.
fs.watch uses file system events to notify the changes in the file/directory. But this is OS dependent and the notification is not completely reliable and does not work as expected on many OS. Also there could be limit on number of watches that can be created, eg. linux and we could exhaust it pretty quickly with programs that include large number of files. But because this uses file system events, there is not much CPU cycle involved.
fs.watchFile uses polling and thus involves CPU cycles. But this is the most reliable mechanism to get the update on the status of file/directory.

By default we use to use fs.watchFile to watch source files, config files and missing files (missing file references) that means the CPU usage depends on number of files in the program.
fs.watch is used to watch directories (eg. source directories included by config file, failed lookup location directories etc) These can handle the missing precision in notifying about the changes. But recursive watching is supported on only Windows and OSX. That means we need something to replace the recursive nature on other OS.

This PR provides the different watchFile and watchDirectory options through environment variable:
watching of files is selected by setting TSC_WATCHFILE to

  • "PriorityPollingInterval" - use fs.watchFile but will use different polling intervals for source files, config files and missing files.
  • "DynamicPriorityPolling" - use a dynamic queue where in the frequently modified files will be polled at shorter interval and the files unchanged will be polled less frequently
  • "UseFsEvents" - use fs.watch which uses file system events (but might not be accurate on different OS) to get the notifications for the file changes/creation/deletion. Note that few OS eg. linux has limit on number of watches and failing to create watcher using fs.watch will result it in creating using fs.watchFile
  • "UseFsEventsWithFallbackDynamicPolling" - this is similar to "UseFsEvents" except on failing to create watch using fs.watch, the fallback watching happens through dynamic polling queues (as explained in "DynamicPriorityPolling")
  • UseFsEventsOnParentDirectory - this watched parent directory of the file with fs.watch (using file system events)
  • default is to use fs.watchFile with 250ms as the timeout for any file watching

The watch directory on platforms that dont support recursive directory watching is supported through recursively creating directory watcher for the child directories using different options selected by TSC_WATCHDIRECTORY

  • "RecursiveDirectoryUsingFsWatchFile" - use fs.watchFile to watch the directories
  • "RecursiveDirectoryUsingDynamicPriorityPolling" - use dynamic polling queue to poll changes to the directory
  • default is to use fs.watch to watch child directories

This is prototype is to get feedback for issues: #19762, #19989, #20017, #20023

@sheetalkamat
Copy link
Member Author

The drop with these options: localBuilt.zip

@jiripospisil
Copy link

I tested the various options with TypeORM (~20k cloc) on macOS 10.13.3 (APFS), CPU values read from Activity Monitor with 1s Update Frequency.

Option idle CPU
not specified, default 16-18%
PriorityPollingInterval 16-18%
DynamicPriorityPolling 0.3%
UseFsEvents 0.0%
UseFsEventsWithFallbackDynamicPolling 0.0%
UseFsEventsOnParentDirectory 0.0%

Each time I modified a few files to see if the changes are noticed (they were) and also to see how long it takes to notice the change (with UseFsEvents being the clear winner). A few issues:

  1. With UseFsEvents I modified a file FindOptionsUtils.ts and received an error saying the file didn't exist (error TS6053: File '(...)/FindOptionsUtils.ts' not found.). The file was edited with Vim, I assume Vim first writes the buffer into a temporary file and then moves it to the
    final destination. Maybe this confuses the event watcher? It happened just once, probably a timing issue.

  2. With DynamicPriorityPolling I received a bunch of File change detected events even though I didn't change anything.

I'm not sure if this is particularly helpful to you so feel free to suggest some scenarios people should test. I currently use TSC_NONPOLLING_WATCHER=true with 2.7.1 in one of our apps because otherwise the CPU usage is around 20% all the time and that's way too much.

@sheetalkamat
Copy link
Member Author

@jiripospisil Thank you for trying this out and getting back.

  • UseFsEvents does have implications with operating system, especially if inode is removed, there wont be new file events being sent and that would explain why you are noticing those errors.
  • For file change detected, we would need to investigate further to see if its those file changes are being detected, or some directory watcher is being invoked.. You would want to run the tsc with --extendedDiagnostics to get detailed analysis of which file changes are being triggering these file changed events
    Note that TSC_NONPOLLING_WATCHER=true is same as UseFsEventsOnParentDirectory

@sheetalkamat
Copy link
Member Author

@mhegazy The PR is updated to add the custom polling intervals etc to get more feedback on it as well and ready to merge as then it would be available in nightlys to get more feedback.

mtime: Date;
}

/* @internal */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const enum ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sheetalkamat
Copy link
Member Author

Ping @mhegazy


const nodeVersion = getNodeMajorVersion();
const isNode4OrLater = nodeVersion >= 4;
nodeSystem.watchFile = getWatchFile();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need nodeSystem in these function calls? can we just use the functions directelly and move these in the object literal above?

return useNonPollingWatchers ?
createNonPollingWatchFile() :
// Default to do not use polling interval as it is before this experiment branch
(fileName, callback) => fsWatchFile(fileName, callback);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the lambda needed? why not just return fsWatchFile;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We weren't passing the polling interval before to the fsWatchFile and hence the lambda.. (to ignore pollingInterval being passed)

@sheetalkamat sheetalkamat merged commit 17b10dc into master Mar 8, 2018
@sheetalkamat sheetalkamat deleted the watchOptions branch March 8, 2018 20:44
@amcasey
Copy link
Member

amcasey commented Mar 12, 2018

I'm still investigating but it looks like, after this change, every poll of a non-existent folder (in particular, bower_components) results in a file-changed callback in the typings installer, which (incorrectly) concludes that it has been deleted and triggers an invalidate event. This event propagates through the TS server to VS and triggers an unnecessary project refresh.

@sheetalkamat
Copy link
Member Author

Are you using some environment variable to change default behavior? tsserver logs would be helpful to investigate..

@amcasey
Copy link
Member

amcasey commented Mar 12, 2018

So far, the most suspicious change in my environment is using Node 8. I'm trying to get set up on a clean box.

@amcasey
Copy link
Member

amcasey commented Mar 12, 2018

Filed #22494

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants