New `WatchService` based on Java 7's `WatchService` #47

Duhemm · 2017-06-20T19:30:57Z

The goal of this is to add an abstraction layer so that we can define
new kinds of WatchServices. This way, we can use the default
WatchService provided by the JDK on certain platform (e.g. Windows),
and revert back to the PollingWatchService that existed before, on
machine where there's no native support for monitoring the file system
(e.g. macOS).

This commit includes an adapter for the WatchService from Java NIO to
our new interface, and a port of the existing PollingWatchService.

This commit shows how it integrates within sbt/sbt: Duhemm/sbt@f5060f2

Duhemm · 2017-06-23T14:08:56Z

I've been doing some benchmarking, and the results with nio and BarbaryWatchService are really good. I need to improve the PollingWatchService (the re-implementation of what's currently there) though.

Here are the results of the benchmarks: https://travis-ci.org/Duhemm/io/builds/246090849

sun.nio.fs.LinuxWatchService@7d376ecf
-------------------------------------
 - minMs      = 17.0
 - maxMs      = 134.0
 - avgMs      = 32.210000000000015
 - medianMs   = 25.0
 - stddevMs   = 21.428623380889405
 - p95Ms      = 59.0
 - p05Ms      = 20.0
 - avg        = 27.96774193548388

sbt.io.PollingWatchService@5936325f
-----------------------------------
 - minMs      = 233.0
 - maxMs      = 943.0
 - avgMs      = 380.7899999999998
 - medianMs   = 368.0
 - stddevMs   = 84.54256856755657
 - p95Ms      = 493.0
 - p05Ms      = 256.0
 - avg        = 374.6043956043956

old sbt file watch
------------------
 - minMs      = 134.0
 - maxMs      = 212.0
 - avgMs      = 145.48
 - medianMs   = 141.0
 - stddevMs   = 11.597827382747171
 - p95Ms      = 165.0
 - p05Ms      = 135.0
 - avg        = 143.84042553191483

The goal of this is to add an abstraction layer so that we can define new kinds of `WatchService`s. This way, we can use the default `WatchService` provided by the JDK on certain platform (e.g. Windows), and revert back to the `PollingWatchService` that existed before, on machine where there's no native support for monitoring the file system (e.g. macOS). This commit includes an adapter for the `WatchService` from Java NIO to our new interface, and a port of the existing `PollingWatchService`.

Duhemm · 2017-06-28T17:22:14Z

Benchmarking results on Windows: https://ci.appveyor.com/project/Duhemm/io/build/12

eed3si9n

Minor comments about duration, but overall looks good. Could you also prepare a corresponding PR to sbt/sbt plz?

eed3si9n · 2017-06-29T02:52:52Z

io/src/main/scala/sbt/io/PollingWatchService.scala

