Improve execution speed by only scanning the current AppDomain for extensions by default #268

Closed
adamralph opened this Issue Mar 1, 2014 · 21 comments

Projects

None yet

3 participants

@adamralph
Member

This should speed up test execution quite a lot in some circumstances. The old behaviour can still be replicated by creating a custom bootstrapper inheriting from DefaultBootstrapper and overriding the GetAssemblyFileNamesToScanForExtensions method.


UPDATE

This change was implemented in version 1.19. For anyone who wants to restore the old behaviours, see https://github.com/FakeItEasy/FakeItEasy/wiki/Bootstrapper

@adamralph adamralph added this to the 2.0 milestone Mar 1, 2014
@matkoch
matkoch commented Mar 2, 2014

I ran into this during developing a test runner for ReSharper. FakeItEasy tried to load all dll files from the current directory. That was indeed ReSharper\bin with more than 250 dll's and took quite a long time. I assume that you want to convert the code into getting all dll's from the base directory of the current app domain. I think this would be a good behavior.

In the DefaultBootstrapper it should be:

return Directory.GetFiles(AppDomain.CurrentDomain.BaseDirectory, "*.dll", SearchOption.TopDirectoryOnly);
@adamralph
Member

@matkoch the intention is to change the default behaviour to not scan for assemblies on the file system at all. Only assemblies already loaded into the current AppDomain (in memory) would be scanned.

@matkoch
matkoch commented Mar 2, 2014

@adamralph Not sure about that.. Sometimes it could be nice that recently unloaded but referenced assemblies are considered automatically... But I would definitely change the behaviour of the DefaultBootstrapper... It's a bad idea to rely on the current directory.

@blairconrad
Member

@matkoch, even though using AppDomain.CurrentDomain.BaseDirectory may be superior to using the current working directory, I don't think this is a change we'll want to make. Any change to existing behaviour has the potential to be disruptive for users, but if we're going to disrupt users, we should aim for what it looks like will provide the best experience for the majority of users.

I think externally-referenced extensions are infrequently used (actually, my guess is that very few people use any of the extension points, but that's another topic), and that the best change in DefaultBootstrapper behaviour would be to stop scanning assemblies on disk.
We've had several complaints about the inconvenience that the scanning can cause, and the new bootstrapper behaviour will allow users who care about externally-defined extensions to explicitly target assembly files for inclusion.

I must confess I don't understand how often recently unloaded but referenced assemblies are a factor—in most cases, I'd expect FakeItEasy's scanning to occur pretty early in the testing process, so I'm not sure why assemblies would've been unloaded. If you're interested in expanding on that, I'd appreciate the learning opportunity, but don't feel obligated.

In any event, if/when we do change the DefaultBootstrapper behaviour in the manner that @adamralph described, it should be a simple matter for you to implement a custom bootstrapper to either scan all assemblies from the AppDomain.CurrentDomain.BaseDirectory or target specific assemblies that contain extensions.

@matkoch
matkoch commented Mar 2, 2014

I would break it ;) At least somewhere in the future. I'm using FIE very often but I'm not aware about the extension capabilities. I don't want to talk only about me, but it might be a good idea to remove the bootstrapper by default.

@blairconrad: Maybe I was a bit inaccurately when talking about "recently unloaded but referenced assemblies". Generally spoken, given a framework you could easily extend various behaviour by just referencing an extension library. That library must not be necessarily loaded when you try to load all extensions. Therefore you should scan the application base directory (where all referenced assemblies are located) and load them manually (and scan for appropriate implementations of your extension interface). So I wasn't talking about explicitly unloading an assembly.

BTW, FIE scans for assemblies at the time you create your first fake object.

@adamralph
Member

Hmmm @matkoch raises an interesting point about referenced assemblies.

It should be possible to fairly easily change the behaviour so that we scan not only assemblies already loaded in the current AppDomain but also any assemblies which are referenced by any of those assemblies. We'd have to do this recursively until all reference paths are exhausted.

