8.0: improve eventing #986

bollhals · 2020-12-13T22:50:39Z

Proposed Changes

Changes the eventing used in this lib to use the new EventingWrapper. The main benefit is that the delegates are cached after the first invocation.

Types of Changes

Bug fix (non-breaking change which fixes issue #NNNN)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause an observable behavior change in existing systems)
Documentation improvements (corrections, new content, etc)
Cosmetic change (whitespace, formatting, etc)

Checklist

I have read the CONTRIBUTING.md document
I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
All tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)
Any dependent changes have been merged and published in related repositories

Further Comments

Local output of new benchmarks

Eventing_AddRemove:

Method	Job	Runtime	count	Mean	Error	StdDev	Median	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated	Code Size
Event	Job-TXURZZ	.NET 4.8	1	38.31 ns	0.107 ns	0.100 ns	38.31 ns	1.00	0.00	-	-	-	-	258 B
Wrapper	Job-TXURZZ	.NET 4.8	1	38.82 ns	0.148 ns	0.138 ns	38.76 ns	1.01	0.00	-	-	-	-	1391 B
List	Job-TXURZZ	.NET 4.8	1	40.95 ns	0.191 ns	0.179 ns	40.95 ns	1.07	0.01	0.0153	-	-	72 B	713 B

Event	Job-ECJYBK	.NET Core 3.1	1	28.59 ns	0.172 ns	0.160 ns	28.65 ns	1.00	0.00	-	-	-	-	259 B
Wrapper	Job-ECJYBK	.NET Core 3.1	1	27.60 ns	0.087 ns	0.081 ns	27.56 ns	0.97	0.01	-	-	-	-	394 B
List	Job-ECJYBK	.NET Core 3.1	1	28.52 ns	0.581 ns	0.938 ns	28.00 ns	1.02	0.04	0.0136	-	-	64 B	671 B

Event	Job-TXURZZ	.NET 4.8	5	589.66 ns	5.712 ns	5.343 ns	589.28 ns	1.00	0.00	0.1659	-	-	786 B	258 B
Wrapper	Job-TXURZZ	.NET 4.8	5	609.88 ns	11.078 ns	10.363 ns	610.47 ns	1.03	0.02	0.1659	-	-	786 B	1391 B
List	Job-TXURZZ	.NET 4.8	5	261.28 ns	1.068 ns	0.999 ns	261.12 ns	0.44	0.00	0.0544	-	-	257 B	713 B

Event	Job-ECJYBK	.NET Core 3.1	5	546.30 ns	2.606 ns	2.176 ns	547.02 ns	1.00	0.00	0.1659	-	-	784 B	259 B
Wrapper	Job-ECJYBK	.NET Core 3.1	5	516.55 ns	3.137 ns	2.934 ns	515.22 ns	0.95	0.01	0.1659	-	-	784 B	1562 B
List	Job-ECJYBK	.NET Core 3.1	5	212.80 ns	0.572 ns	0.535 ns	212.80 ns	0.39	0.00	0.0527	-	-	248 B	669 B

Eventing_Invoke:

Method	Job	Runtime	Length	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated	Code Size
SingleInvoke	Job-UDUQAG	.NET 4.8	1	1.011 ns	0.0028 ns	0.0022 ns	1.00	0.00	-	-	-	-	57 B
GetInvocationList	Job-UDUQAG	.NET 4.8	1	14.499 ns	0.1101 ns	0.1029 ns	14.32	0.11	0.0068	-	-	32 B	501 B
List	Job-UDUQAG	.NET 4.8	1	13.284 ns	0.0530 ns	0.0496 ns	13.15	0.05	-	-	-	-	325 B
Wrapper	Job-UDUQAG	.NET 4.8	1	3.716 ns	0.0078 ns	0.0065 ns	3.68	0.01	-	-	-	-	322 B

SingleInvoke	Job-KHEQLR	.NET Core 3.1	1	1.207 ns	0.0043 ns	0.0036 ns	1.00	0.00	-	-	-	-	37 B
GetInvocationList	Job-KHEQLR	.NET Core 3.1	1	15.319 ns	0.1321 ns	0.1235 ns	12.71	0.11	0.0068	-	-	32 B	488 B
List	Job-KHEQLR	.NET Core 3.1	1	7.838 ns	0.0236 ns	0.0221 ns	6.49	0.03	-	-	-	-	310 B
Wrapper	Job-KHEQLR	.NET Core 3.1	1	3.611 ns	0.0200 ns	0.0187 ns	2.99	0.02	-	-	-	-	308 B

SingleInvoke	Job-UDUQAG	.NET 4.8	5	10.845 ns	0.0726 ns	0.0643 ns	1.00	0.00	-	-	-	-	57 B
GetInvocationList	Job-UDUQAG	.NET 4.8	5	64.140 ns	0.2262 ns	0.2116 ns	5.91	0.04	0.0136	-	-	64 B	501 B
List	Job-UDUQAG	.NET 4.8	5	32.994 ns	0.1074 ns	0.1005 ns	3.04	0.02	-	-	-	-	325 B
Wrapper	Job-UDUQAG	.NET 4.8	5	12.765 ns	0.0529 ns	0.0495 ns	1.18	0.01	-	-	-	-	322 B

SingleInvoke	Job-KHEQLR	.NET Core 3.1	5	15.334 ns	0.0359 ns	0.0319 ns	1.00	0.00	-	-	-	-	37 B
GetInvocationList	Job-KHEQLR	.NET Core 3.1	5	66.992 ns	0.3860 ns	0.3611 ns	4.37	0.03	0.0136	-	-	64 B	488 B
List	Job-KHEQLR	.NET Core 3.1	5	26.444 ns	0.5399 ns	0.7390 ns	1.75	0.05	-	-	-	-	310 B
Wrapper	Job-KHEQLR	.NET Core 3.1	5	11.851 ns	0.0465 ns	0.0412 ns	0.77	0.00	-	-	-	-	308 B

stebet · 2020-12-14T17:28:31Z

Looks good. Have yet to read a bit more in-depth. Would be good to add a test that hammers add/remove events while also invoking them to see if any race conditions happen due to the caching. Impressive speed boost :) this should be back-portable to 6.2 right?

bollhals · 2020-12-14T20:25:08Z

projects/RabbitMQ.Client/client/events/AsyncEventHandler.cs

 using System.Threading.Tasks;

 namespace RabbitMQ.Client.Events
 {
-    public delegate Task AsyncEventHandler<in TEvent>(object sender, TEvent @event) where TEvent : EventArgs;


This is a public API change, but it slips through the APIApproval test. 🙄

(I removed it as the EventHandler doesn't have it either (used to have it before 4.0 I think))
Also removing should be "fine" as it only makes it less strict => no recompilation errors AFAIK

Since this is for 7.0, we can certainly make this change.

If we want to backport it to 6.x and this is an issue, one could add it back (and to AsyncEventingWrapper) for 6.x

bollhals · 2020-12-14T20:35:27Z

Looks good. Have yet to read a bit more in-depth. Would be good to add a test that hammers add/remove events while also invoking them to see if any race conditions happen due to the caching.

Valid point. Question is how could we set up a test for this that actually is verifyable? I'll think about it.

Impressive speed boost :) this should be back-portable to 6.2 right?

Yes, no public API (except see single comment) was changed.

Also might worth mentioning here:

This pr gets rid of delegate allocation for the "stebet testcase Program.cs" for the AsyncEventHandler<BasicDeliverEventArgs> Received
This pr doesn't solve 8.0 Discussion about public events #973 but rather simplifies the current handling of the events. (Result of 8.0 Discussion about public events #973 might be that this is enough)

bollhals · 2020-12-14T22:30:55Z

Looks good. Have yet to read a bit more in-depth. Would be good to add a test that hammers add/remove events while also invoking them to see if any race conditions happen due to the caching.

Valid point. Question is how could we set up a test for this that actually is verifyable? I'll think about it.

I've added a test case about adding event handlers in a concurrent environment and testing whether all of them are called. 👍

bollhals · 2020-12-19T13:28:32Z

Can this be merged?

Regarding backport to 6.x and the removal of where TEvent : EventArgs; in AsyncEventHandler

If we want to backport it to 6.x and this is an issue, one could add it back (and to AsyncEventingWrapper) for 6.x

michaelklishin · 2020-12-24T05:31:38Z

This also needs a separate version for 7.x, specifically I could not backport it because of

projects/RabbitMQ.Client/client/impl/EventingWrapper.cs(7,6): error CS8370: Feature 'nullable reference types' is not available in C# 7.3. Please use language version 8.0 or greater.

so we need to decide to bump the target language version. This means requiring .NET Core 3.x and Standard 2.1 which could be reasonable for 7.x.

bollhals · 2020-12-24T21:59:30Z

This also needs a separate version for 7.x, specifically I could not backport it because of
projects/RabbitMQ.Client/client/impl/EventingWrapper.cs(7,6): error CS8370: Feature 'nullable reference types' is not available in C# 7.3. Please use language version 8.0 or greater.
so we need to decide to bump the target language version. This means requiring .NET Core 3.x and Standard 2.1 which could be reasonable for 7.x.

We can also just get rid of the nullable reference types feature for the backport. It's not a necessity.

michaelklishin · 2020-12-25T00:34:28Z

That works for me, I suspect that earlier master changes will have to be backported or we will keep making every master PR a special case for 7.x.

bollhals · 2020-12-25T00:50:09Z

That works for me, I suspect that earlier master changes will have to be backported or we will keep making every master PR a special case for 7.x.

As said in the other PR =>

I'm wondering is there any change on master that is really 8.0 only? Or couldn't we merge master to 7.0?

bollhals · 2021-01-07T23:31:16Z

That works for me, I suspect that earlier master changes will have to be backported or we will keep making every master PR a special case for 7.x.

As said in the other PR =>

I'm wondering is there any change on master that is really 8.0 only? Or couldn't we merge master to 7.0?

@michaelklishin Any answer for this?
Creating individual merges for 7.0 would be quite some work, so I'd rather only spend it if it is really needed.

michaelklishin · 2021-01-08T16:36:08Z

Since we haven't found any obvious improvements for #959 to ship in 7.0, we can either scrap the milestone entirely or merge master into 7.0. Then a lot of PRs would have to be renamed and their milestone would have to be changed.

michaelklishin · 2021-01-08T16:37:51Z

I personally vote for shipping 8.0 and skipping 7.0 since #959 wasn't a significant (or straightforward) enough problem to spark enough interest for development of new API elements for 7.0.

bollhals · 2021-01-08T21:06:19Z

I was under the impression 7.0 was for smaller interface changes like #959 and 8.0 was the full async client. I'd argue that all changes on master so far still qualifies as "smaller interface changes" and thus should be released with 7.0.

If we manage to fix 959 as part of it, the better, otherwise it will be fixed by the full async client for sure in 8.0. I had another idea today while I thought about it again, so if successful, I may draft a PR for it.

michaelklishin · 2021-01-11T13:26:37Z

We can change the milestone for master to be 7.0. I will ask for feedback in a separate issue.

change eventing

6f1ce6b

bollhals commented Dec 14, 2020

View reviewed changes

bollhals added 2 commits December 14, 2020 23:29

adding test case 'ConcurrentEventingTestForReceived'

93d3bd8

fix Empty Body returning buffer issue

20b14df

Merge branch 'master' into feature/eventing

84e1332

michaelklishin approved these changes Dec 24, 2020

View reviewed changes

michaelklishin merged commit 46c06eb into rabbitmq:master Dec 24, 2020

michaelklishin added this to the 8.0.0 milestone Dec 24, 2020

michaelklishin changed the title ~~7.0 - improve eventing~~ 8.0: mprove eventing Dec 24, 2020

michaelklishin changed the title ~~8.0: mprove eventing~~ 8.0: improve eventing Dec 24, 2020

bollhals mentioned this pull request Jan 8, 2021

Consumer dispatcher improvements #997

Merged

11 tasks

lukebakken modified the milestones: 8.0.0, 7.0.0 Mar 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8.0: improve eventing #986

8.0: improve eventing #986

bollhals commented Dec 13, 2020

stebet commented Dec 14, 2020

bollhals Dec 14, 2020

michaelklishin Dec 14, 2020

bollhals Dec 15, 2020

bollhals commented Dec 14, 2020

bollhals commented Dec 14, 2020

bollhals commented Dec 19, 2020

michaelklishin commented Dec 24, 2020

bollhals commented Dec 24, 2020

michaelklishin commented Dec 25, 2020

bollhals commented Dec 25, 2020

bollhals commented Jan 7, 2021

michaelklishin commented Jan 8, 2021

michaelklishin commented Jan 8, 2021

bollhals commented Jan 8, 2021

michaelklishin commented Jan 11, 2021

8.0: improve eventing #986

8.0: improve eventing #986

Conversation

bollhals commented Dec 13, 2020

Proposed Changes

Types of Changes

Checklist

Further Comments

stebet commented Dec 14, 2020

bollhals Dec 14, 2020

Choose a reason for hiding this comment

michaelklishin Dec 14, 2020

Choose a reason for hiding this comment

bollhals Dec 15, 2020

Choose a reason for hiding this comment

bollhals commented Dec 14, 2020

bollhals commented Dec 14, 2020

bollhals commented Dec 19, 2020

michaelklishin commented Dec 24, 2020

bollhals commented Dec 24, 2020

michaelklishin commented Dec 25, 2020

bollhals commented Dec 25, 2020

bollhals commented Jan 7, 2021

michaelklishin commented Jan 8, 2021

michaelklishin commented Jan 8, 2021

bollhals commented Jan 8, 2021

michaelklishin commented Jan 11, 2021