Do not create string objects from consumerTag, exchange and routingKey, or get them from a string cache

[zgabi]

Proposed Changes

See issue #1231

Types of Changes

What types of changes does your code introduce to this project?

• 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

[bollhals]

I'd prefer to split off at least the confirmTag into it's own PR. (as it contains a lot of special code to handle things, simpler to review track errors)

The other two are so similar that they can remain in this one.

As mentioned in the issue, I think we should look into also providing new API that take ROM for the exchangeName/RoutingKey

[zgabi]

@bollhals This is just a draft PR, which was created for the request of @bollhals
Of cource once your team decides how to implement it, (if it helps to you) I could make another (separated) PRs.
The comsumerTag and other chages are already separated to 2 commits.

[michaelklishin]

I do not have a strong opinion on whether this should be two separate PRs. However,

As mentioned in the issue, I think we should look into also providing new API that take ROM for the exchangeName/RoutingKey

would be nice and can be included into this PR or a separate one.

[lukebakken]

This seems like a reasonable way to reduce memory use.

[Gsantomaggio]

I improved similarly to the .NET stream client here, avoiding tons of allocations.

We need to test the PR deeply, but it makes sense to me.

The problem here is the breaking changes.

[zgabi]

@Gsantomaggio We are using this PR in our application since almost a year ago without any problem... Today I only updated it to the latest code. (nothing else changed in this PR)

[lukebakken]

@zgabi thank you! We appreciate you keeping this PR up-to-date. As you can see, we're making progress on version 7.0.

[zgabi]

@lukebakken Could you please run the build again, there was a problem in the APIApproval file

[lukebakken]

Well, it's going to take some time to resolve conflicts with the current state of main.

@zgabi - if you have time to start resolving conflicts, I'd appreciate it.

[lukebakken]

Thank you @zgabi!!!!

[lukebakken]

@zgabi thanks for all of your work on this. I'm planning to merge it by the end of the day tomorrow (16 May) unless there are further changes needed.

[lukebakken]

Please see this comment: #1231 (comment)

[lukebakken]

@zgabi @bollhals @michaelklishin take a look at this commit:

d4e88ca

I would like to replace the usage of ReadOnlyMemory<byte> and string, when used for exchange and queue names, and routing keys, with the dedicated types in that commit. Thoughts?

[zgabi]

Hi,

So the parameters of the of the HandleBasicDeliver will be alsoe ExchangeKey and RoutingKey?
For me it is ok.. I had a different branch/commit for this change (However they are using CachedStrings, but not so hard to separate them to the 2 new types):
https://github.com/zgabi/rabbitmq-dotnet-client/commits/routingkeyandexchange

This branch was an alternative to the "stringallocations" branch.

[lukebakken]

@zgabi I realize this PR now contains an enormous number of changes, but the vast majority of them are the following:

• Replace string with RoutingKey, QueueName, ExchangeName or ConsumerTag, as appropriate.
• Update APIs with the above

Here are the "big" changes -

• Consumer dispatchers now pass RentedMemory for the method and body part of a delivery, which prevents string allocations unless the handler explicitly triggers them in the AmqpString instance - https://github.com/rabbitmq/rabbitmq-dotnet-client/pull/1233/files#diff-74d6657893b833f7aefa9d2a615ae0d7dc1c4cfefa6615461c1b1638ff4b17abR53-R59
• This is one such place where casting an instance of an AmqpString subclass will result in a string allocation.

@zgabi if you wouldn't mind re-running your allocation tests that would be great. Or, let me know how you did them and I will run them. Thanks!

[bollhals]

While this change and the effort from you @lukebakken are honerable, I think it is currently way too big to properly analyze and judge the impact of the change. And also it contains many unrelated changes, making it even harder to judge.

My proposal would be to split off any unrelated changes, to pass them individually.

Regarding the main topic of the PR:

• With the introduction of these types, it's much clearer what you're suppose to pass as a parameter. But this comes at a tradeoff as well:
  • You always have to go through an indirection (the type) if you want / need it or not
  • You need to allocate this intermediate type, which slows you down if you do not plan on reusing it.

As it is right now, I think it is further away from what it tried to acheive than where the main is right now ... :/

IMHO, having classes wrapping string / bytes is only useful if

