[SR-9979] FileHandle class used to implement FileManager._compareFiles #2029

karthikkeyan · 2019-03-21T18:48:55Z

FileHandler APIs used to replace the curret implementation of the private method
_compareFiles(withFileSystemRepresentation:andFileSystemRepresentation:size:bufSize:)

Bug Link: https://bugs.swift.org/browse/SR-9979

compnerd · 2019-03-21T21:12:46Z

@swift-ci please test

spevans · 2019-03-22T09:00:16Z

Thanks for this, I just have a few comments:

Since the filenames are already in FSR (File System Representation), converting them back to a String is unnecessary, especially since that String will then be converted back to an FSR to use in the open call to actually open the file.

I would suggest adding a new init method to FileHandle along the lines of internal init?(fileSystemRepresentation: UnsafePointer<Int8>, flags: Int32, createMode: Int) which takes an FSR instead of a String and defaults to _closeOnDealloc = true.

For the read loop, FileHandle.readData(ofLength:) is actually quite heavy since it will involve calling fstat() and a malloc()/free() for every call in the loop.

I would add another FileHandle method, something like:
internal func _readBytes(into: UnsafePointer<UInt8>, count: Int) throws -> Int which just wraps the OS read system call and also refactor the inner read loop of _readDataOfLength to use it.

This could make the _compareFiles look something like:

guard let file1 = FileHandle(fileSystemRepresentation: file1Rep, ...) else { return false }
guard let file2 = FileHandle(fileSystemRepresentation: file2Rep, ...) else { return false }

let buffer1 = UnsafeMutablePointer<UInt8>.allocate(capacity: bufSize)
let buffer2 = UnsafeMutablePointer<UInt8>.allocate(capacity: bufSize)
defer {
    buffer1.deallocate()
    buffer2.deallocate()
}


var bytesLeft = size
while bytesLeft > 0 {
    let bytesToRead = Int(min(Int64(bufSize), bytesLeft))
    guard let bytesRead = try? file1._readBytes(into: buffer1, count: bytesToRead),
        bytesRead == bytesToRead else {
	return false
    }
    guard let bytesRead = try? file2._readBytes(into: buffer2, count: bytesToRead),
        bytesRead == bytesToRead else {
	return false
    }
    guard memcmp(buffer1, buffer2, bytesToRead) == 0 else {
	return false
    }
    bytesLeft -= Int64(bytesToRead)
}
    
return true

karthikkeyan · 2019-03-22T19:18:33Z

@spevans Thanks for reviewing my PR Simon. I have updated the code as per your suggestion. Now it is ready for the next round of review.

Foundation/FileHandle.swift

compnerd · 2019-03-22T20:22:12Z

Foundation/FileHandle.swift

@@ -425,6 +433,15 @@ open class FileHandle : NSObject, NSSecureCoding {
        }
    }
 #endif
+
+    internal init?(fileSystemRepresentation: UnsafePointer<Int8>, flags: Int32, createMode: Int) {


I don't think that we should do this. The problem is that on Windows, you need to use UTF16 (or Unicode) for the filenames for the FSR otherwise you will not be able to open all files.

The methods _compareFiles() and contentEquals() does not support Windows platform, yet. We will cross the bridge when we get to it. As for as this PR concern it seems to me, it is safe to wrap this init method under #if !os(Windows). What do you think @compnerd?

The SR was filed specifically to enable Windows support, so I think that it should be part of this change. Using the FileHandle was the way around the FSR conversions.

Sounds good. I will update the code to support Windows as well.

@compnerd Thanks for the suggestions. I have updated the init method. Now it supports windows as well. Please do a round of review when you get a chance.

Maybe a better solution would be to add an internal typealias for FSR

#if os(windows) internal typealias _FileSystemRepresentation = UnsafePointer<UInt16> #else internal typealias _FileSystemRepresentation = UnsafePointer<Int8> #endif

And use that in the signatures for these internal functions. I guess FileManager.fileSystemRepresentation(withPath path: String) -> UnsafePointer<Int8> is technically broken on Windows anyway but can probably be ignored for now.

Hmm, we don't want to break the public interfaces, I think that we should convert across the encodings.

compnerd · 2019-03-22T20:22:25Z

Foundation/FileManager.swift

        defer {
            buffer1.deallocate()
            buffer2.deallocate()
        }
-
+        


Bleeding whitespace

The space between defer and the next likes seems necessary, but If you feel strongly about this I will update it.

Also while we are in the topic, I have noticed a couple more unnecessary whitespaces as well in the same method; I will remove them too.

Having an empty line is not an issue, its the trailing whitespace which is the problem (the large green rectangle).

compnerd · 2019-03-23T05:21:51Z

Foundation/FileHandle.swift

+        guard fd > 0 else { return nil }
+
+#if os(Windows)
+        self.init(handle: HANDLE(bitPattern: _get_osfhandle(fd))!, closeOnDealloc: true)


Why can't you use the init(fileDescriptor:closeOnDealloc:)?

I did not notice that init method; I will update the code. Thank you.

Foundation/FileHandle.swift

…a() in order to avoid calling fstat, alloc and dealloc while reading a file * New internal init method introduced to init with fileSystemRepresentation UnsafeMutablePointer<Int8> * New internal method _readBytes() introduced for continuous iteration of reading a file without allocating and deallocating buffer memory.

…leSystemRepresentation:flags:createMode:).