+import scala.collection.mutable
+
+/** A `WatchService` that polls the filesystem every `delayMs` milliseconds. */
+class PollingWatchService(delayMs: Long) extends WatchService {


Let's use Duration.

eed3si9n · 2017-06-29T02:53:35Z

io/src/main/scala/sbt/io/WatchService.scala

+   * @param timeoutMs Maximum time to wait, in milliseconds.
+   * @return The next `WatchKey` that received an event.
+   */
+  def poll(timeoutMs: Long): WatchKey


Let's use Duration.

Duhemm · 2017-06-29T07:47:13Z

@eed3si9n Done! Corresponding sbt/sbt PR: sbt/sbt#3295

typesafe-tools · 2017-06-29T20:09:12Z

dbuild has checked the following projects against Scala 2.12:

Project	Reference	Commit
sbt	pull/3295/head	sbt/sbt@`0774a24`
zinc	pull/323/head	sbt/zinc@`6a6965b`
io	pull/47/head	`75afc79`
librarymanagement	1.0	sbt/librarymanagement@`7e87603`
util	1.0	sbt/util@`54cf4a4`
website	1.0.x	sbt/website@`5c32a0a`

❌ The result is: FAILED
(restart)

fommil · 2017-08-03T13:45:40Z

io/src/test/scala/sbt/internal/io/SourceModificationWatchSpec.scala

+
+import scala.concurrent.duration._
+
+abstract class SourceModificationWatchSpec(getService: => WatchService, pollDelay: FiniteDuration, maxWait: FiniteDuration) extends FlatSpec with Matchers {


is this tested on all three major platforms? Because the semantics are very different and there are OS limitations to fidelity of changes (e.g. the Linux kernel only records FS accuracy to second precision on most filesystems)

fommil · 2017-08-03T13:46:37Z

io/src/main/scala/sbt/internal/io/SourceModificationWatch.scala

+        if (terminationCondition) {
+          (false, state)
+        } else {
+          Thread.sleep(delay.toMillis)


fommil · 2017-08-03T13:46:45Z

io/src/main/scala/sbt/internal/io/SourceModificationWatch.scala

+        if (filteredCreated.nonEmpty || filteredDeleted.nonEmpty || filteredModified.nonEmpty) {
+          (true, newState.withCount(newState.count + 1))
+        } else {
+          Thread.sleep(delay.toMillis)


fommil · 2017-08-03T13:54:33Z

I'm a little confused about the implementation of this. It doesn't seem to use NIO as much as it could, and therefore will be very limited in its performance gains. e.g. I couldn't find any use of the walk pattern https://docs.oracle.com/javase/tutorial/essential/io/walk.html which is orders of magnitude faster on Windows.

Is this only tested in travis? You really need to test in in appveyor to see Windows behaviour and even OS X / Linux kernels make a big difference when it comes to IO.

We cracked almost all of these problems in ensime already with https://github.com/ensime/java7-file-watcher and (GPLv3 but we'd be willing to talk about it) https://github.com/ensime/ensime-server/blob/2.0/core/src/main/scala/org/ensime/indexer/FileWatchers.scala https://github.com/ensime/ensime-server/blob/2.0/core/src/main/scala/org/ensime/filewatcher/FileWatchService.scala (it's not pretty, I'd rather use an fs2 Source, but it is pretty rock solid). How come you didn't want to use that as the basis for this? We could have created a shared lib that both of us could have used. Since the maintenance burden is quite high for this I/O stuff, we would have appreciated the extra help.

fommil · 2017-08-03T13:56:47Z

BTW, when we swapped to NIO in ensime (and even some of my sbt hacks to use nio for jar packing) I was seeing x1000 improvements on large projects in windows. Actually sometimes it was the difference between "working" and "using all CPUs and not working at all, not even nearly". I believe this PR only unlocks a very small percentage of the potential gains.

Duhemm · 2017-08-03T14:07:21Z

I'm a little confused about the implementation of this. It doesn't seem to use NIO as much as it could, and therefore will be very limited in its performance gains. e.g. I couldn't find any use of the walk pattern https://docs.oracle.com/javase/tutorial/essential/io/walk.html which is orders of magnitude faster on Windows.

The goal was to use java.nio.file.WatchService. I think that you're confused about the PollingWatchService. This implementation is just an adaptation of the previous watch service that was used in sbt 0.13. It's only there for platforms that do not have a good implementation of java.nio.file.WatchService, like MacOS for instance. On other platform, this should not be used. For instance on Linux, you may get an implementation of WatchService that relies on inotify.

This PR also puts everything in place to let users write new WatchService implementations as sbt plugin. For instance, someone could write a plugin that would provide a WatchService based on fsevents.

Is this only tested in travis? You really need to test in in appveyor to see Windows behaviour and even OS X / Linux kernels make a big difference when it comes to IO.

This has been tested locally and on Travis with MacOS and Linux, and on AppVeyor on Windows.

fommil · 2017-08-03T14:08:59Z

We've had all these discussions before in ensime (especially re OS X), I really wish you'd reached out because it would have saved you a tonne of time doing your research.

"has been tested" != "is tested" (but == "has rotted"). Can sbt get an appveyor subscription and include this as part of CI?

Duhemm changed the title ~~New WatchService based on Java 7's WatchService~~ [On hold] New WatchService based on Java 7's WatchService Jun 23, 2017

Duhemm force-pushed the topic/watchservice branch 3 times, most recently from 199740c to d2a6570 Compare June 28, 2017 16:59

Duhemm force-pushed the topic/watchservice branch from d2a6570 to 3f7488b Compare June 28, 2017 17:06

Duhemm changed the title ~~[On hold] New WatchService based on Java 7's WatchService~~ New WatchService based on Java 7's WatchService Jun 28, 2017

Duhemm requested review from eed3si9n and dwijnand June 28, 2017 17:22

eed3si9n requested changes Jun 29, 2017

View reviewed changes

Use Duration

75afc79

Duhemm mentioned this pull request Jun 29, 2017

Adapt to use the new WatchService sbt/sbt#3295

Closed

eed3si9n approved these changes Jun 29, 2017

View reviewed changes

eed3si9n merged commit 177ba25 into sbt:1.0 Jun 29, 2017

fommil reviewed Aug 3, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New `WatchService` based on Java 7's `WatchService` #47

New `WatchService` based on Java 7's `WatchService` #47

Duhemm commented Jun 20, 2017

Duhemm commented Jun 23, 2017

Duhemm commented Jun 28, 2017

eed3si9n left a comment

eed3si9n Jun 29, 2017

eed3si9n Jun 29, 2017

Duhemm commented Jun 29, 2017

typesafe-tools commented Jun 29, 2017

fommil Aug 3, 2017

fommil Aug 3, 2017

fommil Aug 3, 2017

fommil commented Aug 3, 2017

fommil commented Aug 3, 2017

Duhemm commented Aug 3, 2017

fommil commented Aug 3, 2017


		import scala.concurrent.duration._

		abstract class SourceModificationWatchSpec(getService: => WatchService, pollDelay: FiniteDuration, maxWait: FiniteDuration) extends FlatSpec with Matchers {

New WatchService based on Java 7's WatchService #47

New WatchService based on Java 7's WatchService #47

Conversation

Duhemm commented Jun 20, 2017

Duhemm commented Jun 23, 2017

Duhemm commented Jun 28, 2017

eed3si9n left a comment

Choose a reason for hiding this comment

eed3si9n Jun 29, 2017

Choose a reason for hiding this comment

eed3si9n Jun 29, 2017

Choose a reason for hiding this comment

Duhemm commented Jun 29, 2017

typesafe-tools commented Jun 29, 2017

fommil Aug 3, 2017

Choose a reason for hiding this comment

fommil Aug 3, 2017

Choose a reason for hiding this comment

fommil Aug 3, 2017

Choose a reason for hiding this comment

fommil commented Aug 3, 2017

fommil commented Aug 3, 2017

Duhemm commented Aug 3, 2017

fommil commented Aug 3, 2017

New `WatchService` based on Java 7's `WatchService` #47

New `WatchService` based on Java 7's `WatchService` #47