search

jbogard / MediatR

Simple, unambitious mediator implementation in .NET
Apache License 2.0
10.91k stars 1.16k forks source link

Improve performace and make the Mediator implementation DiC independent with the registration #939

Closed Link9001 closed 9 months ago

Link9001 commented 1 year ago

As the title might suggest, this PR improves the Performance of the Mediator and also decouples the implementation from Microsoft.Extension.DependencyInjection Package. I haven't updated the Readme and all the other documentation yet because I wanted to wait for the final implementation outcome. I will update it as soon the implementation is Approved.

Features Removed

Features Moved

Features Added

Things to Discuss

I put all lot of Time and and effort in the improvements. Therefor I would appreciate it if I could add me as one of the Authors in the Project (.csproj). But bevor you reject this PR please have a look at it.

Benchmarks

Handlers

These Benchmarks show how long it takes to just invoke the corresponding handler.

Old Handler

Method Times Mean Error StdDev Gen0 Allocated
PublishingNotifications 1 178.9 ns 2.40 ns 2.01 ns 0.0687 288 B
SendingRequests 1 350.7 ns 2.31 ns 2.04 ns 0.1450 608 B
SendingRequestResponse 1 330.2 ns 2.20 ns 1.95 ns 0.1278 536 B
SendingStreamRequest 1 476.6 ns 2.87 ns 2.68 ns 0.1125 472 B
PublishingNotifications 50 8,868.2 ns 152.83 ns 119.32 ns 3.4332 14400 B
SendingRequests 50 16,206.3 ns 178.40 ns 166.87 ns 7.2632 30400 B
SendingRequestResponse 50 15,649.6 ns 262.35 ns 232.57 ns 6.3782 26800 B
SendingStreamRequest 50 23,654.5 ns 155.61 ns 145.55 ns 5.6152 23600 B

Improved Handlers

Method Times Mean Error StdDev Gen0 Allocated
PublishingNotifications 1 129.1 ns 2.52 ns 3.36 ns - -
SendingRequests 1 167.0 ns 2.87 ns 2.69 ns 0.0153 64 B
SendingRequestResponse 1 206.6 ns 1.82 ns 1.62 ns 0.0153 64 B
SendingStreamRequest 1 287.4 ns 5.70 ns 5.59 ns 0.0210 88 B
PublishingNotifications 50 5,985.6 ns 73.75 ns 61.58 ns - -
SendingRequests 50 7,224.6 ns 83.20 ns 69.48 ns 0.7629 3200 B
SendingRequestResponse 50 8,910.5 ns 122.37 ns 95.54 ns 0.7629 3200 B
SendingStreamRequest 50 13,535.3 ns 110.92 ns 103.75 ns 1.0376 4400 B

Cached Handlers

Method Times Mean Error StdDev Gen0 Allocated
PublishingNotifications 1 78.51 ns 0.510 ns 0.477 ns - -
SendingRequests 1 52.99 ns 0.295 ns 0.276 ns - -
SendingRequestResponse 1 98.95 ns 0.361 ns 0.301 ns - -
SendingStreamRequest 1 263.36 ns 1.585 ns 1.483 ns 0.0210 88 B
PublishingNotifications 50 3,696.94 ns 15.405 ns 12.027 ns - -
SendingRequests 50 2,048.98 ns 9.222 ns 8.175 ns - -
SendingRequestResponse 50 4,021.12 ns 25.300 ns 21.127 ns - -
SendingStreamRequest 50 12,547.16 ns 86.083 ns 71.883 ns 1.0376 4400 B

Pipelining

These Benchmarks show how long it takes with one Pipeline Handler to invoke the corresponding handlers.

Old Pipeline

Method Times Mean Error StdDev Gen0 Allocated
SendingRequests 1 351.1 ns 2.45 ns 2.29 ns 0.1450 608 B
SendingRequestResponse 1 328.9 ns 1.79 ns 1.49 ns 0.1278 536 B
SendingStreamRequest 1 461.2 ns 2.96 ns 2.63 ns 0.1125 472 B
SendingRequests 50 16,510.4 ns 174.15 ns 162.90 ns 7.2632 30400 B
SendingRequestResponse 50 15,376.5 ns 113.62 ns 106.28 ns 6.3934 26800 B
SendingStreamRequest 50 22,456.0 ns 175.86 ns 164.50 ns 5.6152 23600 B

Improved Pipeline

