Improve scan perf #1339
Improve scan perf #1339
Conversation
…ate loads of containers in a test suite
Codecov Report
@@ Coverage Diff @@
## develop #1339 +/- ##
========================================
Coverage 77.76% 77.77%
========================================
Files 190 190
Lines 5488 5490 +2
Branches 1097 1098 +1
========================================
+ Hits 4268 4270 +2
Misses 711 711
Partials 509 509
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -39,4 +40,16 @@ public static IEnumerable<Type> GetLoadableTypes(this Assembly assembly) | |||
| return ex.Types.Where(t => t is not null)!; | |||
| } | |||
| } | |||
|
|
|||
| private static readonly ConcurrentDictionary<Assembly, IEnumerable<Type>> PossibleAssemblyTypes = new(); | |||
There was a problem hiding this comment.
I think this is the only part of the solution that I'm a tiny bit concerned with - a static dictionary that will be used during registration but never after, but also won't ever get flushed. Do we know the memory impact here? (Also probably should move the definition to the top of the class, but that's more cosmetic than functional.) @alistairjevans am I overthinking it?
There was a problem hiding this comment.
This also relates to issue #1324, an assembly referenced in this way can never be unloaded. We haven't really agreed a solution over there, but perhaps this is a good opportunity to revisit, since I'm not sure we want to make the problem "worse".
As for memory usage, this will be a list as long as the number of valid types in the assembly, which is an unknown size. I'm pretty sure a bunch of use-cases involve assemblies with a lot of types in them.
Personally I'd imagine defining an IReflectionCache interface that lets existing code 'register' cached reflection datastores in a single location, and put an instance of that interface on the IComponentRegistryBuilder interface. This means the scanning registration extensions can reference that IReflectionCache instance. That reflection cache can then be cleared on-demand, and even passed through into the built container if needed.
I guess this is sort of an extension of the 'static' approach I started in #1326. Maybe it's actually time to make the jump and create an instance to hold a reflection cache.
There was a problem hiding this comment.
Man, I'm glad I asked, I totally forgot about that other issue about assembly unloading. Excellent point.
Agreed, some sort of instance for this that can be disposed/cleared later would maybe be better and allow the assembly unloading to work.
There was a problem hiding this comment.
That reflection cache can then be cleared on-demand, and even passed through into the built container if needed.
If letting this state go out of scope/cleared (which of course would be nicer), having a possibility to reuse the state between containers as you write above would be important. When creating one container, this PR's perf gain doesn't matter too much to us, it comes into play in our large test suites when many containers are created. So keeping this state/cache between containers (tests in our case) is what would help us.
Thanks.
There was a problem hiding this comment.
Good point @RogerKratz, it suggests we would need a constructor overload on ContainerBuilder that allows you to specify an existing IReflectionCache, and possibly expose the IReflectionCache from the Container?
There was a problem hiding this comment.
Why expose it?
Also, maybe it would be needed also if used for a single container (?), but to allow it to work in scenarios like ours when containers are used in parallel, in needs to be impl thread safe.
There was a problem hiding this comment.
Seems like there are other options if the issue is having a bunch of common things that get registered for every test but each test then has overrides - like using a common base container with the reflection registered stuff (do it once) and then nested lifetime scopes for the overrides (one per test).
I'm a little torn on ContainerBuilder constructor params. I get it, but I also wonder about precedent and future need to add things. Maybe a ContainerBuilderContext that has a property in it, so we could add to the ContainerBuilderContext later without changing/adding constructors on ContainerBuilder. Default zero-param would create the default context with whatever default cache in it.
I also know we have the Properties dictionary that gets created; maybe a well-known property is a place to set that.
public static void SetReflectionCache(this ContainerBuilder builder, IReflectionCache cache)
{
builder.Properties["SomeWellKnownKey"] = cache;
}
public static IReflectionCache GetReflectionCache(this ContainerBuilder builder)
{
return builder.Properties["SomeWellKnownKey"] as IReflectionCache;
}
var b = new ContainerBuilder();
b.SetReflectionCache(myCache);
b.RegisterAssemblyTypes(...);There was a problem hiding this comment.
Seems like there are other options if the issue is having a bunch of common things that get registered for every test but each test then has overrides - like using a common base container with the reflection registered stuff (do it once) and then nested lifetime scopes for the overrides (one per test).
I've been thinking doing it like this in our tests before. In my head this could make nasty bugs hard to find though (eg if developer has incorrectly put some mutable state on services/components etc). Sure, a fix like this PR is also in a sense "leaking state" between tests but.... ;)
new ContainerBuilder().SetReflectionCache(myCache)
sounds fine to me.
There was a problem hiding this comment.
Yep, that all makes sense.SetReflectionCache and GetReflectionCache make sense to me. What would the default instance be?
There are caches in the TypeExtensions class, currently static, that would also likely fall into "things we would put in an IReflectionCache", to address #1324 at the same time.
However, if we do move those into an IReflectionCache, and default to having a new instance per ContainerBuilder, people's existing unit test performance would plummet as each container has a new set of caches. We don't want people really to have to add SetReflectionCache everywhere to get the old performance back.
It almost feels like IReflectionCache should have a shared static Default instance to mimic current behaviour, and if someone wants to use a custom instance that can be reset on-demand, then they can do so as described...but then that leads to the same memory leak problems as before.
There was a problem hiding this comment.
Maybe the Get checks to see if the cache doesn't exist and instantiates/sets a default value on the fly?
|
@RogerKratz, I've pulled your assembly scanning caching into #1341, which is nearing merge. No additional "share cache between these builders" settings needed, so this PR has probably not been superseded? |
|
I'm out of office atm and cannot really look at code, but if I understand you correctly - just drop this PR. |
When, like us, have multiple RegisterAssemblyTypes (and large assemblies) and creating loads of containers when running our test suites, the "general type checks" IsClass, IsAbstract and IsDelegate that is called over and over again on the very same types takes noticeable time.
I used old test AssemblyScanningPerformanceTests.MeasurePerformance as a benchmark. It now runs "too fast" so fyi increased the loop with *10 as part of this PR.
Note! Everything should work as before except...
In
ScanningRegistrationExtensions.RegisterTypes(RegisterTypes, params Type[]), it's different behavior now. the "general type checks" mentioned above is no longer called, as that method callsScanTypesdirectly. Not sure if these "general type checks" needs to be added explicitly here? I didn't change it in this PR as all tests are green, better listen with you first.