isSymlink could be much faster if using lstat directly

[pyrtsa]

Profiling what takes Xcode and SPM so long with some large repositories, the current isSymlink implementation stood out. (See link to Swift Forums post for details.)

I was able to cut time in a third (in the given test case) by replacing it with the lstat call apple/swift-corelibs-foundation makes as part of its FileManager.attributesOfItem(atPath:) implementation.

I have limited resources to take this work forward currently, cleaning up the implementation and adding Windows support, but leaving a PR here for comments so it's not lost.

[tomerd]

great idea, need to make sure it works on all platforms

cc @abertelrud @MaxDesiatov @compnerd

[tomerd]

@swift-ci test

[tomerd]

@parkera is this something that can be addressed in foundation?

[parkera]

Perhaps, depending on what platforms are involved here. We should look at the full trace and see what FileManager is doing here, and if there is a better way to get the info without reimplementing a chunk of it in SPM.

[neonichu]

@kperryua You had some ideas on how to do this using Foundation instead, could you elaborate more here?

[kperryua]

@kperryua You had some ideas on how to do this using Foundation instead, could you elaborate more here?

From the slack conversation:

try myURL.resourceValues(forKeys: [.isSymbolicLink])?.isSymbolicLink ?? false

[pyrtsa]

Unfortunately accessing resourceValues(forKeys: [.isSymbolicLinkKey]) appears to lead to the same slow FileManager._attributesOfItem(atPath:includingPrivateAttributes:) code path as what currently slows down SwiftPM.

[kperryua]

Can you spindump the difference for me? When I checked last week it was demonstrably faster than the attributesOfItem implementation that makes multiple syscalls.

[hassila]

Can you spindump the difference for me? When I checked last week it was demonstrably faster than the attributesOfItem implementation that makes multiple syscalls.

@kperryua not sure how to spin dump the diff for you, but I followed the instructions in the forum thread and it was trivial to reproduce. Running this (on Sonoma b2 this time) time swift package show-dependencies took ~7s on my machine after warmup (10s first run), attaching sample of swift-package too.
sample.txt

[kperryua]

I didn't realize this was dependent on swift-corelibs-foundation, which has a completely different implementation than Darwin's Foundation.framework.

The SCL-Foundation implementation of URL resource values is in fact still not as efficient as it could be compared to just perform a direct lstat. I think there's definitely an opportunity for improvement there, though in time it'll be replaced by a swift-foundation package implementation instead.

[neonichu]

I didn't realize this was dependent on swift-corelibs-foundation, which has a completely different implementation than Darwin's Foundation.framework.

That seems surprising to me as well tbh. @hassila were you using the toolchain bundled with Xcode or one downloaded from swift.org?

[hassila]

@neonichu I was testing with the Xcode 15b2 toolchain

[hassila]

I made another measurement to give more exact information on sample source, this is on macOS 13.4.1 with Xcode 15b2:

hassila@max ~/p/SlowSPM> uname -a
Darwin max.local 22.5.0 Darwin Kernel Version 22.5.0: Thu Jun  8 22:22:20 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T6000 arm64
hassila@max ~/p/SlowSPM> swift package --version
Swift Package Manager - Swift 5.9.0
hassila@max ~/p/SlowSPM> swift --version
swift-driver version: 1.82.2 Apple Swift version 5.9 (swiftlang-5.9.0.114.10 clang-1500.0.29.1)
Target: arm64-apple-macosx13.0
hassila@max ~/p/SlowSPM> xcode-select -p
/Applications/Xcode-beta.app/Contents/Developer

Run time when sampling:

________________________________________________________
Executed in   11.01 secs    fish           external
   usr time    7.34 secs    5.12 millis    7.34 secs
   sys time    3.31 secs    2.03 millis    3.31 secs

And sampling results:
sample_new.txt

[kperryua]

This process being sampled here doesn't appear to be using the URL resourceValue + .isSymlink API directly still. It's calling into NSFileAttributes code which does way more work than just the desired lstat.

    + !           :     |       +       ! : | 1044 ???  (in swift-package)  load address 0x102798000 + 0x51a04  [0x1027e9a04]
    + !           :     |       +       ! : | + 343 +[NSFileAttributes _attributesAtPath:partialReturn:filterResourceFork:error:]  (in Foundation) + 296  [0x18248694c]

[hassila]

Right, it's the out of the box tool chain from Xcode 15b2 as mentioned. I can try to build a version with the attached patch from the PR, but not sure what the point is - it's proven to help significantly? It seems this case is stuck between possible future foundation improvements vs. duplicating the code in this PR to get the performance issue addressed in a more timely manner?

[neonichu]

I think the idea is to use try myURL.resourceValues(forKeys: [.isSymbolicLink])?.isSymbolicLink ?? false instead of FileManager.attributesOfItem(atPath:).

[kperryua]

I think the idea is to use try myURL.resourceValues(forKeys: [.isSymbolicLink])?.isSymbolicLink ?? false instead of FileManager.attributesOfItem(atPath:).

Yep!

[neonichu]

I think this commit should do that, don't have time right now to run any kind of comparison, but if anyone else on this thread is able to, that would be much appreciated.

[pyrtsa]

@neonichu I don't know if it's different for the Apple platforms implementation for NSURL.resourceValues(forKeys:), but at least the open-source one in swift-corelibs-foundation still follows the slow code path of FileManager._attributesOfItem(atPath:includingPrivateAttributes:) when called with .isSymbolicLinkKey; see around here: https://github.com/apple/swift-corelibs-foundation/blob/main/Sources/Foundation/NSURL.swift#L1310-L1311.

If your suggested approach was used (rather than lstat directly), then it seems to me we should patch NSURL accordingly.

[hassila]

I think this commit should do that, don't have time right now to run any kind of comparison, but if anyone else on this thread is able to, that would be much appreciated.

I gave it a spin with SPM main with TSC main built in release mode (changed SPM Package.swift to use a local copy of TSC) with your patch applied and without. This is on macOS / M1 Ultra.

hassila@ice ~/s/SlowSPM> time /Users/hassila/github/swift-package-manager/.build/release/swift-package show-dependencies

Two runs without the patch gave:

Executed in   13.75 secs    fish           external
   usr time    6.54 secs    0.17 millis    6.54 secs
   sys time    2.45 secs    2.11 millis    2.45 secs
and
Executed in   13.32 secs    fish           external
   usr time    6.50 secs    0.13 millis    6.50 secs
   sys time    2.43 secs    1.12 millis    2.43 secs

with the patch applied:

Executed in   12.66 secs    fish           external
   usr time    5.85 secs    0.25 millis    5.85 secs
   sys time    2.09 secs    2.37 millis    2.09 secs
and
Executed in   12.31 secs    fish           external
   usr time    5.87 secs    0.28 millis    5.87 secs
   sys time    2.02 secs    2.33 millis    2.02 secs

As a reference, both are significantly slower than the proper Xcode release version that clocks in around 7 seconds for the same test, but it seems your patch gave a measurable improvement.

Sample with the patch applied:
sample.txt

[hassila]

And as a reference, with @pyrtsa original patch in this PR to get completely comparable numbers, I see:

hassila@ice ~/s/SlowSPM> time /Users/hassila/github/swift-package-manager/.build/release/swift-package show-dependencies
...
________________________________________________________
Executed in   11.67 secs    fish           external
   usr time    5.55 secs    0.31 millis    5.55 secs
   sys time    1.67 secs    2.42 millis    1.67 secs

and
________________________________________________________

Executed in   11.66 secs    fish           external
   usr time    5.57 secs    0.24 millis    5.57 secs
   sys time    1.64 secs    2.22 millis    1.63 secs

[neonichu]

Thank you so much for doing this @hassila! So it seems as if at least on macOS, doing what @kperryua suggested gives us the perf benefit without having to roll our own code here.

On other platforms, we won't benefit because of how corelibs-foundation is implemented (as both @pyrtsa and @kperryua pointed out), but I don't think that should keep us from doing this to at least improve macOS performance. @tomerd wdyt?

[tomerd]

sgtm

[neonichu]

I lost track of this one, going to land my patch next week.

[neonichu]

Closing in favor of #428