Method Times Mean Error StdDev Gen0 Allocated
SendingRequests 1 189.0 ns 3.73 ns 5.35 ns 0.0381 160 B
SendingRequestResponse 1 230.4 ns 4.46 ns 4.58 ns 0.0381 160 B
SendingStreamRequest 1 303.3 ns 5.49 ns 4.59 ns 0.0439 184 B
SendingRequests 50 7,713.8 ns 68.67 ns 60.88 ns 1.9073 8000 B
SendingRequestResponse 50 10,260.5 ns 97.92 ns 91.60 ns 1.9073 8000 B
SendingStreamRequest 50 14,503.3 ns 164.71 ns 154.07 ns 2.1973 9200 B

Cached Pipeline

Method Times Mean Error StdDev Gen0 Allocated
SendingRequests 1 57.27 ns 0.171 ns 0.143 ns - -
SendingRequestResponse 1 99.50 ns 0.375 ns 0.351 ns - -
SendingStreamRequest 1 285.45 ns 1.378 ns 1.222 ns 0.0439 184 B
SendingRequests 50 2,266.45 ns 8.808 ns 8.239 ns - -
SendingRequestResponse 50 4,042.02 ns 23.704 ns 22.173 ns - -
SendingStreamRequest 50 14,043.15 ns 76.087 ns 63.536 ns 2.1973 9200 B

Exception Handling

These Benchmarks show how long it takes to handle an Exception that is thrown by the Handler until it is handled by the only Exception Handler. There are no Exception Action Handlers registered.

Old ExceptionHandling

Method Times Mean Error StdDev Gen0 Allocated
SendingRequests 1 26.04 us 0.097 us 0.091 us 1.0986 4.58 KB
SendingRequestResponse 1 14.46 us 0.048 us 0.045 us 0.8240 3.4 KB
SendingRequests 50 1,376.33 us 8.038 us 7.125 us 54.6875 228.91 KB
SendingRequestResponse 50 705.29 us 4.533 us 4.240 us 41.0156 169.93 KB

Improved ExceptionHandling

Method Times Mean Error StdDev Gen0 Allocated
SendingRequests 1 10.24 us 0.202 us 0.326 us 0.1221 528 B
SendingRequestResponse 1 10.45 us 0.090 us 0.070 us 0.1221 536 B
SendingRequests 50 505.89 us 6.577 us 6.152 us 5.8594 26401 B
SendingRequestResponse 50 541.82 us 4.619 us 3.857 us 5.8594 26801 B

Cached Exception Handling

Method Times Mean Error StdDev Gen0 Allocated
SendingRequests 1 8.678 us 0.0539 us 0.0450 us 0.0763 368 B
SendingRequestResponse 1 8.900 us 0.0404 us 0.0315 us 0.0763 376 B
SendingRequests 50 460.521 us 5.4347 us 4.8177 us 3.9063 18401 B
SendingRequestResponse 50 465.830 us 4.5280 us 4.2355 us 4.3945 18800 B

Assembly Scanner

These Benchmarks show how long it takes to register MediatR in the Service Collection. The most expensive Operation is the Assembly Scanning and registering.

Old Assembly Scanner

Method Mean Error StdDev Gen0 Gen1 Allocated
ScanAllTypes 58.14 ms 5.498 ms 16.21 ms 4788.0859 9.7656 57.29 MB

Improved Assembly Scanner

Method Mean Error StdDev Gen0 Gen1 Allocated
ScanAllTypes 20.37 us 0.397 us 0.641 us 4.8828 1.2207 21.25 KB
jbogard commented 1 year ago

OK this PR is enormous. I appreciate you taking the time and effort! It'd take me a day to review this, which I don't have that sort of time, but some general feedback:

I JUST moved the MS.DI.Extensions package into MediatR proper because like 98% of folks that use MediatR use both. Don't create separate packages or projects, this was an intentional decision on my part. I don't want any new packages, I want to simplify. MediatR and MediatR.Contracts is good enough.

Past that, can you give a rough overview of what problems this PR is trying to solve? There's a discussion to be had around that.

Link9001 commented 1 year ago

Thanks for the quick Feedback. Well I didn't know that the Contract Project was more or less finished, but that fine. My implementation works also with the old Version. I can roll that back.

Why I even got here in the first place was due to a Side Project of mine. At the start it went well and was fast. But after some time of development Performance got worse and worse. And after some Benchmarking and Profiling I found some Issues in my Code but while I was at it I saw that the time spend in the Mediator was quite long. I am publishing a lot of messages but I heard that MediatR should be optimized. And then I started looking a little more into MediatR and its implementation. Well I saw potential for improvement. I would like to spend some of the time that currently is spent in the Mediator rather in my App. I am also using DryIoc and not ServiceCollection in my Project and registering MediatR was not that easy. But the Idea of having it in my Project is simplifying multiple things for me. Therefor the general decoupling of the ServiceCollection.

