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

Remove redundant validation when enumerating files & folders in SystemFile/SystemFolder. #56

Merged

Conversation

itsWindows11
Copy link
Contributor

@itsWindows11 itsWindows11 commented Jun 19, 2024

Currently, when enumerating or copying files, we are checking whether the file exists or not in the constructor. This can cause unnecessary huge disk I/O overhead especially with large folders.

A new internal constructor is created specifically to prevent this scenario (only creates an instance with the necessary item info, nothing else), since in these cases we already know that the file/folder indeed exists.

cc. @HEIC-to-JPEG-Dev feel free to review.

Copy link
Owner

@Arlodotexe Arlodotexe left a comment

Choose a reason for hiding this comment

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

This is a great change. The constructor validation/guard is largely meant for consumers who might provide an invalid or nonexistent path. A file or folder resource should exist before it's accessed through our abstraction.

That means instances that are created internally can pre-validate this and skip the consumer-facing validation/guard.

It's a free performance gain that might be especially noticeable for extremely large storage graphs.

My only suggestion is that we rename this from isInternal to something more descriptive of what it does. Maybe we just call it noValidation and keep the constructor internal?

@itsWindows11
Copy link
Contributor Author

I've merged the PR with the requested changes.

Copy link
Owner

@Arlodotexe Arlodotexe left a comment

Choose a reason for hiding this comment

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

Great work here, we'll get this out in the next version bump.

@itsWindows11
Copy link
Contributor Author

Just added even more optimizations that should get the enumeration time in tests to less than 1ms in most cases. Using StorableType.All mode we are now using the DirectoryInfo to enumerate file system info entries, which makes it easier to distinguish between files and folders by just accessing the attributes instead of another File.Exists() call.

@itsWindows11 itsWindows11 requested a review from Arlodotexe June 22, 2024 14:25
Copy link
Owner

@Arlodotexe Arlodotexe left a comment

Choose a reason for hiding this comment

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

New changes look good!

@Arlodotexe Arlodotexe changed the title Fixed duplicate Exists() check when enumerating files & folders in SystemFile/SystemFolder. Remove redundant validation when enumerating files & folders in SystemFile/SystemFolder. Jun 23, 2024
@Arlodotexe Arlodotexe merged commit 1a6e589 into Arlodotexe:main Jun 24, 2024
1 check passed
@itsWindows11 itsWindows11 deleted the imp/dupe-sysio-impl-exists-check branch June 24, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants