Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parallel notifications, correct ordering for switch statement cases #145

Merged
merged 18 commits into from
Apr 7, 2024

Conversation

martinothamar
Copy link
Owner

@martinothamar martinothamar commented Mar 29, 2024

Solves #115 and #138

  • INotificationPublisher abstraction similar to that of MediatR, apart from making the interface slightly worse while allowing better performance (avoiding closures, same as for the pipeline behavior interface)
  • Ordering of requests when building the compilation model now makes switch statements correct when there is inheritance and the base classes aren't abstract
  • Mediator.Abstractions references .NET Standard 2.0 + NET 6.0 with conditional package references to
    • Microsoft.Bcl.AsyncInterfaces for IAsyncEnumerable<T>
    • System.Runtime.CompilerServices.Unsafe to avoid cost of casting
    • PolySharp for some attributes

Still unsure if netstandard2.0 is the way to go.. that might chage

TODO

  •  Better naming?
  •  More tests
  •  More benchmarking - for multiple handlers (sync and async)
  •  Experiment more with ValueTask.WhenAll variant based on IValueTaskSource
    • Might make it easier to do memory pooling for task arrays..
  •  Sample
  • Docs

Benchmarks

image

@martinothamar
Copy link
Owner Author

Note to future self: Putting more of the code into Mediator/Mediator.Abstractions is good, easier to read and iterate on. Source generation is mostly important to avoid reflection, mapping opaque messages to concrete messages and resolving the correct handlers. I.e. once the concrete message is unwrapped and the underlying handlers are instantiated and ready, the benefits of source generation/compilation analysis are fully exhausted I think

@TimothyMakkison
Copy link
Contributor

TimothyMakkison commented Apr 1, 2024

Impressive changes, haven't done a major deep dive yet.

It might be possible to create a more memory efficient awaiter for known lengths. Task[] will allocate 24 + remaining length * 8 bytes, this means that in the common case that 2 notifications are asynchronous we'll have to allocate 40 bytes to store 2 references to Task values.

Instead Mediator could have methods for common array lengths where instead of having a nullable array we have task variables, one for each possibly asynchronous task. This way the array is inlined inside of the generated async state machine saving the 24 bytes for the array.

I think this will always use less memory than the array equivialent, up to 4 handlers. Thereafter unlikely setups may use slightly more memory.

async ValueTask Publish2(INotificationHandler[] handlers, TNotification notification, CancellationToken cancellationToken)
{
    Task task0 =  handler[0].Handle(notification, cancellationToken).AsTask();
    Task task1 =  handler[1].Handle(notification, cancellationToken).AsTask();

    List<Exception>? exceptions = null;

    try
    {
        await task0;
    }
    catch (Exception ex)
    {
        exceptions ??= new List<Exception>(1);
        exceptions.Add(ex);
    }
    // await each one etc
}

@martinothamar
Copy link
Owner Author

Took me a while to be able to benchmark this properly... But yes it makes a difference, atleast for the synchronous case

Top: before
Bottom: after

image

Now I just have to cleanup a bit before I can push... made some significant config changes to make it easier to benchmark and test different things based on compile time config

@TimothyMakkison
Copy link
Contributor

Thanks for sharing the benchmarks!

Looks like it saved a small amount of memory for the async case, although I can't tell why it saves 32 (35) bytes and not the expected 24. I'll look at the lowered code in the morning, benchmarkdotnet might be measuring memory usage incorrectly.

The synchronous case getting a speed boost is unexpected. Perhaps it's due to loop unrolling.

@martinothamar
Copy link
Owner Author

martinothamar commented Apr 2, 2024

Yeah when it's manually unrolled like that I'm guessing it can elide bounds checks as well, I kind of doubt it is able to do so for enumerators even if they are plain structs (not boxed, no virtual calls)

EDIT: nvm that was not the custom enumerator, codegen looks identical. I guess it just saves the control flow of the loop? branches and jumps, even if they are predictable

Updated the PR desc with a full run through benchmarks as well

@martinothamar
Copy link
Owner Author

Great points as always! Reduced the allocations in the asynchronous case.
I also made sync optimization for the foreach publisher, just by "branching inwards".
After extending NotificationHandlers<TNotification> to implement IEnumerable<> some inlining decisions changed and 2x'ed latency for some cases, so with this latest commit I'm a bit more careful about extracting the non-common branches into separate functions, seems to do the trick 😄
image

Also included sample and docs. I'm pretty happy with this now

Copy link
Contributor

@TimothyMakkison TimothyMakkison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked through most of it, LGTM. No idea what you did with ForeachAwaitPublisher 😅

I did think sorting with InheritanceComparer could cause big O issues (had a similar issue with Mapperly). But I don't think these issue will happen in practice.

@martinothamar
Copy link
Owner Author

True it's pretty naive, I guess I'm betting there aren't many inheritance hierarchies out there, atleast there isn't in my code :P

We can improve it in subsequent PRs, just want to get a preview out now so that I can test it some more

@martinothamar martinothamar merged commit 1858bbc into main Apr 7, 2024
7 checks passed
@martinothamar martinothamar deleted the feature/parallel-notifications branch April 7, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants