Overflows #155
Merged
Overflows #155
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eed3si9n
previously approved these changes
May 19, 2018
| @@ -146,11 +155,62 @@ private[sbt] object EventMonitor { | |||
| notExist.foreach(s.unregister) | |||
| updatedFiles ++ newFiles ++ notExist | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
@eatkins Thanks for the descriptive commit message on this commit.
The handleFileDescendant method was recursively constructing a list of results by calling File.list and recursively calling back into itself for each directory that it found in the listed files. There were two performance bugs in this approach. One was that handleFileDescendant actually called File.list twice for each directory. The other was that after it got the second list of files, it had to filter the results based on whether or not it was a directory. When I called jstack on my sbt process during a slow test run, invariably one of the threads was bloced on File.isDirectory. The fix is simple, if verbose. Use Files.walkFileTree instead. It is verbose, but it recursively lists all of the files in the tree specified by the path. An alternative would be to use Files.walk. I didn't go this route because Files.walk is somewhat hard to reason about because the iterator can throw an exception at any time. This is a problem if the directory is being traversed and modified at the same time. With Files.walkFileTree, I know that if, for example, a directory disappears from out from underneath us, then we'll just move on without throwing an exception.
This test sometimes spuriously fails and I'm pretty sure it's because of concurrent modifications of lines.
While working on a related project, I realized that it's essential to handle the OVERFLOW case for a WatchEvent. The EventMonitor relies on subdirectory creation events to create a new watch key for each subdirectory. If those events are missed due to overflow, the EventMonitor will not be monitoring the newly created subdirectories. To fix this, when the EventMonitor detects an overflow, it will now poll the directory of the watch key repeatedly until it stabilizes. Once that happens, it will register all of the newly found directories (if any) with the watch service and will create events for all of the files in said directories. It is still possible that a trigger could be missed in the event that a lot of files in a watched directory cause an overflow before a valid source file is touched and before the EventMonitor handles the overflow. I am not particularly worried about this. I also discovered that the PollingWatchService was using an unbounded list of events. This seemed risky so I switched to using an ArrayBlockingQueue of size 256, which is what the MacOSXWatchService uses as well. Since the queue is now bounded, I added overflow events to the PollingWatchService as well. I also added synchronization for a few methods in PollingWatchey and MacOSXWatchKey. This is more consistent with the AbstractWatchKey in the jdk. The other methods return constant values so synchronization is pointless. I ran a number of travis builds against the content of this commit and none of them failed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I am optimistic that these changes will stop the spurious test failures that we've seen. I was working on top of #151 because it added some additional synchronization that I needed.