Callback manager in SignalR C++ client should use ints instead of strings for ids

[mprather]

Is there an existing issue for this?

• I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

The callback id managed by the callback_manager class tracks info by converting an int into a string. This seems like a less than optimal way to manage resources. That's a string allocation and overhead that could be better managed if the id remained as int.

Describe the solution you'd like

In order to reduce the impact of string allocations and other string-related slowdowns, is it possible to convert the callback manager to use ints instead of strings?

Additional context

No response

[BrennanConroy]

The invocation ID has to be a string per spec https://github.com/dotnet/aspnetcore/blob/main/src/SignalR/docs/specs/HubProtocol.md#invocations

Even if we store it as an int in the callback_manager we're going to have to convert it to a string at some point and then convert a string from the server to an int when we get the result. I don't see what we would save by storing the int instead of the string.

Are you actually seeing performance issues specifically related to this string allocation? I would be very surprised if you were.

[mprather]

@BrennanConroy I don't have specific metrics to show. I was taking a deeper look to understand a bug that we've encountered and I just ran across the callback id. However, in general, we have done a lot of performance testing of our application and the impact of using the c++ signalr library in the past.

Most of our installations are on small boards (rpi 3b+ or smaller, all the way down to a tiny 454Mz/64Mb). On those boards, we're limited in both CPU and mem. It's worth noting that the RPi 3B+ is the smallest board we've attempted to use signalr on.

Our implementations tend to process data up to speeds of 300Hz. Our mem footprint and cpu impact is typically rather small (in the single digits). However, when we start using signalr on a rpi 3b+, cpu usage ramps up and becomes the big consumer by several factors. We've taken a lot of steps to reduce the number of allocations before we hand anything over to the signalr api. We also only send messages when something has changed (i.e. if we're monitoring 125 states and only 2 change, then we send those changes -- for each timeslice). The goal is to provide data to a site so that other can watch what is happening in the lab/robots/etc.

In general, though, it just feels like signalr is not as well-tuned as we had hoped. On a recent application, we had to downgrade our processing from 100Hz to 10Hz so that signalr would work on the board. Otherwise, the cpu was clearly overloaded and everything would be impacted.

Thus, when I see something like a string-based key, it makes me wonder -- why? I get that the server is going to send it to as a string or vice-versa, but until it's actually needed as a string, wouldn't the best course of action be to leave it as an int (for lookups, etc)? Temp string is created, then copied, then another is created based on that string, etc. Also, string lookups/finds/comparisons are also pretty expensive. Between get_callback_id() and where the key is stored, there are at least 6 allocations (3 in get_callback_id and 3 more with the insert). If that key had been an int, there would be only a single allocation for the pair insertion.

Are there other places where the api can be smarter about what is handling and how? Those are the types of questions we ask of our own code.

We really like signalr from the web server side of things and not once has signalr's perf impact on the web server has come into question (it's noise on a web server with beefy everything) but getting data to the web server from a small system is key. If the api is not careful with what it's doing, then it will over-consume and make it harder to justify its use.

[BrennanConroy]

Thanks for sharing your scenario, it's nice to see where you're coming from.

We haven't done any real perf work for the C++ client yet, most of it has been driven by "don't do obviously bad things" and the logging issue you opened a while back. And we're still mostly focused on getting the functionality up to par with the other client and ironing out bugs.

We can easily remove 1 of the temporary string allocations, and we'll do that at a minimum. I'm still trying to convince myself that storing an int is ok from a protocol correctness standpoint. It might be since it's only used for sending from the client. And future changes to support receiving invocation IDs from the server could use a different callback manager type.

[ghost]

Thanks for contacting us.
We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[ghost]

Thanks for contacting us.
We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.