getting rid of a Func allocation

[bollhals]

Proposed Changes

The AsyncConsumerWorkService was allocating a new Func for each call to Schedule (Also spotted in the images of #824 due to this roslyn bug

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

I haven't verified these changes myself, as I just stumbled upon this while looking for some other answer and I quickly edited it in Github only.

[bollhals]

You learn something new every day, so you can't field assign a lambda function that calls an instance method. :)
Moved the initialization to the new ctor and also moved the _workPool down there to keep it at one place.

[lukebakken]

Speaking of, read the "Remarks" here -

https://docs.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentdictionary-2.getoradd?view=netcore-3.1

[bollhals]

True, that's actually a new bug that you've fixed as well. 👍
If it is on the hotpath the question is, how "bad" is the lock for all the gets as we'd only need it really for the add case. Are there alternatives?

[bording]

@lukebakken Taking a quick look at this, I'm not sure that remark is really relevant here. Seems like It would only matter if we expect the same model instance to be starting work pools from different threads.

We definitely should be avoiding adding a lock in this path, but it's not clear to me that we need it.

[lukebakken]

OK, I'll take your word for it @bording ... I just traced the code a bit and it made my eyes cross. Only connections are supposed to be thread-safe so people abusing IModel instances are warned.

[lukebakken]

Of course, the question is why is a ConcurrentDictionary being used in the first place 🤔

[bording]

OK, I'll take your word for it @bording ... I just traced the code a bit and it made my eyes cross. Only connections are supposed to be thread-safe so people abusing IModel instances are warned.

This was just a quick look, so don't take what I said as gospel! I'm just trying to think of how you'd have the same model instance calling Schedule from multuple threads in a valid scenario, and I didn't immediately come up with one.

[bording]

Of course, the question is why is a ConcurrentDictionary being used in the first place 🤔

We do need to protect against multiple models adding work at the same time and writing to a regular dictionary from multiple threads would require a lock to avoid corrupting the collection.

[lukebakken]

You can see why that is confusing... "No need for a lock here, but we do in this other place in the same class"

[bording]

It seems like we could look at just removing a shared work service as a concept and let each model have its own work pool, removing the need for a collection at all.

[bording]

You can see why that is confusing... "No need for a lock here, but we do in this other place in the same class"

Where is there a lock already?

[lukebakken]

Where is there a lock already?

The use of a ConcurrentDictionary.

I'll add the per-model work pools as a future idea.

[michaelklishin]

So we removed some allocations but introduced a lock. @stebet can you please run your profiling workload and share if this is a net positive change in terms of CPU usage and allocations?

[lukebakken]

So we removed some allocations but introduced a lock

The lock is already gone!

[bording]

The use of a ConcurrentDictionary.

Hmm, I wouldn't have classified that as a lock, but I see what you mean.

The distinction here is that we need a ConcurrentDictionary because the collection itself could be modified concurrently, where the GetOrAdd issue is about adding the same key to that collection from separate threads, and if the func being called more than once is a problem.

In this case, we'd end up creating more than one work pool, only one of which would be stored in the collection. The other one would never have work queued to it, so it doesn't seem like it could interfere in any way. It would await forever for the dequeue to return something.

[lukebakken]

In this case, we'd end up creating more than one work pool, only one of which would be stored in the collection. The other one would never have work queued to it, so it doesn't seem like it could interfere in any way. It would await forever for the dequeue to return something.

Would the other work pool be leaked or, since there would be no reference to it, would it be cleaned up? My guess is it would be leaked, or the resources used by it leaked... emphasis on the word "guess" 🤷

[stebet]

Yeah, I've wondered about this code a few times. I like @bording suggestion to just have each Model keep it's own work queue. I'll run the PR through the profiling tool now though.

[stebet]

This does indeed work and ends up in a reduction. Good spot there @bollhals. It wasn't apparent to me where that sneaky Func<> allocation was coming from.

Before:

After:

[stebet]

And on that note, I was thinking for 7.0 that it'd be a good idea to get rid of these dispatchers, and just have dedicated Channel instances on the models that an async task would simply be reading from. That way the message deliveries would always run asynchronously and there is no need for a collection to keep track of these instances.

[lukebakken]

Thanks @bollhals and everyone else. Interesting discussion!