Past that, can you give a rough overview of what problems this PR is trying to solve? There's a discussion to be had around that. So the only Problem that this PR should fix is the (my) Performance Problem with the current implementation and the registration of MediatR in other Containers than ServiceCollection. But I also think that having more Performance in general is not only appreciate by me. It is fine for me if the ServiceColletion is still in the Main Nuget. It should have been more of a question then 100% changing it. Except for the ´IRequest´ Interface but it seems that I there was some more work done then I thought.

Another Question would be why are 'dynamic handler' handlers there that accept any object but in the End just cast them to the Message Type or just fail. Shouldn't the User of the Mediator know what Message he is sending? Then why even providing such methods?

remcoros commented 1 year ago

Hi @Link9001

Let me start off by saying that it's overall very appreciated by any open-source community to have contributions, be it filing issues, writing docs, investigations, or writing code. Don't let what I'm about to say put you off from achieving your own goals, but take it as good advice.

MediatR is a library with over 135 million total and 40k daily average downloads. It is serving a wide variety of projects, personal, commercial, small, and big. Jimmy has taken on the great responsibility to keep maintaining it in a professional manner. That comes with responsibilities that are not free.

It takes time to review code, especially big changes. Not only 'number of files' changes, but architectural changes (like changing interface/methods names) have to be thought over very well. Is it backward compatible? Does it break current users? Do we need a new major version number? Etc.

That all takes time, and in general, you cannot expect ANY project, be it a one-man open-source project or an entire team behind it, to just dump 200 changed files on them and ask for a review and a credit. The unit tests are there for a reason and should NOT be changed unless absolutely needed and more likely than not being discussed over before.

The fact that you changed so much of the architecture, that an update would break basically anyone using MediatR. That you don't know what the 'Send(object)' are for, and also bluntly asked for a credit in .csproj with the expectation of this getting merged, makes me think you're still a junior developer and have (and should!) still learn a thing or two.

Again, don't let this discourage you from reaching your own goals. You could still use this in your own project, or if you want to maintain your own version, you're always free to fork, give it a name and maintain it in a different repository. That's how open-source works most of the time.

Link9001 commented 1 year ago

Hi @remcoros I really appreciate your Feedback. I always like to learn new stuff and your comments aren't different. But I also did invest quite some time in how to structure and design a simpler MediatR. Also I think that there are might some misunderstandings regarding my Implementation. Correct me if my understanding is wrong, but my implementation just breaks the naming and some return types of handlers. The behavior of MediatR to send messages and letting them be handled by the Pipelines still remains the same. There will be no architectural breaking changes in the Mediator. And I also want to clarify that I would only apprehend it if I can get some Credits for what I have done. I did never demand @jbogard to make a place only for me so I can get my Credits. I think this is a big difference between demanding and just friendly asking. If he had said "no" then that is his choice. He is the Owner of this Repo and can do what he wants.

The "Send(object)" has of course its usage with the dynamic dispatching of the message type. I was caught up in removing as much reflection Code as possible because reflection is never fast. That is definitely something that I forgot about and that I should add back.

But regardless of that here are my thoughts on the current design of MediatR and how I think that it is more complicated then it should be.

First about the IBaseRequest Interface. For me it seems that this interface is another Layer to handle both Request Types with a single implementation. But for which reason are there coupled. I can not think that this should be a non-generic abstraction of the Requests because the IStreamRequest doesn't inherit from it but still is a Request. So what does this Interface provide except for the implementation of the Mediator? And why are two different Message Types even coupled.

The Unit Type is something that just bothers me the most. The dotnet team designed C# in a very good way. And for the most part you will never have to represent Nothing with a new custom Type. Just use the Task or ValueTask for all aysnc operations that need to return nothing. This seems to me like a Design flaw in the MediatR implementation, that can be fixed. On top of that the Unit Type just overcomplicates more, then helping to keep MediatR as simple as possible. And in my PR I fixed that issue.

About thous loose generic Constrains on all the IRequest... Interfaces. Thous generic parameter in should only allow the types that are Requests and nothing else. It can be quite confusing that the Object Type is event allowed in them. If a Developer just adds Types that don't implement any Message related Interfaces are not warned by the Compiler that they are using the wrong Type. Another thing that makes the current MediatR not simpler but more complicated in my Opinion.

