search

apple / swift-foundation

The Foundation project
Apache License 2.0
2.29k stars 135 forks source link

Reduce number of read calls when not asking for progress reporting. #623

Closed parkera closed 1 month ago

parkera commented 1 month ago

Also, when asking for progress reporting, cap the number of updates at approximately 100 instead of fileSize/4k.

parkera commented 1 month ago

@swift-ci test

jmschonfeld commented 1 month ago

The linux test failure looks related:

Test Case 'DataIOTests.test_largeFile' started at 2024-05-20 18:29:52.910
/build/swift-foundation/Tests/FoundationEssentialsTests/DataIOTests.swift:215: error: DataIOTests.test_largeFile : XCTAssertEqual failed: ("2147549184") is not equal to ("2147479552") - 
Test Case 'DataIOTests.test_largeFile' failed (17.749 seconds)
parkera commented 1 month ago

@swift-ci test

parkera commented 1 month ago

Updated to provide a "read until EOF" option, true by default. On Darwin we were always reading the requested 0x7ffffff, but on Linux we were reading 0x7fff000, then failing to loop again. This mirrors the implementation from swift-corelibs-foundation.

itingliu commented 1 month ago

LGTM! Can we merge this?

parkera commented 1 month ago

Yah, I wanted @kperryua to take a second look if possible.

kperryua commented 1 month ago

Yah, I wanted @kperryua to take a second look if possible.

Took me a bit to grok what the readUntilEOF change is about. I take a little issue with the name, since it presumes that the length parameter is expected to be EOF of a regular file. That's only technically true in this particular case of Data reading, since we're first querying the length of the file on disk and then passing that parameter for length. In theory one could call this function with a length less than the full length of the file, if one has a limited buffer.

parkera commented 1 month ago

Yah, I wanted @kperryua to take a second look if possible.

Took me a bit to grok what the readUntilEOF change is about. I take a little issue with the name, since it presumes that the length parameter is expected to be EOF of a regular file. That's only technically true in this particular case of Data reading, since we're first querying the length of the file on disk and then passing that parameter for length. In theory one could call this function with a length less than the full length of the file, if one has a limited buffer.

Fair enough - it's more like readUntilLength? I lifted the name from the equivalent function in swift-corelibs-foundation, where it is called with both Data and FileHandle reads.

kperryua commented 1 month ago

it's more like readUntilLength

I like that better.