For 1.x we can then continue scanning for any other assemblies on disk, but when we break this behaviour in 2.0 it will probably break very few consumers. So much so that I'm almost tempted to call it a non-breaking change and do it in 1.x. After all, how many people are going to have assemblies in the output folder of their project which they are not referencing? Sounds like a corner case to me.

It all sounds a bit too obvious, am I missing something?

@blairconrad
Member

I agree that having assemblies in the output folder of the project that aren't being referenced seems like a corner case. Unfortunately, I think the corner case that it is is the one that the on-disk scanning was added to handle—someone bulids an ArgumentValueFormatter (say) in an assembly and then wants to share it among their projects. Or the world. So they copy the assembly into the output directory to get picked up automatically, as sometimes people do with plugins.

Now, I would still be surprised to learn that anyone is doing this. I think our common use cases, in descending order of popularity, are for users who

  1. don't define FakeItEasy extensions, or
  2. define the extensions in the assembly that their tests live in, or
  3. define extensions in another assembly that also contains helpful methods or types that will be used in the test assembly, so that the other assembly will be loaded into the AppDomain, or
  4. are @patrik-hagne

Still, it is a bit of a breaking change. I know for few clients, but we'd be breaking a promise to those clients.

Oh, and we should consider that it's not assemblies in the output folder that we have to worry about—it's assemblies in the current working directory. This is why @matkoch had a problem to begin with—FakeItEasy was initialized when the current directory was not the same as AppDomain.CurrentDomain.BaseDirectory. It's possible that people are changing working directories and then relying on the difference, instead of changing working directories and being surprised by the difference.
I think (and I haven't done this in a while, precisely because of the behaviour I describe) if one makes an NUnit project that executes tests from multiple assemblies, the working directory becomes the directory that the project file lives in, and I've had this cause surprises when accessing on-disk resources in some integration tests.

This next question ranges a bit far afield, so if you prefer, @adamralph, maybe we should take it offline, but I'm curious about the resistance to bumping the Major component in the version number. Since I've shown up, I've been calling changes breaking and suggesting we SemVerishly move to version 2.0.0. I understand that there breaking changes and then there are breaking changes, but still I worry, and I'd rather surprise people by warning of a "breaking change" that they don't care about than by not warning them of a change in documented behaviour that they have been relying on.
Are you concerned that people will see the Major version bump, not read release notes, become scared and then not upgrade (or jump ship to one of our worthy competitors)?

@blairconrad
Member

As an alternative, @adamralph, we could hold off making the "don't scan the disk" change until we're ready for another breaking/Major release.

Unfortunately, this the only breaking issue that I see right now that we're likely to want to move on in the near future, unless something dramatic happens with #30 or #175 in the near future.

Either way, we could address the recursive reference-scanning separately, possibly earlier. Although my guess is that we're still looking at a sliver of users who will benefit (basically category 4 from my previous comment), and that maybe we just let them override the bootstrapper.

@matkoch
matkoch commented Mar 4, 2014

Sure, there may be some users who rely on that behavior. But for me it's more a bug than an improvement / breaking change. You could easily get a different behavior of your tests, if you take the test runner (command line tool) and execute it from another directory.

@blairconrad
Member

@matkoch, I'm no longer sure if you're advocating for the change in which directory is scanned or for the removal of the scanning behaviour altogether.

Either way, while I understand your point about the differing behaviour, I can't call the current behaviour a bug. It was definitely intentional and has been behaving the way it does since September 2010. Moreover, it's has been documented in the wiki since July, so it's part of the public API. Granted, a part that probably few people rely on.

@matkoch
matkoch commented Mar 6, 2014

Well, if I were a maintainer of FIE, I'd probably change the scanning behavior to rely on the appdomain base path. But I'm not 😁

@blairconrad
Member

@adamralph, after much soul-searching, I think I'm coming to accept "don't bump up the Major version".

As far as recursively loading the referenced assemblies goes, I have a concern. What if the libraries reference assemblies that aren't present? Perhaps there's conditional execution based on OS version, or some kind of configuration (detecting whether other programs are installed, say)? Then when the recursive scan tries to load assemblies that aren't present, we'll spew error messages that will alarm the client.

On the other hand, I concede that there might be some value in performing the recursive search, but I'm going to throw a wrinkle in—should this deep search be used to find the bootstrapper?

Given my recent meditations on the assumed usage rate of the extension methods, I lean toward removing the directory scan and leaving the rest of the type-finding algorithm as it is. I think putting a bootstrapper in an assembly that will be loaded into the current AppDomain and using it to locate any "hard to find" extensions is a reasonable thing to ask of the users who will want to define the extensions in hard to find assemblies. Especially since there's nothing about the bootstrapper or extensions that would require the use to reference the assembly anyhow; we'd be relying on them referencing the hard to find assemblies for some other reason.

@adamralph
Member

I'll sleep on it and get back to you 😉

@blairconrad
Member

Mr. @adamralph, how's the sleeping going? 😀

@adamralph
Member

@blairconrad OK, I'm thinking that we go with your suggestion of removing the directory scan and leaving everything else as is.

I'm also in favour of regarding this as a non-breaking change. The worst that can possibly happen is that a minority of consumers will get slightly less helpful exception messages when they break a test and it's easy for them to restore the old behaviour. SemVer says you should bump the major version 'when you make incompatible API changes'. I don't think we're making an 'incompatible API change' here.

@adamralph adamralph added 1 - Ready and removed 0 - Backlog labels Mar 19, 2014
@adamralph adamralph modified the milestone: 1.19, 2.0 Mar 19, 2014
@adamralph adamralph removed the breaking label Mar 19, 2014
@adamralph adamralph changed the title from Only scan the current AppDomain for extensions by default to Improve execution speed by only scanning the current AppDomain for extensions by default Mar 19, 2014
@adamralph
Member

Moved to Ready, removed breaking label, removed from 2.0 milestone. Updated title to reflect benefit of this enhancement.

@blairconrad blairconrad self-assigned this Mar 19, 2014
@blairconrad
Member

Thanks, @adamralph. Agreement all around. I'm claiming it, unless you want it.

@blairconrad blairconrad added 2 - Working and removed 1 - Ready labels Mar 20, 2014
@blairconrad blairconrad added this to the 1.19 milestone Mar 20, 2014
@blairconrad blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Mar 20, 2014
@blairconrad blairconrad #268 - adding (failing) test 061f5a4
@blairconrad blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Mar 20, 2014
@blairconrad blairconrad #268 - making DefaultBootstrapper.GetAssemblyFileNamesToScanForExtens…
…ions return an empty list
4c0f6e2
@adamralph adamralph closed this in #278 Mar 20, 2014
@adamralph adamralph added 3 - Done and removed 2 - Working labels Mar 20, 2014
@matkoch
matkoch commented Mar 20, 2014

Thanks! :)

@blairconrad
Member

Note

The previous behaviour can be realized by including a custom Bootstrapper in a test assembly. For example:

public class MyBootstrapper : DefaultBootstrapper
{
    /// <summary>
    /// Returns the absolute paths of all the DLLs in the
    /// <see cref="System.Environment.CurrentDirectory"/>.
    /// </summary>
    public override IEnumerable<string> GetAssemblyFileNamesToScanForExtensions()
    {
        return Directory.GetFiles(Environment.CurrentDirectory, "*.dll");
    }
}

Although a better option in most cases would be to target the assemblies that contain extension points:

public class MyBootstrapper : DefaultBootstrapper
{
    /// <summary>
    /// Returns the absolute path to my special assembly that defines
    /// FakeItEasy extension points.
    /// </summary>
    public override IEnumerable<string> GetAssemblyFileNamesToScanForExtensions()
    {
        return new [] { Path.GetFullPath("MyExtensions.dll") };
    }
}

This approach will ensure that only one external assembly is loaded, reducing startup times.

@blairconrad
Member

Removed contribution label, since we went a whole different way with the issue than we thought.

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