Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Unix FileStatus flag retrieval without perf regression #47348

Closed
wants to merge 31 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
a50c0fc
New hidden and readonly tests
carlossanlop Jan 21, 2021
b8f69b7
FileStatus.Unix.cs public->internal
carlossanlop Jan 21, 2021
84433dd
FileStatus.Unix.cs sort members
carlossanlop Jan 21, 2021
5070cde
FileStatus.Unix.cs brackets
carlossanlop Jan 21, 2021
0686368
Add VerifyStatCall method
carlossanlop Jan 21, 2021
5e5932b
Use VerifyStatCall with existing FileStatus Lstat call
carlossanlop Jan 21, 2021
a866e73
FileStatus.Unix.cs sort Initialize method
carlossanlop Jan 21, 2021
48d0d5a
Rename _fileStatus => _mainCache, _fileStatusInitialized => _initiali…
carlossanlop Jan 21, 2021
96dd50c
Add _secondaryCache and _initializedSecondaryCache
carlossanlop Jan 21, 2021
f6b35c3
Add IsValid. Use it where value of _initializedMainCache is checked.
carlossanlop Jan 21, 2021
f865030
Update EnsureStatInitialized to check both _initialized* values
carlossanlop Jan 21, 2021
334b561
Adjust VerifyStatCall to consider ENOENT, ENOTDIR as 'path not exists…
carlossanlop Jan 21, 2021
e8e5d9b
Add and consume IsSymbolicLink and HasSymbolicLinkFlag.
carlossanlop Jan 21, 2021
82c382f
Fix Refresh stat call error verification to prevent bug in SymLinksMa…
carlossanlop Jan 21, 2021
4dbb740
Use Invalidate to reset _initialized* values
carlossanlop Jan 21, 2021
b3cb947
Add HasReadOnlyFlag and consume in GetAttributes and IsReadOnly
carlossanlop Jan 21, 2021
a69901b
Add HasHiddenFlag and IsHidden and consume in SetAttributes
carlossanlop Jan 21, 2021
9dc2681
Add IsDirectory and HasDirectoryFlag
carlossanlop Jan 22, 2021
6ba7c79
Remove Initialize. Call Invalidate and InitiallyDirectory directly on…
carlossanlop Jan 22, 2021
4b3aec0
FileSystemEntry.Initialize should get stat info via its FileStatus in…
carlossanlop Jan 22, 2021
75552fe
FileSystemEntry.Initialize should get lstat info via its FileStatus i…
carlossanlop Jan 22, 2021
62b77b8
Combine FileSystemEntry.IsHidden dot check with flag check.
carlossanlop Jan 22, 2021
ac7062b
Move internal static FileSystemInfo.Create logic into FileSystemEntry…
carlossanlop Jan 22, 2021
c4aa27f
In FileSystemEntry, only refresh each cache when needed.
carlossanlop Jan 22, 2021
371d1a4
Rename EnsureStatInitialized to EnsureCachesInitialized
carlossanlop Jan 22, 2021
f9e1619
Rename Refresh to RefreshCaches
carlossanlop Jan 22, 2021
6c489ad
Add ThrowOnCacheInitializationError
carlossanlop Jan 22, 2021
136e2c3
Only refresh relevant cache for IsHidden, IsReadOnly and IsSymbolicLink
carlossanlop Jan 22, 2021
43476e7
Ensure FileSystemEntry.IsHidden refreshes cache if needed. Invert Fil…
carlossanlop Jan 22, 2021
a869d2c
Add FileSystemEntry.HasHiddenPrefix to check for dot in name. Do soft…
carlossanlop Jan 22, 2021
76dd793
Fix test comments
carlossanlop Jan 22, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,10 @@ namespace System.IO.Enumeration

// IMPORTANT: Attribute logic must match the logic in FileStatus

entry._status = default;
entry._status.Invalidate();
Comment on lines +39 to +40
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be possible to encapsulate the logic that is modifying the FileStatus below and move it into FileStatus itself? So the FileStatus could keep all the caching logic as it's private detail and return only a valid FileStatus?

Suggested change
entry._status = default;
entry._status.Invalidate();
entry._status = FileStatus.Create(entry, directoryEntry, out FileAttributes attributes);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