The Pipeline Delegates that would now require the request and cancellation token parameter is just the adaption from the MVC Middleware. When something is similar to a Developer that they already know the simpler the Library gets. Most people that are using MediatR are using it with the MVC framework. And a Pipeline and a Middleware are just synonyms for the same thing. So why do it different.

About the Unittests of course they shouldn't be changed. But they also should be good maintainable and also readable. Now there isn't any structure of how Unittests are written which makes them difficult to read and to update. I just copied them over and put them into a structure that is in union with all the other tests. Having different of how the Tests are implemented doesn't make them more maintainable.

The Version have to be updated. But that just comes with it when you update a Library with new Features of C# like ValueTasks. Something updating Code creates Breaking changes. But therefor is the Versioning to let other Developers know that the new Version might break your Code. What I also have seen here is that there aren't always Major Version updates if the public interface changes.

Because making breaking changes shouldn't happen very often. But when they happen then you should at least fix some naming issues from the current implementation. My renaming of the IMediator Methods from Send to SendAsync is just something that should have already happened. Annotating Async Methods with a postfix of Async is not that uncommon today and it also makes the code more readable. This is just a best practice of writing C#.

If you disagree with something in this list, I would appreciate any feedback to make this bigger breaking change worthwhile a new Version of MediatR.

jbogard commented 1 year ago

Therefor the general decoupling of the ServiceCollection.

MediatR isn't coupled to ServiceCollection though? It's only coupled to IServiceProvider.

The optional registration is coupled to ServiceCollection, but that's absolutely by design. 95% of folks that use MediatR were using the original separated out MediatR.Extensions.Microsoft.DependencyInjection project, so I combined them together. It's absolutely not required to use with the "guts" of MediatR.

The performance of services.AddMediatR is not something I've looked much at, mainly because it affects the startup performance, not runtime performance. If you're seeing performance issues with the registration that gets "slower and slower" over time, I suspect it's something wrong with your code. MediatR registration should be a one-time activity for the lifetime of the application.

Link9001 commented 1 year ago

But the AddMediatR Method is coupled to the ServiceCollection. I like the idea of a Mediator which abstracts everything. So I added it into my Ui Application that uses DryIoC as its Container. The startup time here actually matters but because I had to write my own AddMediatR Method that isn't the Performance Issue. I know that the most people will use it with the API. And yes, there isn't the Startup time that relevant. Writing my own AddMediatR Method wasn't that of a simple task. In the End I had to look at the internal implementation of MediatR itself to figure out how to set it up correctly. And that's why I decoupled the AssemblyScanner and the whole setup with the Pipelines from the ServceCollection and made that you can use it with any container you like.

The Performance that is getting slower over time is because of sending "lots" of Messages with MediatR. My Application is growing and the more Features I add the slower it will get. I already did some benchmarking to figure out where the Bottleneck is. I also did improve in my App quite a bit. But the Exception Handling/Action and also just the sheer amount of Messages that are being sent in parallel is slowly but surly making my App slower. And after looking at the implementation of MediatR I saw lots of Reflection that could be removed and also the potential of Caching the Handlers. All of that will Improve as the Benchmarks show the Performance by a lot.

github-actions[bot] commented 11 months ago

This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days.

karol-gro commented 10 months ago

As as MediatR user I just wanted to add some two cents on MediatR+MicrosoftDI registration. While it's not a problem for me that the registration is built-in the main package, it's a bit confusing that scanning feature is required. We use modular monolith architecture and in some of our products there is no handlers at all in main project, but each module has it's own DI definition, which registers it's modules, including all handlers. It's a multitenant application (tenants as nested containers), so we want to have full control on how and what we register

So we are required to scan an assembly, which has no handlers, just to satisfy this requirement.

This auto-scanning behavior also caused some troubles to this StackOverflow user, so I'd argue that it's not always desired

jbogard commented 10 months ago

We can add an explicit opt-out for scanning of handlers OR, remove the requirement that there must be at least one assembly to scan.

karol-gro commented 10 months ago

@jbogard yeah, from what I can see, this PR removes the requirement, so that's where I wanted to mention it. But since the changes related to MS DI are still under discussion, I just wanted to stress that this part is useful change.

I'm fine with explicit opt-out as well, if that will be preferred

jbogard commented 10 months ago

Yes but this PR also does 80 other changes, some of them huuuuge breaking changes. I'll look at other similar libraries like FluentValidation to see how they like to handle it. I don't want to make normal apps harder or more confusing just to satisfy a single architectural style.

github-actions[bot] commented 9 months ago

This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] commented 9 months ago

This PR was closed because it has been stalled for 14 days with no activity.