Use nio.Path instead of io.File to properly support PlainNioFile #3395

nicolasstucki · 2017-10-27T14:17:38Z

No description provided.

nicolasstucki · 2017-10-27T15:20:24Z

The CI should not have passed on 72fa00d.

allanrenucci · 2017-10-27T15:01:51Z

compiler/src/dotty/tools/io/AbstractFile.scala

@@ -94,7 +95,7 @@ abstract class AbstractFile extends Iterable[AbstractFile] {
  def path: String

  /** Returns the path of this abstract file in a canonical form. */
-  def canonicalPath: String = if (file == null) path else file.getCanonicalPath
+  def canonicalPath: String = if (jpath == null) path else jpath.toFile.getCanonicalPath


I think it is equivalent to jpath.normalize.toString

allanrenucci · 2017-10-27T15:04:43Z

compiler/src/dotty/tools/io/AbstractFile.scala

@@ -107,18 +108,26 @@ abstract class AbstractFile extends Iterable[AbstractFile] {
  def container : AbstractFile

  /** Returns the underlying File if any and null otherwise. */
-  def file: JFile
+  def file: JFile = try {


Ideally we should remove this

That will be for next step. That might need changes outside of the Path/File/Directory abstractions. I would prefer to keep the changes only inside these abstractions for this first step.

allanrenucci · 2017-10-27T15:14:12Z

compiler/src/dotty/tools/io/AbstractFile.scala

+      val path = jpath.resolve(name)
+      try {
+        if (isDir) Files.createDirectories(path)
+        else Files.createFile(path)


This does not create intermediate directories if necessary. We need to make sure it behaves as before

allanrenucci · 2017-10-27T15:14:56Z

compiler/src/dotty/tools/io/AbstractFile.scala

+        if (isDir) Files.createDirectories(path)
+        else Files.createFile(path)
+      } catch {
+        case _: FileAlreadyExistsException =>


I guess it should never happen, otherwise lookupName would not have returned null

allanrenucci · 2017-10-27T15:16:30Z

compiler/src/dotty/tools/io/File.scala

  override val creationCodec = constructorCodec

  override def addExtension(ext: String): File = super.addExtension(ext).toFile
  override def toAbsolute: File = if (isAbsolute) this else super.toAbsolute.toFile
-  override def toDirectory: Directory = new Directory(jfile)
+  override def toDirectory: Directory = new Directory(jfile.toPath)


Why not simply new Directory(jpath)?

allanrenucci · 2017-10-27T15:21:25Z

compiler/src/dotty/tools/io/Path.scala

        case null =>
          if (isAbsolute) toDirectory // it should be a root. BTW, don't need to worry about relative pathed root
          else Directory(".")         // a dir under pwd
        case x    =>
-          Directory(x)
+          Directory(x.toFile)


There should probably be a constructor that takes a JPath. And I think we should remove the JFile constructor

allanrenucci · 2017-10-27T15:39:03Z

compiler/src/dotty/tools/io/Path.scala

-    }
-    f.delete()
+  def deleteRecursively(): Boolean = deleteRecursively(jpath)
+  private def deleteRecursively(p: JPath): Boolean = {


I think the proper way to delete recursively with nio is:

def deleteRecursively(dir: Path): Unit = if (Files.exists(dir)) { Files.walkFileTree(dir, new SimpleFileVisitor[Path]() { override def visitFile(file: Path, attrs: BasicFileAttributes) = { Files.delete(file) FileVisitResult.CONTINUE } override def postVisitDirectory(dir: Path, exc: IOException) = { Files.delete(dir) FileVisitResult.CONTINUE } })

Not sure what should be the behavior if the path does not exist

Same comment as before:

def deleteRecursively(dir: Path): Unit = if (Files.exists(dir)) { Files.walkFileTree(dir, new SimpleFileVisitor[Path]() { override def visitFile(file: Path, attrs: BasicFileAttributes) = { Files.delete(file) FileVisitResult.CONTINUE } override def postVisitDirectory(dir: Path, exc: IOException) = { Files.delete(dir) FileVisitResult.CONTINUE } })

allanrenucci · 2017-10-27T15:44:34Z

compiler/src/dotty/tools/io/Path.scala

  }

+  private def tryCreate(create: => JPath): Boolean =
+    try { create; true } catch { case _: FileAlreadyExistsException => false }


Semantic is different for something like foo/bar/ if foo does not exist

As we saw together the semantics are the same. In the example, an IOException would be thrown in both implementations.

allanrenucci · 2017-10-27T15:53:54Z

compiler/src/dotty/tools/io/Path.scala

  }

+  private def delete(path: JPath): Boolean =
+    try { Files.delete(path); true } catch { case _: DirectoryNotEmptyException | _: NoSuchFileException => false }


You can use deleteIfExists and not catch NoSuchFileException

allanrenucci · 2017-10-27T15:56:46Z

compiler/src/dotty/tools/io/ZipArchive.scala

@@ -112,7 +112,7 @@ abstract class ZipArchive(override val file: JFile) extends AbstractFile with Eq
  }
 }
 /** ''Note:  This library is considered experimental and should not be used unless you know what you are doing.'' */
-final class FileZipArchive(file: JFile) extends ZipArchive(file) {
+final class FileZipArchive(file: JFile) extends ZipArchive(file.toPath) {


Can't you use a JPath here instead?

allanrenucci · 2017-10-27T16:19:26Z

.drone.yml

@@ -13,6 +13,7 @@ pipeline:
    image: lampepfl/dotty:2017-10-20
    commands:
      - cp -R . /tmp/1/ && cd /tmp/1/
+      - ./project/scripts/sbt compile # to ensure that side projects (such as dotty doc) compile
      - ./project/scripts/sbt testAll
      - ./project/scripts/sbt ";dotty-bench/jmh:run 1 1 tests/run/arrays.scala"


This was meant to make sure that dotty-bench compiles. Should it be part of dottyRoot so that we can get rid of this line?

That would be a solution

allanrenucci · 2017-10-27T16:50:24Z

In PlainFile, I believe the following methods can be rewritten using nio APIs:

def input
def output
def iterator

allanrenucci · 2017-10-27T16:59:14Z

PlainNioFile should not exist anymore since PlainFile is now backed by nio. We should make sure, the methods in PlainFile are implemented as the ones in PlainNioFile.

allanrenucci · 2017-10-27T17:07:22Z

doc-tool/src/dotty/tools/dottydoc/staticsite/Site.scala

@@ -409,7 +409,7 @@ case class Site(
  }

  private def toSourceFile(f: JFile): SourceFile =


Should probably be changed to take a JPath instead

This will be for another PR.

smarter · 2017-10-27T18:09:50Z

I don't understand what's going on in this PR. What's the motivation?

nicolasstucki · 2017-11-02T09:28:10Z

test performance please

dottybot · 2017-11-02T09:28:44Z

performance test scheduled: 2 job(s) in queue, 1 running.

smarter · 2017-11-02T10:04:44Z

I still would like to know the rationale behind this PR :). Also it's likely incomplete, ExtractDependencies#recordDependency needs an extra case to handle PlainNioFiles: https://github.com/dotty-staging/dotty/blob/813d376d78ccf752b480882fbcb58cd6a35431a4/compiler/src/dotty/tools/dotc/sbt/ExtractDependencies.scala#L98, otherwise incremental compilation will be broken

allanrenucci · 2017-11-02T10:37:47Z

@smarter I believe writing to a jar required using the java.nio API but the dotty.io abstraction is mainly based on java.io. So it required some glue code to make it work.
The idea of this PR is to constantly use the java.nio everywhere and avoid conversions between JFile and JPath. Also, java.nio is supposed to be faster

nicolasstucki · 2017-11-02T11:00:04Z

ExtractDependencies#recordDependency won't have any problems, I removed PlainNioFile.

nicolasstucki · 2017-11-02T11:00:37Z

test performance please

dottybot · 2017-11-02T11:00:48Z

performance test scheduled: 2 job(s) in queue, 1 running.

dottybot · 2017-11-02T11:15:10Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3395/ to see the changes.

Benchmarks is based on merging with master (cbb481c)

dottybot · 2017-11-02T12:42:42Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3395/ to see the changes.

Benchmarks is based on merging with master (cbb481c)

smarter · 2017-11-02T17:26:31Z

@retronym is there any reason scalac still has PlainFile and PlainNioFile or would the kind of refactoring done here make sense in scalac too?

retronym · 2017-11-02T21:58:46Z

I added PlainNioFile as a minimal bridge from the AbtractFile to nio, as a means to support JDK’s jrt:// filesystem. I think AbstractFile should probably be removed from compiler APIs entirely in favour of direct use NIO. A bit of experimentation is needed to make sure we can still support in memory use cases (via a custom in memory filesystemprovider)

allanrenucci

Still methods in File are implemented in term of jfile

allanrenucci · 2017-11-03T09:50:46Z

compiler/src/dotty/tools/io/Directory.scala

-    jfile.listFiles match {
-      case null   => Iterator.empty
-      case xs     => xs.iterator map Path.apply
+  def list: Iterator[Path] = {


It should be lazy

if (isDirectory) Files.list(jpath).iterator.asScala.map(Path.apply) else Iterator.empty

allanrenucci · 2017-11-03T09:58:28Z

compiler/src/dotty/tools/io/Path.scala


  def onlyDirs(xs: Iterator[Path]): Iterator[Directory] = xs filter (_.isDirectory) map (_.toDirectory)
  def onlyDirs(xs: List[Path]): List[Directory] = xs filter (_.isDirectory) map (_.toDirectory)
  def onlyFiles(xs: Iterator[Path]): Iterator[File] = xs filter (_.isFile) map (_.toFile)

-  def roots: List[Path] = java.io.File.listRoots().toList map Path.apply
+  def roots: List[Path] = java.io.File.listRoots().toList.map(r => Path.apply(r.toPath))


FileSystems.getDefault().getRootDirectories()?

allanrenucci · 2017-11-03T10:03:22Z

compiler/src/dotty/tools/io/Path.scala

  }
  def createFile(failIfExists: Boolean = false): File = {
-    val res = jfile.createNewFile()
+    val res = tryCreate(Files.createFile(jpath))
+    jfile.createNewFile()


Should this be here?

allanrenucci · 2017-11-03T10:03:40Z

compiler/src/dotty/tools/io/Path.scala

-    }
-    f.delete()
+  def deleteRecursively(): Boolean = deleteRecursively(jpath)
+  private def deleteRecursively(p: JPath): Boolean = {


Same comment as before:

def deleteRecursively(dir: Path): Unit = if (Files.exists(dir)) { Files.walkFileTree(dir, new SimpleFileVisitor[Path]() { override def visitFile(file: Path, attrs: BasicFileAttributes) = { Files.delete(file) FileVisitResult.CONTINUE } override def postVisitDirectory(dir: Path, exc: IOException) = { Files.delete(dir) FileVisitResult.CONTINUE } })

allanrenucci · 2017-11-03T10:07:31Z

compiler/src/dotty/tools/io/ZipArchive.scala

@@ -32,7 +33,7 @@ object ZipArchive {
   */
  def fromFile(file: File): FileZipArchive = fromFile(file.jfile)


I would add fromPath and make the other delegate to it

nicolasstucki · 2017-11-03T14:19:50Z

test performance please

dottybot · 2017-11-03T14:20:09Z

performance test scheduled: 2 job(s) in queue, 1 running.

allanrenucci · 2017-11-03T16:45:10Z

compiler/src/dotty/tools/io/File.scala


  /** Obtains a OutputStream. */
-  def outputStream(append: Boolean = false) = new FileOutputStream(jfile, append)
+  def outputStream(append: Boolean = false) = Files.newOutputStream(jpath, APPEND)


allanrenucci · 2017-11-03T16:50:26Z

compiler/src/dotty/tools/io/ZipArchive.scala

-    try   { new FileZipArchive(file) }
+  def fromFile(file: File): FileZipArchive = fromPath(file.jpath)
+  def fromPath(jpath: JPath): FileZipArchive =
+    try   { new FileZipArchive(jpath) }
    catch { case _: IOException => null }


Not sure what can throw an IOException here

dottybot · 2017-11-03T17:01:33Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3395/ to see the changes.

Benchmarks is based on merging with master (22ff8a4)

allanrenucci · 2017-11-03T17:06:27Z

Also, I think you can implement resolve, relativize, and parent in Path in term of nio

allanrenucci · 2017-11-06T13:14:47Z

test performance please

dottybot · 2017-11-06T13:14:50Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2017-11-06T14:41:37Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3395/ to see the changes.

Benchmarks is based on merging with master (22ff8a4)

nicolasstucki · 2017-11-06T14:47:33Z

test performance please

dottybot · 2017-11-06T14:47:53Z

performance test scheduled: 1 job(s) in queue, 1 running.

dottybot · 2017-11-06T18:59:22Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3395/ to see the changes.

Benchmarks is based on merging with master (45387e2)

nicolasstucki · 2017-11-06T19:03:09Z

test performance please

dottybot · 2017-11-06T19:03:47Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2017-11-06T20:31:53Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3395/ to see the changes.

Benchmarks is based on merging with master (45387e2)

nicolasstucki requested a review from allanrenucci October 27, 2017 14:17

nicolasstucki assigned allanrenucci Oct 27, 2017

nicolasstucki added the stat:needs review label Oct 27, 2017

nicolasstucki mentioned this pull request Oct 27, 2017

Support -d with .jar paths #3382

Merged

allanrenucci suggested changes Oct 27, 2017

View reviewed changes

allanrenucci reviewed Oct 27, 2017

View reviewed changes

allanrenucci removed the stat:needs review label Oct 29, 2017

nicolasstucki force-pushed the update-to-nio-paths branch from e2222dd to a914812 Compare November 2, 2017 09:27

nicolasstucki added the stat:needs review label Nov 2, 2017

nicolasstucki force-pushed the update-to-nio-paths branch from a914812 to 813d376 Compare November 2, 2017 09:47

nicolasstucki force-pushed the update-to-nio-paths branch from 813d376 to 2694273 Compare November 2, 2017 10:34

allanrenucci suggested changes Nov 3, 2017

View reviewed changes

nicolasstucki force-pushed the update-to-nio-paths branch from bfc9784 to 3108499 Compare November 3, 2017 14:17

allanrenucci reviewed Nov 3, 2017

View reviewed changes

nicolasstucki added 11 commits November 5, 2017 10:53

Use nio.Path instead of io.File to properly support PlainNioFile

32b0a5b

Fix creation and deletion methods

aeec429

Fix nio.Path in dotty doc and ensure that its compilation is tested

aced7e0

Fixes and more translations

7ff1cb6

Remove PlainNioFile as it PlainFile is on nio now

92a25cb

Rename java.io.File to JFile for consistency

3c1c655

Remove dotty.tools.io.Path.jfile

1eb1270

Improve some implementations based on java Paths

757fb6a

Fix outputStream append flag

88aa338

Remove unneeded exception handling

672f557

Implement resolve and relativize with nio

5065cf2

nicolasstucki force-pushed the update-to-nio-paths branch from 5dea2a7 to 5065cf2 Compare November 5, 2017 10:02

allanrenucci approved these changes Nov 6, 2017

View reviewed changes

nicolasstucki merged commit 42577bb into scala:master Nov 7, 2017

Duhemm mentioned this pull request Nov 9, 2017

Too many open files when running dotty.tools.dotc.IdempotencyTest.idempotency. #3450

Closed

allanrenucci deleted the update-to-nio-paths branch December 14, 2017 19:25

		@@ -409,7 +409,7 @@ case class Site(
		}

		private def toSourceFile(f: JFile): SourceFile =

		@@ -32,7 +33,7 @@ object ZipArchive {
		*/
		def fromFile(file: File): FileZipArchive = fromFile(file.jfile)

Use nio.Path instead of io.File to properly support PlainNioFile #3395

Use nio.Path instead of io.File to properly support PlainNioFile #3395

Conversation

nicolasstucki commented Oct 27, 2017

nicolasstucki commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allanrenucci Oct 27, 2017 • edited Loading

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

allanrenucci Nov 3, 2017 • edited Loading

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

allanrenucci commented Oct 27, 2017 • edited Loading

allanrenucci commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarter commented Oct 27, 2017

nicolasstucki commented Nov 2, 2017

dottybot commented Nov 2, 2017

smarter commented Nov 2, 2017 • edited Loading

allanrenucci commented Nov 2, 2017

nicolasstucki commented Nov 2, 2017

nicolasstucki commented Nov 2, 2017

dottybot commented Nov 2, 2017

dottybot commented Nov 2, 2017

dottybot commented Nov 2, 2017

smarter commented Nov 2, 2017

retronym commented Nov 2, 2017 via email

allanrenucci left a 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

allanrenucci Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasstucki commented Nov 3, 2017

dottybot commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dottybot commented Nov 3, 2017

allanrenucci commented Nov 3, 2017

allanrenucci commented Nov 6, 2017

dottybot commented Nov 6, 2017

dottybot commented Nov 6, 2017

nicolasstucki commented Nov 6, 2017

dottybot commented Nov 6, 2017

dottybot commented Nov 6, 2017

nicolasstucki commented Nov 6, 2017

dottybot commented Nov 6, 2017

dottybot commented Nov 6, 2017

allanrenucci Oct 27, 2017 •

edited

Loading

allanrenucci Nov 3, 2017 •

edited

Loading

allanrenucci commented Oct 27, 2017 •

edited

Loading

smarter commented Nov 2, 2017 •

edited

Loading

allanrenucci Nov 3, 2017 •

edited

Loading