Improved performance of assembly scanning #133

Closed
blairconrad opened this Issue Jun 19, 2013 · 13 comments

Projects

None yet

3 participants

@blairconrad
Member

This issue was spun off of issue 130 after some investigation was done there.

The Nancy directory and AppDomain scanning for extension types is more sophisticated than that in FakeItEasy, and emulating it could bring speed and stability to FakeItEasy's extension auto-discovery process. Here's the gist:

  1. the locations of all Assemblies in the AppDomain are found, then any DLLs found when scanning the Application Directory that are already in the list are not loaded
  2. when DLLs are loaded, they're first loaded using Assembly.ReflectionOnlyLoadFrom, a lighter load
  3. then the newly-loaded assembly's references are checked - if the assembly doesn't reference a Nancy assembly, it's skipped, since none of its classes can extend the bootstrapper
  4. if we pass all these checks, the assembly is fully loaded and only then is its types scanned

Assemblies in the AppDomain have their references checked as well, before their types are scanned.

This sounded like it would resolve the problems I encountered before creating issue 130 - potentially quicker scans as many assembly files don't need to be opened at all, and we only scan for types when there's a chance that we implement one of the ArgumentValueFormatter, DummyDefinition, and FakeConfigurator interfaces. By not fully loading many of other assembly files in the directory, I figured, we'd lose the LoaderLock and Runtime Errors as well.

An rough implementation worked well for for me, changing ApplicationDirectoryAssembliesTypeCatalogue to implement what I just described. The FakeItEasy tests all passed (I'd reported earlier that Should_be_able_to_get_types_from_assembly_in_same_directory failed, but after a tweak it doesn't, although I think that the test is not implemented properly - it's just looking for A in the found types, which

  1. is not an extension type, so kind of corrupts the spirit of the test, and
  2. is probably in the AppDomain, not just the current directory)

My application's tests passed. There were no popups or crashes in my runner. And the tests started up quickly, so long as I didn't let my test runner make shadow copies of my assemblies (it messes up the "have we loaded the assembly from this path already" shortcut).

Since we only use the ApplicationDirectoryAssembliesTypeCatalogue to provide a list of types for the ImportsModule, I think the change would be safe - we wouldn't lose functionality. Also, we'd get more intelligent default scanning behaviour that would benefit all users (or at worst not harm them) even if they didn't provide a custom bootstrapper. I think the best next steps are to implement this issue and then reassess issue 130, which I'd be happy to work on, but we'd need to decide if the "assemblies to scan" override is the best first extension point. It's no worse than any, I think.

@blairconrad blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Jun 19, 2013
@blairconrad blairconrad #133 - rough implementation of improved scanning for extensions, insp…
…ired by Nancy
e052091
@blairconrad blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Jun 19, 2013
@blairconrad blairconrad #133 - removing commented-out code 8afc4cd
@blairconrad
Member

@philippdolder, back in #130 you asked about loading the types from the assemblies after they'd been loaded using Assembly.ReflectionOnlyLoadFrom. I confirmed that this isn't possible. We need to fully load the assembly before getting the types. Otherwise, GetTypes throws an exception:

Cannot resolve dependency to assembly 'an assembly that the one we're examining depends on' because it has not been preloaded. When using the ReflectionOnly APIs, dependent assemblies must be pre-loaded or loaded on demand through the ReflectionOnlyAssemblyResolve event.":null}

An alternative (as the error says) is to register for the ReflectionOnlyAssemblyResolve event, but for the relatively few assemblies in the directory but not in the AppDomain and that reference FakeItEasy, I'd suggest that it's not worth the effort.

@philippdolder
Member

@blairconrad I agree. It's ok as we are down to very few assemblies with the initial filterings applied

@adamralph
Member

Marking as taken by @blairconrad (thanks!)

@blairconrad
Member

@adamralph, does this mean you've reviewed and are at least not completely opposed to the implementation? I think the only other thing I'd do to this is to update Should_be_able_to_get_types_from_assembly_in_same_directory test to actually load a type from a non-AppDomain assembly. This may require creating a new assembly that implements an extension,or maybe copying an assembly in from another project, such as the example (I've not checked yet to see if anything's available).
After that, if it's not hated, I could issue a pull request?

@adamralph
Member

No, it doesn't mean that, I just assumed you'd taken it on 😉. I'll take a closer look at your branch tomorrow and offer any comments.

@blairconrad
Member

Okay. I look forward to it. And yeah, I've totally taken it on. On is where I took it.

@adamralph
Member

@blairconrad the implementation looks good!

2 comments for slight enhancement:

  1. I would probably collapse GetAllAvailableAssemblies() and LoadAssemblyFilesWithFakeItEasyReferences() into one method so that GetAllAssembliesInAppDomain() only needs to be called once (and can probably then be removed and inlined).
  2. I would use a List<T> instead of a LinkedList<T>. Unless I've missed something, I can't see a reason to use LinkedList<T> here and it is slower than List<T>.
@adamralph adamralph referenced this issue Jun 25, 2013
Merged

Issue133 #135

@adamralph adamralph was assigned Jun 25, 2013
@adamralph adamralph added a commit to adamralph/FakeItEasy that referenced this issue Jun 25, 2013
@adamralph adamralph #133 reformatting and refactoring of ApplicationDirectoryAssembliesTy…
…peCatalogue

* moved field assignment from constructor to initializer
* inlined single use private methods
* inlined variables
* shortened variable names
* replaced foreach continuation with Linq method call
* reduced nesting
* formatted comments
* whitespace
4637890
@adamralph adamralph closed this Jun 25, 2013
@adamralph
Member

@blairconrad I've released a beta to NuGet - would be great if you could try it out on a real world code base to make sure everything's working as expected. I'll do the same today. https://nuget.org/packages/FakeItEasy/1.13.0-beta01

@blairconrad
Member

@adamralph, that's great. I've installed it and used it against our "problem solution" and also ad hocked up a smaller test project to test the ArgumentValueFormatter location. It worked a charm.

@adamralph
Member

Excellent! I ran it against a couple of large solutions yesterday and it all seemed fine too. I should have time to push out a stable package sometime later today.

@adamralph
Member

@blairconrad out of interest, how much performance improvement did you observe in your "problem solution"?

@blairconrad
Member

@adamralph, I just compared against 1.11 (we don't use NuGet at work, instead having a custom (legacy) package management system, and 1.11 was what we were using before we started using 1.13 tomorrow).

1.13 shows my first test running in 0.534 seconds (the actual test takes no time. All or nearly all of the elapsed time would be scanning assemblies for NUnit and FakeItEasy). 1.11 shows 1.822 tonight, for a savings of almost 1.5 seconds.

1.822 seems really fast, though. I'm accustomed to seeing something in the 5 range, but I didn't record the numbers, so it's just an anecdote, I guess.

I'm enjoying the speedup, and the Guy Across the Aisle is over the moon, but that benefit was secondary. The real win for me is the lack of loaderlocks and testrunner crashes.

@adamralph
Member

That's awesome. So more than 3x speed up in that case. FYI the NancyFx guys were asking about it over at https://jabbr.net/#/rooms/nancyfx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment