Reduce allocations in PhysicalFileProvider when GetDirectoryContents is called but not enumerated #240

natemcmaster · 2016-10-07T20:41:04Z

Resolves #224

Optimize PhysicalFileProvider for scenario when it is basically being used for Directory.Exists() (cough, looking at you, aspnet/StaticFiles).

Add singleton of NotFoundDirectoryContents.
Delay calling DirectoryInfo.EnumerateFileSystemInfos which does lots of up-front allocation.

Benchmark: 100 checks for directory that exists but contents not enumerated
Importantly, this isn't creating an enumerator. We just want to know if the directory exist.

bool exists;
for (var i = 0; i < 100; i++)
{
    var dirContents = provider.GetDirectoryContents("exists");
    exists = dirContents.Exists;
}

Before:

After:

Benchmark: 100 checks for directory that does not exists

bool exists;
for (var i = 0; i < 100; i++)
{
    var dirContents = provider.GetDirectoryContents("doesnotexist");
    exists = dirContents.Exists;
    var e = dirContents.GetEnumerator();
}

Before:

After:

cc @davidfowl @Tratcher

natemcmaster · 2016-10-07T20:48:50Z

Typo. This resolves #224 not 214

pranavkm · 2016-10-07T21:10:52Z

src/Microsoft.Extensions.FileProviders.Abstractions/NotFoundDirectoryContents.cs

@@ -13,6 +13,11 @@ namespace Microsoft.Extensions.FileProviders
    public class NotFoundDirectoryContents : IDirectoryContents
    {
        /// <summary>
+        /// A shared instance of <see cref="NotFoundDirectoryContents"/> 
+        /// </summary>
+        public static NotFoundDirectoryContents Singleton { get; } = new NotFoundDirectoryContents();


We've use Singleton in abstractions, e.g. NullChangeToken.Singleton. Seems like they mean the same thing. Is there some nuance I'm missing?

Gotcha. Never mind in that case.

pranavkm · 2016-10-07T21:12:00Z

src/Microsoft.Extensions.FileProviders.Physical/PhysicalFileProvider.cs

+
+        private static IDirectoryContents GetEnumerableContents(string fullPath)
+        {
+            return new EnumerableDirectoryContents(() =>


Couldn't we pass in the DirectoryInfo to EnumerableDirectoryContents and have it iterate it lazily (instead of passing in the Func?)

We could, but EnumerableDirectoryContents shared with providers that don't necessarily use the physical file system.

Could we fork it and make a PhysicalDirectoryContents? There's not much to implement in the interface.

I considered that too and started an implementation with that approach, but ended up with the current implementation. There are probably a dozen ways it could be done, but I landed on this. I couldn't see a strong case for adding a new type such as PhysicalDirectoryContents.

pranavkm · 2016-10-07T21:14:10Z

src/Microsoft.Extensions.FileProviders.Sources/EnumerableDirectoryContents.cs

+
+        private void EnsureInitialized()
+        {
+            LazyInitializer.EnsureInitialized(ref _entries, _entriesFactory);


Probably don't need any of the behavior of LazyInitializer. if (_entries == null) { .. }` should suffice.

Fair enough. Will update.

natemcmaster · 2016-10-11T18:10:49Z

Ping. @Tratcher can you also review?

Tratcher · 2016-10-11T18:32:00Z

src/Microsoft.Extensions.FileProviders.Physical/PhysicalFileProvider.cs

-                    });
-
-                return new EnumerableDirectoryContents(contents);
+                return GetEnumerableContents(fullPath);
            }
            catch (DirectoryNotFoundException)


Do these catches need to be moved within the closure?

That's a good catch. Due to our lazy instantiation of the enumerator, it's possible the directory could be deleted after we create the EnumerableDirectoryContents and before it's enumerated. I'll move it inside the closure.

or duplicate it..

natemcmaster · 2016-10-11T22:06:04Z

🆙 📅

dnfclas added the cla-already-signed label Oct 7, 2016

pranavkm suggested changes Oct 7, 2016

View reviewed changes

Tratcher reviewed Oct 11, 2016

View reviewed changes

natemcmaster force-pushed the namc/less-enumeration branch from 87aa57f to 35779b9 Compare October 11, 2016 21:45

Tratcher approved these changes Oct 11, 2016

View reviewed changes

Defer creation of directory contents enumerator

a54ef9c

natemcmaster force-pushed the namc/less-enumeration branch from 28754ae to a54ef9c Compare October 11, 2016 22:15

natemcmaster merged commit a54ef9c into dev Oct 11, 2016

natemcmaster deleted the namc/less-enumeration branch October 11, 2016 22:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce allocations in PhysicalFileProvider when GetDirectoryContents is called but not enumerated #240

Reduce allocations in PhysicalFileProvider when GetDirectoryContents is called but not enumerated #240

natemcmaster commented Oct 7, 2016 •

edited

Loading

natemcmaster commented Oct 7, 2016

pranavkm Oct 7, 2016

natemcmaster Oct 7, 2016

pranavkm Oct 7, 2016

pranavkm Oct 7, 2016

natemcmaster Oct 7, 2016

pranavkm Oct 7, 2016

natemcmaster Oct 7, 2016

pranavkm Oct 7, 2016

natemcmaster Oct 7, 2016

natemcmaster commented Oct 11, 2016

Tratcher Oct 11, 2016

natemcmaster Oct 11, 2016

Tratcher Oct 11, 2016

natemcmaster commented Oct 11, 2016

Reduce allocations in PhysicalFileProvider when GetDirectoryContents is called but not enumerated #240

Reduce allocations in PhysicalFileProvider when GetDirectoryContents is called but not enumerated #240

Conversation

natemcmaster commented Oct 7, 2016 • edited Loading

natemcmaster commented Oct 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster commented Oct 11, 2016

natemcmaster commented Oct 7, 2016 •

edited

Loading