search

umco / umbraco-ditto

Ditto - the friendly view-model mapper for Umbraco
http://our.umbraco.org/projects/developer-tools/ditto
MIT License
79 stars 33 forks source link

Ditto Factory misusing `PluginManager.ResolveTypes`? #202

Closed leekelleher closed 7 years ago

leekelleher commented 7 years ago

Something came out of a recent Umbraco support ticket, that our logs were filled with Umbraco.Core.PluginManager entries. Which @Shazwazza raised this question...

Why is PluginManager.ResolveTypes being used so much? The reason your logs are full of this is because it shouldn't be used this way, types should be resolved one time per type in your application, please consider updating your code to only resolve once otherwise you will end up type loading using reflection all of the time and filling up your logs.

It turns out that Ditto Factory is making that call... DittoFactoryAttribute.cs line 45.

I can see why we're calling ResolveTypes from within the factory, but wondering if there's a better way to rectify this?

leekelleher commented 7 years ago

Oops! I've just noticed the AllowedTypes property... which will work around my specific issue.

Making a note to make this more prominent in our upcoming documentation.

JimBobSquarePants commented 7 years ago

That method needs caching and the result needs caching also. There's examples in the code where I cache reflected methods.

https://github.com/leekelleher/umbraco-ditto/blob/0.10.0/src/Our.Umbraco.Ditto/Common/CachedInvocations.cs

After that I would use a static ConcurrentDictionary to store the type and the resultant type collection.

mattbrailsford commented 7 years ago

I've kept it simple for now and just wrapped it in an ApplicationContext RunttimeCache method. Should do the trick.

JimBobSquarePants commented 7 years ago

That's one way to do it! 😄 I'll try and have a look when I'm a little less swamped in this imaging rubbish I keep messing about with.

Shazwazza commented 7 years ago

For this type of thing you should just be storing the result on an instance property. If you don't have that then use the StaticCache not RuntimeCache - the RuntimeCache can become cleared and then you have cache turnover problems if you fill it up. Resolving types should exist in memory and never leave, they can and will not ever change during the lifetime of the app domain.

mattbrailsford commented 7 years ago

Cheers Shan, will update to static cache.

On 23 Jan 2017 1:03 p.m., "Shannon Deminick" notifications@github.com wrote:

For this type of thing you should just be storing the result on an instance property. If you don't have that then use the StaticCache not RuntimeCache - the RuntimeCache can become cleared and then you have cache turnover problems if you fill it up. Resolving types should exist in memory and never leave, they can and will not ever change during the lifetime of the app domain.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/leekelleher/umbraco-ditto/issues/202#issuecomment-274482681, or mute the thread https://github.com/notifications/unsubscribe-auth/AAgLyUxz9GN8Fo8dBgnqQLvztIKYWkm7ks5rVKUpgaJpZM4Lmw04 .

mattbrailsford commented 7 years ago

Switched to static cache in commit c6b8637a88083e2655fdefa161bf1703a85aa0c5

mattbrailsford commented 7 years ago

@Shazwazza out of interest, where is StaticCache saved? is there a way to clear this if needed? We used RuntimeCache because I'm under the impression this lasts until the next AppPool recycle which seemed a good fit as it's possible peoples classes change due to a compile. Would be good to know when each should be used.

Shazwazza commented 7 years ago

Static cache is saved to a static dictionary. Runtime cache should be used when you want to cache application wide for a period of time but eventually want it to expire, because you know it's either going to become stale or because you know it won't be used any longer and you want to free up some memory.

Static cache is rare, normally if you want to static cache something you'd use your own variables in your own classes but you have the option to use this too. As with all static variables, there is no expiration.

Both static cache and runtime cache are cleared on app domain restart.

Since you are storing your resolved types from the app domain here you would never need to clear it, it will never become stale until the next app domain restart.

mattbrailsford commented 7 years ago

Awesome! Thanks for the explanation. Really useful.

On 23 Jan 2017 10:13 p.m., "Shannon Deminick" notifications@github.com wrote:

Static cache is saved to a static dictionary. Runtime cache should be used when you want to cache application wide for a period of time but eventually want it to expire, because you know it's either going to become stale or because you know it won't be used any longer and you want to free up some memory.

Static cache is rare, normally if you want to static cache something you'd use your own variables in your own classes but you have the option to use this too. As with all static variables, there is no expiration.

Both static cache and runtime cache are cleared on app domain restart.

Since you are storing your resolved types from the app domain here you would never need to clear it, it will never become stale until the next app domain restart.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/leekelleher/umbraco-ditto/issues/202#issuecomment-274634506, or mute the thread https://github.com/notifications/unsubscribe-auth/AAgLyVmppPF3XDfR9uQz1VBPfE2RYn6uks5rVSYMgaJpZM4Lmw04 .