bool isDirectory = false;
bool isSymlink = false;
if (directoryEntry.InodeType == Interop.Sys.NodeType.DT_DIR)
{
// We know it's a directory.
Expand All @@ -46,38 +48,49 @@ namespace System.IO.Enumeration
// Some operating systems don't have the inode type in the dirent structure,
// so we use DT_UNKNOWN as a sentinel value. As such, check if the dirent is a
// directory.
else if ((directoryEntry.InodeType == Interop.Sys.NodeType.DT_LNK
|| directoryEntry.InodeType == Interop.Sys.NodeType.DT_UNKNOWN)
&& Interop.Sys.Stat(entry.FullPath, out Interop.Sys.FileStatus targetStatus) >= 0)
else if ((directoryEntry.InodeType == Interop.Sys.NodeType.DT_LNK || directoryEntry.InodeType == Interop.Sys.NodeType.DT_UNKNOWN) &&
entry._status.TryRefreshSecondaryCache(entry.FullPath)) // Only call stat if inode is link or unknown
{
// Symlink or unknown: Stat to it to see if we can resolve it to a directory.
isDirectory = (targetStatus.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFDIR;
// Symlink or unknown: Stat should tell us if we can resolve it to a directory.
isDirectory = entry._status.HasSecondaryDirectoryFlag;
}
// Same idea as the directory check, just repeated for (and tweaked due to the
// nature of) symlinks.

// Same idea as the directory check, just repeated for (and tweaked due to the nature of) symlinks.
bool isSymlink = false;
if (directoryEntry.InodeType == Interop.Sys.NodeType.DT_LNK)
{
isSymlink = true;
}
else if ((directoryEntry.InodeType == Interop.Sys.NodeType.DT_UNKNOWN)
&& (Interop.Sys.LStat(entry.FullPath, out Interop.Sys.FileStatus linkTargetStatus) >= 0))
else if (directoryEntry.InodeType == Interop.Sys.NodeType.DT_UNKNOWN &&
entry._status.TryRefreshMainCache(entry.FullPath)) // Only call lstat if inode is unknown
{
isSymlink = (linkTargetStatus.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFLNK;
isSymlink = entry._status.HasSymbolicLinkFlag;
}

entry._status = default;
FileStatus.Initialize(ref entry._status, isDirectory);
entry._status.InitiallyDirectory = isDirectory;

FileAttributes attributes = default;
if (isSymlink)
{
attributes |= FileAttributes.ReparsePoint;
}
if (isDirectory)
{
attributes |= FileAttributes.Directory;
if (directoryEntry.Name[0] == '.')
}

// If at this point we have not collected the main cache (lstat) we don't have a hard requirement
// to hit the disk yet, so a soft check is enough to prevent perf regression.
// FileSystemEnumerator.MoveNext may hit the disk, if needed, when updating the attributes.
if (entry.HasHiddenPrefix || (entry._status.IsMainCacheValid && entry._status.HasHiddenFlag))
{
attributes |= FileAttributes.Hidden;
}

if (attributes == default)
{
attributes = FileAttributes.Normal;
}

entry._initialAttributes = attributes;
return attributes;
Expand All @@ -99,6 +112,8 @@ private ReadOnlySpan<char> FullPath
}
}

private bool HasHiddenPrefix => (_directoryEntry.NameLength > 0 && _directoryEntry.Name[0] == '.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit: I am not sure what the official repo guidelines are, but we should try to keep the private properties below the public ones: https://stackoverflow.com/a/310967/5852046

Suggested change
private bool HasHiddenPrefix => (_directoryEntry.NameLength > 0 && _directoryEntry.Name[0] == '.');
private bool HasHiddenPrefix => _directoryEntry.NameLength > 0 && _directoryEntry.Name[0] == '.';

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can do that. My intention was to keep everything alphabetically sorted by member type, ignoring accessibility.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use string.StartsWIth('.').


public ReadOnlySpan<char> FileName
{
get
Expand Down Expand Up @@ -143,12 +158,22 @@ public FileAttributes Attributes
public DateTimeOffset LastAccessTimeUtc => _status.GetLastAccessTime(FullPath, continueOnError: true);
public DateTimeOffset LastWriteTimeUtc => _status.GetLastWriteTime(FullPath, continueOnError: true);
public bool IsDirectory => _status.InitiallyDirectory;
public bool IsHidden => _directoryEntry.Name[0] == '.';
public bool IsHidden =>
HasHiddenPrefix || _status.IsHidden(FullPath, continueOnError: true); // Need to hit the disk (lstat) to check for flag

public FileSystemInfo ToFileSystemInfo()
{
Debug.Assert(!PathInternal.IsPartiallyQualified(FullPath), "FullPath should be fully qualified when constructed from directory enumeration");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug.Assert in public API? If it is important check it should be a real time check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the other comment below, I decided to revert this change since it was unrelated to the purpose of this PR.


string fullPath = ToFullPath();
return FileSystemInfo.Create(fullPath, new string(FileName), ref _status);
string fileName = new(FileName);

FileSystemInfo info = IsDirectory
? new DirectoryInfo(fullPath, fileName: fileName, isNormalized: true)
: new FileInfo(fullPath, fileName: fileName, isNormalized: true);

info.Init(ref _status);
Comment on lines +171 to +175
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not create internal ctor-s with the parameter?

Copy link
Member Author

@carlossanlop carlossanlop Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I can revert this change. Originally it was not a constructor but a factory method. I thought we could save one method call if I put all that logic in the only place where it was called. But it's unrelated to this change anyway.

return info;
}

/// <summary>
Expand Down
Loading