Make the cache "just work" by applying an attribute + test suite.

[mlidbom]

This is more a request for review than a pull request. The code seems to work, but is not "done". What's to like:

• It just works: Add the new AutoOutputCache(preliminary name) attribute and you can trust that the policy is obeyed. No matter which .Action overload you use or how your action is invoked. The policy willalways simply work.
• More than 6 times faster. No kidding. Check out the load test controller :).
• NUnit test suite that tests the real output from a real MVC application and does some unit tests too.
• Works no matter what the parameter to your action. It need not be DataContractSerializer compatible.

What's not to like:

• Not compatible with distributed cache. Fixable I think(Obviously with complexities related to serializers etc.).
• Memory leaks are conceivable. (It stores the Action Parameters without serializing them. Should a param regularly include something like an NHibernate entity with a ton of references..... )
• Not done/clean. It's in its own temporary folder/namespace : Mlidbo to keep it separate for now until I get your feedback. Not everything is clean code yet.
• Tests are primarily for the new "It just works" code.

Why: The reason I send you this without more polishing is that I've been working like mad to get this reasonably sane before our release(Too soon). In short the old code was just not reliable enough for us when nesting actions in a ton of different ways including sections, nested layouts, CMS infrastructure called actions etc. And we require the performance that caching gives us. So I've been working from the time I wake up to the time I go to sleep on this since we last talked (Local time here is half past three in the morning..).

I would really appreciate it if you could give me feedback on the code. More than anything it would be great if you could tell me about any gotchas that you think my code has not considered. Anything that might cause us grief in production.

Regards /Magnus

[moonpyk]

Huge work here, I'm gonna look closely.

[mlidbom]

Thanks. That's very much appreciated :) I'm still working on it working out some issues with exceptions etc so new commits are likely to follow soon.

One clarification. I've tried not to touch the old structure code more than absolutely necessary. Except for if the change to CacheItem breaks serialization I think all the old functionality should be unchanged.

[moonpyk]

Yep, old functionnality has to been not changed at all.

[mlidbom]

One more note. I went down more than one dead end. Reading individual commits will probably be a waste of your time. I recommend just diffing the latest from this branch against the latest from dev.

Unless you want to smile at my initial non-understanding of how actions are invoked in MVC of course :)

[moonpyk]

The Donut class cannot be serialized, it's way too complex. Adding it as a property to the existing CacheItem class is not possible.

In the way things are actually designed, we need to have a subclass, something like AutoCacheItem that has this property.

[mlidbom]

OK. I'm on it.

[mlidbom]

I've pushed AutoCacheItem now.

[mlidbom]

I love how tests give me confidence and speed . I have most common cases covered so I don't need to do a ton of manual testing to make a change :)

[moonpyk]

The Donut class does actually all the work. I think that it maybe would be better to split the responsabilities, so that the Donut becomes just some Tree like container. And give the responsability to handle the work to another class.

[moonpyk]

(https://jabbr.net/#/rooms/mvcdonutcaching)

[mlidbom]

Agreed. The current class should be split into at least two. The responsibility for gathering data should not be mixed with the responsibility for rendering it once cached. And a simple structure holder would probably be good for storing it in the cache. So probably 3 classes. I think that's going to have to wait until I've ironed out the bugs with the output being wrong when exceptions are thrown.

[mlidbom]

Just letting you know that the code structure has been very much improved now. It's not perfect in any way but i don't think it needs major restructuring anymore.

Test coverage is also much improved. I believe that most standard usage scenarios are covered along with quite a few edge cases.

[mlidbom]

Another note. Performance being several times better was an illusion caused by a bug :( It is still faster than the old code, but not by any huge margin. Profiling shows that it is the mvc action invocation infrastructure that causes most/all of the slowdown. (Instantiation via autofac and glimpse inserted proxies make up much of the cost. Just taking Autofac out of the loop in the test project made requests jump from 1600/s too 2700/s)

In theory action invocations could be avoided when child actions are in the cache and performance should go up several times. However that would break things like authentication via attributes on child actions etc. (Of course this is already broken in the old code...)

I've considered ways of exposing the ability to flexibly and easily pick and choose among strategies for this and ways to try and make the code self documenting so that users will not be surprised by things like authentication attributes not being honored.

I'd like to keep any such dangerous optimizations as explicit choices though. We are already talking about something that will push out thousands of requests a second for fully cached pages that don't have lots of nested child actions. Even for the case where you have 20 child actions, invoked/instantiated via Autofac/Glimpse, nested in 2 levels and quite a large page it spits out about 500/s on a 6 VCPU virtual machine. It's not exactly slow :)

I'm also considering ways of specifying overrides/additions to the key generation on a per attribute basis. This would allow you to safely cache things such as user specific navigation etc which could really speed things up. In practice in real word projects I believe that that sort of feature will often buy much more performance than trying to wring more performance out of the cached case. The cached case is already orders of magnitude faster than the non-cached case. Expanding the cases that can be safely cached should be prioritized above features that are likely to bite the developer in my opinion.

[moonpyk]

You are doing a really awesome work, I looking from time to time. I let you test your stuff. Next week I'll be in formation and have more time to test and take decisions.

[mlidbom]

I'm glad you like it. It's been quite fun(but stressful due to the time limit) to sit down and attack a hard programming challenge for the first time in a while. I consult as the architect for two teams of developers and spend most of my time with code review and helping other developers out. It's only when we run into something really complex and/or really high priority that I get to sit down and crunch code. I wish I got to do it more.

Looking forward to your feedback when you have more time :)

[mlidbom]

Just making a note that this is now in use in a production website, since about a week, with no problems so far.

[mlidbom]

Checking in to let you know that this has been running rock solid for us. We have not seen a single problem with it during the 4 months since we went into production. Is there any prospect of this getting merged or should I be considering creating a fork?

[moonpyk]

Yeah, I think it's a better idea. Your solution is kinda like a revolution and I have to deal with existing codebases... So yeah, your PowerDonutCaching should live his life, maybe we'll reach each other back at some point.

[mlidbom]

Yeah, This is hardly an incremental change. Essentially the only thing left is the configuration code. I fully understand :)

I'll let you know when/if I do decide to publish a fork. Thanks a thousand times for showing me the wrinkles of the output system in asp.net and how to navigate them in reasonable safety. Whatever the new project will be called it would never have been born without your work. I've found it really fun chatting with you and talking code. I really hope we can remain friendly and not descend into flame wars as so many forked projects do. On that note there is something to consider. Naming. The new project certainly is an implementation of donut caching for Asp.Net MVC.

How do I name it without causing confusion and/or offending you? PowerDonutCaching? :)

[moonpyk]

PowerDonutCaching is the nickname I gave to your branch on my local working copy :smile: I don't take your fork as a goodbye, just a new beginning. We will remain friendly, because we both are not the flame trolls type. I'm pretty sure we'll discuss again code and exchange ideas because we both love it.

I propose a toast to developer brothership and good code :wine_glass:

[mlidbom]

I'll drink to that :smile:

Cheers! :wine_glass: