Arlodotexe / OwlCore.Storage

The most flexible file system abstraction, ever. Built in partnership with the UWP Community.
16 stars 4 forks source link

Use of Async in disk IO #25

Closed HEIC-to-JPEG-Dev closed 5 months ago

HEIC-to-JPEG-Dev commented 1 year ago

Issue Disk IO is very expensive even on todays SSD drives; using synchronous calls on the UI thread is bad practice and can interupt the users experience.

Reason Any call that requires > 50ms should be Async

Example StorageFile constructor: if (!File.Exists(path)) throw new FileNotFoundException($"File not found at path {path}");

Potential Changes Change all calls to the disk sub-system to Async calls.

Arlodotexe commented 1 year ago

Thanks @HEIC-to-JPEG-Dev. We used asynchronous method calls to underlying libraries whenever available, but it's possible we missed something.

Which implementation is this for? Can you provide more information?

HEIC-to-JPEG-Dev commented 1 year ago

Systemfile creator uses a none async disk access (file.exists) - as a developer I need to start using background threads to use the class.

Arlodotexe commented 1 year ago

It sounds like using File.Exists in SystemFile's constructor is causing a performance issue for you. I'd be curious to measure this as it's a one-time operation. We should set up some quick and simple benchmarks to make sure this isn't causing excessive overhead.

HEIC-to-JPEG-Dev commented 1 year ago

File.exists, when first run, actually calls file.refresh, which in turn calls 4 other io methods to cache data. After that it’s a cached result.

Arlodotexe commented 1 year ago

Thanks, I plan on also setting up a quick formal benchmark with BenchmarkDotNet, with and without the File.Exists call.

sylveon commented 1 year ago

File.Exists should be dropped entirely #37

Arlodotexe commented 1 year ago

Someone has already done the benchmarks! :) https://github.com/sntcz/FileExistsBenchmark Some shared experience from StackOverflow as well: https://stackoverflow.com/a/35870684/5953220

This is pretty interesting, it shows that File.Exists is faster than FileInfo.Exists when the file exists, but slower when it does not. Overall, File.Exists doesn't have much of a performance penalty unless it throws.

Arlodotexe commented 7 months ago

Let's book this for re-evaluation when we move to Toolkit Labs.

This has helped me uncover bugs in code, so if it's only going to be slower when the file doesn't exist (i.e. when it's going to throw anyway), I would prefer to keep this check in.

Arlodotexe commented 5 months ago

It just dawned on me that in the benchmarks, error throwing (so the slowest scenario) are measured in microseconds, fractions of a single millisecond. The File.Exists calls is inbox to .NET and this performance may be why they made it synchronous instead of asynchronous.

The recommendation for making calls async is >50ms, while throwing for File.Exists doesn't even take half a microsecond-- 0.0005 milliseconds. We're good here.

Just to reiterate once more

This has helped me uncover bugs in code, so if it's only going to be slower when the file doesn't exist (i.e. when it's going to throw anyway), I would prefer to keep this check in.

If this poses a genuine problem for anyone in the future, please let us know the details of your scenario. For now, I'm closing this as complete.