srtp_stream_hash_t

[jmillan]

Store SRTP streams in several lists rather than in a single one, making those effectively smaller (and faster to iterate) than the current single one.

Each hash entry is a srtp_stream_list_t, being this one where the streams are ultimately stored as they are currently. In order to select the srtp_stream_list_t where a stream is stored, the SSRC is masked with the hash size (the number of lists it contains).

Enhances greatly the performance derived from iterating the lists as the list to be iterated is, in average, N times smaller comparing to having a single one. N being the number of lists in the hash, which in this PR has been set to 32.

I'm aware of #612, and I think what I'm doing here cannot be done by rewriting the _list methods from the outside. These changes perfectly fit with the mentioned PR anyway.

EDIT:

• It adds little to no performance penalty as, in order to know in which list a stream is stored, it only performs a binary mask operation.
• It adds little to no performance gain to scenarios with a low number of streams/SSRCs.
• It adds a huge performance gain to scenarios with a high number of streams/SSRCs. 32 times faster in average with the current defined constant SRTP_STREAM_HASH_SIZE 32.

[nils-ohlmeier]

I like the approach of this PR as it appears to give a big improvement for server side usage, with very little to almost no downside for usage on endpoints with low number of streams.
If I'm not mistaken we are planing on using this in Mediasoup to improve performance, right @jmillan?

@fluffy @bifurcation @pabuhler what do you think?

[jmillan]

I like the approach of this PR as it appears to give a big improvement for server side usage, with very little to almost no downside for usage on endpoints

Both server and endpoints handling large number of streams benefit from this improvement in the same way. This improvement applies to both.

BTW, the number of buckets in the hash table is hardcoded to 32, which I find a reasonable number (creates 32 lists per hash table, 32 times smaller each, in average). I can make this number configurable if needed.

[pabuhler]

performance boosts are always good but I am a little confused, in the srtp code you have basically just replace all calls to xx_list with xx_hash, does't this mean in practice that you could have written you own list implementation that has this hashing functionality inside ? Maybe there is something I am missing, and maybe you could not use libsrtp list inside you hash list but would have to make your own.
Also this is fast when ssrc's are randomly distributed, but I have seen cases where they are not.

[jmillan]

does't this mean in practice that you could have written you own list implementation that has this hashing functionality inside ?

I have not implemented a list. This PR basically creates N (32 concretely) lists and handles them via a hash. I definitely don't know how could I have done it outside the library by rewriting the _list methods. Would you tell me?

This is, this PR provides enhancement also for user created lists since it basically reduces the length of them.

Also this is fast when ssrc's are randomly distributed, but I have seen cases where they are not.

Of course the ssrc numbering nature will determine how streams are distributed among lists. We cannot guarantee an even number of streams in every list of the hash, but that does not invalidate this enhancement IMO.

EDIT: I think this change would benefit the overall libsrtp usages and hence would make sense to have it inside the library. If it's found it does not, no problem, I'll try to implement it by rewriting the _list methods.

[jmillan]

The srtp_driver test is failing because it does not define the _hash functions.

If the hash functions were to be part of the base library then they should not be rewritable and need to be defined somewhere else.

I'll wait for confirmation on whether there is interest on having this inside libsrtp before doing any other change.

[jmillan]

This PR is ready for review. GitHub actions should all succeed.

_hash functions are internal. The purpose of holding streams in multiple lists rather than in a single one is to make those lists shorter and hence faster to traverse.

Definition and implementation of _list functions are untouched, and downstreams can override them as before.

Do you need anything from my side that I can provide to consider this change?

[pabuhler]

@jmillan, Will try to get some other opinions, but I am not against using a hash for this to improve performance.
I have some concerns regarding changes in existing behavior, both with regard to memory and number of lists created.
The default value of 32 maybe good for speed but in the case where there are not many ssrc's (people can use this differently) there will be a 190 byte, (32 * (4 + 4)), increase in memory for each session created and if someone has already provided their own list implementation then it could be even more if their list uses more than 4 bytes.
Maybe need to be able to configure this value.

[jmillan]

Thanks @pabuhler,

Sure, I can make this configurable if needed.

[jmillan]

Should this option be build time and follow the same behaviour as others like USE_EXTERNAL_CRYPTO?

[pabuhler]

Hi @jmillan, I have talked with some others on the team and the consensus is that we are unsure if this the right approach. Having a faster lookup using a hash or some other data structure is a good idea but using the approach of having multiple list per session adds an unnecessary layer. It would be preferable to replace the current internal list implementation with something faster and flexible but that there is still one list per session.
So I think there are 3 options;

• you replace the srtp list in your own application with your hash based multi list using the existing compile time mechanisms
• you create multiple srtp session's and place them in a hash map out side of libsrtp
• work with us to replace the internal list implementation with a faster structure (this we will try to do no matter what at some point, maybe reusing some of this PR if that is ok)

This was maybe not the answer you where looking for, and I apologize for taking some time to gather feedback.

[jmillan]

Hi @pabuhler,

This is the best option I could think of. Yes, it adds an extra layer which IMHO is so small it can't even be measured, as it makes a single binary mask on two unsigned integers to get the list where the stream is stored: stream->ssrc & (SRTP_STREAM_HASH_SIZE - 1).

you replace the srtp list in your own application with your hash based multi list using the existing compile time mechanisms

I think I'll go this way for the time being. I think I'll have to fork libsrtp anyway as I need to modify meson.build (which is what we use) to add the new file, with the hash implementation on the sources to compile.

work with us to replace the internal list implementation with a faster structure (this we will try to do no matter what at some point, maybe reusing some of this PR if that is ok)

This was my best shot, I don't know about a better approach at the moment, but don't hesitate to contact me anytime.

This was maybe not the answer you where looking for, and I apologize for taking some time to gather feedback.

All is good!

[jmillan]

Closing as this approach was rejected by the project.

Also, this is replaced by a hopefully more suitable solution #674