• You should be reusing them / they're not supposed to be allocated just for this one call
• The library has a cache to avoid the allocation of it (e.g. Avoid allocating the exchange string + ExchangeName object for the delivery of messages, as they're often the same over and over again.)

[bollhals]

Throwing exceptions on dispose is generally not expected behaviour. I'd challange this change.

[lukebakken]

What is the best option, then?

I have run into bizarre CI failures that can only be explained by the use of .GetAwaiter().GetResult() within Dispose().

[lukebakken]

This is one example, from running the entire testsuite on my local Windows 11 workstation. I have no idea how the operations here could have lead to the following error. Notice that, somehow, the test's DisposeAsync method (which calls CloseAsync) occurred at the same time as a BasicCancelAsync operation 🤔 Each operation would have had to acquire the same SemaphoreSlim for their RPC call (since the test was using the shared _channel instance when the error happened). This is also why I modified the tests to use dedicated IChannel instances per-Task:

rabbitmq-dotnet-client-1233-stacktrace.txt

[lukebakken]

I could look into bringing back a synchronous Abort to be used by Dispose in another PR (ugh)

[bollhals]

Generally you either do sync or async, which includes on Dispose you call sync methods not asyncs. Otherwise you'll implement IAsyncDisposable and do it there.

On the other hand, this.AbortAsync().GetAwaiter().GetResult(); Should be the way to go. It will block the current thread as a consequence, which might be problematic if

• The threadpool is maxed out, all threads are blocked (See Threadpool starvation)
• the thread has a synchronization context set and it deadlocks itself (Not entirely sure about the details here, rarely happens wihtout an UI AFAIK)

So I don't know exactly based on this why it happens without looking deep into it.

Sidenote: this would be a change I rather see separate from this PR, as I see the intent of enfocing the close as a different breaking change rather than it being burried in a large PR with a different goal in mind.

[lukebakken]

this would be a change I rather see separate from this PR, as I see the intent of enfocing the close as a different breaking change rather than it being burried in a large PR with a different goal in mind.

The thing is, if I move it into another PR, there is a good chance that this PR's CI runs will start failing again.

[bollhals]

Not if those are merged first ;)

But please, do it as you think it is best. This is just my personal opinion =)

[lukebakken]

Ah, yeah, good point!

[paulomorgado]

GetAwaiter().GetResult() per-se should not throw exceptions. Exceptions come from task being observed.

The exception in question seems to be coming from here

If you are thinking of providing synchronous APIs, they should not be really sync and not sync over async. If it's just for the sake of implementing IDisposable, implement just IAsyncDisposable.

[lukebakken]

Hi @bollhals thanks for your input.

The library has a cache to avoid the allocation of it (e.g. Avoid allocating the exchange string + ExchangeName object for the delivery of messages, as they're often the same over and over again.)

Well, that's exactly what this PR accomplishes with the AmqpString/ExchangeName types, so there's that at least 😄.

There is no cache, but upon delivery, ExchangeName only points to the backing ReadOnlyMemory<byte> of the incoming data. A string is only initialized from it if the user takes an action to do so. Once the delivery handler exits, the memory is released.

I will see about moving other changes to a different PR.

Actually, nearly all of the changes in this PR are due to the migration from string to dedicated types for exchange name etc, so I don't see the value in a separate PR.

[lukebakken]

IMHO, having classes wrapping string / bytes is only useful if

@bollhals there is another benefit - I have had RabbitMQ support cases that boil down to the wrong string passed as the wrong parameter to a library call. Having dedicated types makes that kind of mistake much less likely.

[bollhals]

IMHO, having classes wrapping string / bytes is only useful if

@bollhals there is another benefit - I have had RabbitMQ support cases that boil down to the wrong string passed as the wrong parameter to a library call. Having dedicated types makes that kind of mistake much less likely.

Sure, that's what I ment by stating to make it much clearer what to pass.
But the tradeoff is the allocation of the objects and the indirection through another object.

It depends in the end what the goal is:

• Increasing performance? => I have a feeling that these additional objects & indirection made it slower for most real world usecases. (Perf tests needed to confirm)
• Increasing usability? => Fine, this surely helps, but then the PR Title should be changed, as the goal of not allocating string objects becomes a side thing.

[lukebakken]

I'll run some performance tests. I don't think I have access to memory profiling tools to see how this PR affects allocations ... @zgabi ?

[lukebakken]

@zgabi @bollhals I have force-reset the stringallocations branch to 2320480, which is just before I went crazy with this PR. I saved my work on another branch.

[paulomorgado]

• Increasing performance? => I have a feeling that these additional objects & indirection made it slower for most real world usecases. (Perf tests needed to confirm)

Sometimes, minimal local extra work/complexity avoids extra processing, allocations, GC work. It really needs to be measured.