Avoid unnecessary calls to stat within NSURLDirectoryEnumerator

[bdash]

fts_read calls stat internally to determine the type of each file that is visited. If URL(fileURLWithPath:) is called with a path that doesn't end with a slash, it calls stat to determine whether the path represents a file or a directory. The FTSENT's fts_info field tells us whether the entry represents a file or a directory, so we can instead use URL(fileURLWithPath:isDirectory:) and avoid the second stat.

Additionally, the FTS_NOSTAT flag indicates to fts_read that the caller is not interested in the fts_statp field of the FTSENT structure, allowing fts_read to skip calling stat in some circumstances.

These two changes reduce the runtime of FileManager.enumerator(at:includingPropertiesForKeys:options:errorHandler:) by around 55%.

[spevans]

@swift-ci test

[algal]

This looks great. For anyone new to the issue, It should be easy to benchmark the difference, and see how Swift’s file enumeration compares to Python’s, with these two scripts: https://github.com/algal/get_files

What got us wondering about this was the finding that Python’s os.walk is faster than Foundation’s enumerate method.

[spevans]

@swift-ci test linux

[algal]

Since this PR concerns performance of Foundation's file enumeration routine NSURLDirectoryEnumerator, perhaps this is the best place to add a few more notes relevant to its performance -- in particular, why this enumerator performs worse than Python's os.walk.

One issue is what is addressed by this PR, that Foundation calls stat unnecessarily because it is calling URL(fileURLWithPath:) instead of URL(fileURLWithPath:isDirectory), rather than using the file-or-directory info that is already provided by fts_read.

However, a separate issue may be that Foundation is calling fts_read per file and per directory, instead of using fts_children, a single call which will return a linked list structure containing stat information for all the files in a directory. I would guess this is also contributing to overhead.

The Python implementation is not based on fts_read and friends. It's based on scandir, which also returns bulk information, and this Python PEP 471 provides detailed notes on how os.walk was designed for good performance, using scandir. It bottoms out in the C implementation of Python's posix module.

So one further refinement, if there's appetite for it, might be to refactor NSURLDirectoryEnumerator so it called fts_children() to minimize the number of system calls when traversing a directory with many files.

[spevans]

That's interesting info, I hadn't heard of fts_children or scandir before. I am still reading though the glibc code for both functions but one thing that could be an issue is that both functions allocate memory for the entire result set which doesn't help in keeping memory usage low.

After a re-reading of the fts_read manage it looks like we could add in the FTS_NOSTAT_TYPE to the fts_open call and it will avoid the stat call per file whilst still returning a flag indicating if it is a directory. I think that may be a quick fix whilst some better benchmarks are developed to compare the current code (with this flag) against scandir and fts_children.

[bdash]

I briefly looked at using FTS_NOSTAT_TYPE, but it's not supported on Linux.

It's worth noting that after this change FileManager.enumerator(at:includingPropertiesForKeys:options:errorHandler:) performs very similarly to Python's os.walk on Linux in my testing.

After this change, around 1/3 of the runtime is spent doing work that isn't related to walking the filesystem. URL(fileURLWithPath:isDirectory:) accounts for > 20% of the runtime, with a further 10% below FileManager.string(withFileSystemRepresentation:length:). You can see this breakdown in the flame graph at http://bdash.net.nz/files/get_files-perf.svg.

[spevans]

It looks like FTS_NOSTAT should also work as well.

[bdash]

FTS_NOSTAT leaves the fts_info field set to FTS_NSOK, so you cannot tell if the entry is a file or directory.

[spevans]

The directory flag FTS_D should still come from the dirent entry returned from readdir and I think the only information required is if it is a directory or file. Here's the appropriate comment from glibc's fts.c:

 * The real slowdown in walking the tree is the stat calls.  If FTS_NOSTAT is
 * set and it's a physical walk (so that symbolic links can't be directories),
 * we can do things quickly.  First, if it's a 4.4BSD file system, the type
 * of the file is in the directory entry.  Otherwise, we assume that the number
 * of subdirectories in a node is equal to the number of links to the parent.
 * The former skips all stat calls.  The latter skips stat calls in any leaf
 * directories and for any files after the subdirectories in the directory have
 * been found, cutting the stat calls by about 2/3.

[bdash]

Yeah, I'd misunderstood FTS_NOSTAT. It's not about entirely eliminating calls to stat, but just eliminating them when possible. The man page on my Mac says:

Note that because fts detects directory cycles and dangling symbolic links, stat(2) is always called for directories and is called for symbolic links when FTS_LOGICAL is set.

I've confirmed that directories are correctly reported via FTS_D when FTS_NOSTAT is passed. When combined with the original change to avoid the redundant stat calls, this reduces the runtime of my test case by a total of 55% (i.e., FTS_NOSTAT is only a 25% improvement by itself).

Should I go ahead and include the FTS_NOSTAT change in this PR?

[spevans]

Yeah it seems like an easy win if it passes all the tests - doesn't look likeftp_statp is used anywhere. I think you can also make the change in _removeItem which also uses fts_open.

[bdash]

Done. The relevant tests pass locally for me.

[millenomi]

@swift-ci please test

[spevans]

@swift-ci please test

[millenomi]

@swift-ci please test