…nto:lenght) in FileHandle

karthikkeyan · 2019-03-25T18:29:11Z

Thank you @spevans @compnerd, I think I have addressed all the comments, except the new typealias _FileSystemRepresentation which I could not quite figure out whats the expectation. Please give it another round of review when you get a chance.

Foundation/FileHandle.swift

…ndows platform.

compnerd · 2019-03-26T00:44:46Z

@swift-ci please test

karthikkeyan · 2019-03-26T02:56:21Z

@compnerd Looks like the compilation failed. I have built and ran the test cases using Xcode-Beta 10.2 beta 4 (10P107d) using Swift Development Snapshot 2019-03-17 (a) snapshot, successfully. Not sure if it is CI setup issue.

spevans · 2019-03-26T07:46:40Z

@swift-ci please test

spevans · 2019-03-26T07:48:06Z

@karthikkeyan Don't worry too much about my typealias _FileSystemRepresentation idea, it was just a vague notation, I may try prototyping it in a separate PR to see if its any benefit but its not a blocker for this PR.

compnerd requested changes Mar 22, 2019

View reviewed changes

compnerd reviewed Mar 23, 2019

View reviewed changes

karthikkeyan added 3 commits March 25, 2019 11:16

[SR-9979] FileHandle class used to implement FileManager._compareFiles

e1b3d7b

Windows platform compatibility added for the internal method init?(fi…

690d266

…leSystemRepresentation:flags:createMode:).

karthikkeyan force-pushed the karthik/SR-9979-comaprefile-filehandle branch from adf41ce to 3e6884b Compare March 25, 2019 18:20

Windows platform compatibility added for internal method _readBytes(i…

5e020b8

…nto:lenght) in FileHandle

karthikkeyan force-pushed the karthik/SR-9979-comaprefile-filehandle branch from 3e6884b to 5e020b8 Compare March 25, 2019 18:22

compnerd reviewed Mar 25, 2019

View reviewed changes

Foundation/FileHandle.swift Show resolved Hide resolved

FileSystemRepresentation converted to UTF16 before opening file in Wi…

e33eb72

…ndows platform.

compnerd approved these changes Mar 26, 2019

View reviewed changes

compnerd merged commit 80bbe98 into swiftlang:master Mar 26, 2019

[SR-9979] FileHandle class used to implement FileManager._compareFiles #2029

[SR-9979] FileHandle class used to implement FileManager._compareFiles #2029

Uh oh!

Conversation

karthikkeyan commented Mar 21, 2019

Uh oh!

compnerd commented Mar 21, 2019

Uh oh!

spevans commented Mar 22, 2019

Uh oh!

karthikkeyan commented Mar 22, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karthikkeyan Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

karthikkeyan commented Mar 25, 2019

Uh oh!

Uh oh!

compnerd commented Mar 26, 2019

Uh oh!

karthikkeyan commented Mar 26, 2019

Uh oh!

spevans commented Mar 26, 2019

Uh oh!

spevans commented Mar 26, 2019

Uh oh!

Uh oh!

karthikkeyan Mar 22, 2019 •

edited

Loading