Bug 231171 – Bind the number of WebRTC sockets opened per process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Bind the number of WebRTC sockets opened per process as a healthy measure.

[youenn fablet]

<rdar://problem/83610575>

[youenn fablet]

Created attachment 440064 [details]
Patch

[Alex Christensen]

Comment on attachment 440064 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440064&action=review

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.h:156
> +    StdMap<WebCore::LibWebRTCSocketIdentifier, std::unique_ptr<Socket>, SocketComparator> m_sockets;

Can we use ListHashSet instead?

[youenn fablet]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 440064 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440064&action=review
> 
> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.h:156
> > +    StdMap<WebCore::LibWebRTCSocketIdentifier, std::unique_ptr<Socket>, SocketComparator> m_sockets;
> 
> Can we use ListHashSet instead?

We could, I think StdMap is closer to what we want: a map (we very often query the map by ID) which happens to have a key order (useful to close the socket with a given order).

[youenn fablet]

Comment on attachment 440064 [details]
Patch

Marking it as r? Alex, let me know if you still prefer to use ListHashSet (and why), I can migrate to it pretty easily in any case.

[Alex Christensen]

Comment on attachment 440064 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440064&action=review

>>> Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.h:156
>>> +    StdMap<WebCore::LibWebRTCSocketIdentifier, std::unique_ptr<Socket>, SocketComparator> m_sockets;
>> 
>> Can we use ListHashSet instead?
> 
> We could, I think StdMap is closer to what we want: a map (we very often query the map by ID) which happens to have a key order (useful to close the socket with a given order).

I think ListHashSet also does what you want and it uses fastMalloc.  StdMap seems to only be used when we need to allow all possible keys (including 0 and -1) but that isn't the case here.

[youenn fablet]

(In reply to Alex Christensen from comment #6)
> Comment on attachment 440064 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440064&action=review
> 
> >>> Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.h:156
> >>> +    StdMap<WebCore::LibWebRTCSocketIdentifier, std::unique_ptr<Socket>, SocketComparator> m_sockets;
> >> 
> >> Can we use ListHashSet instead?
> > 
> > We could, I think StdMap is closer to what we want: a map (we very often query the map by ID) which happens to have a key order (useful to close the socket with a given order).
> 
> I think ListHashSet also does what you want and it uses fastMalloc.  StdMap
> seems to only be used when we need to allow all possible keys (including 0
> and -1) but that isn't the case here.

ListHashSet in that case would require to use a std::pair, provide a specific Hash...
StdMap is using FastAllocator so we should be good.

[EWS]

Patch 440064 does not build

[youenn fablet]

Created attachment 440588 [details]
Rebasing

[EWS]

Committed r283797 (242690@main): <https://commits.webkit.org/242690@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440588 [details].

[Geoffrey Garen]

> ListHashSet in that case would require to use a std::pair, provide a
> specific Hash...
> StdMap is using FastAllocator so we should be good.

But you wrote a custom comparator for StdMap! :P

An additional benefit of ListHashSet is that you don't have to allocate each bucket separately, which StdMap still will.

Anyway, this is not perf sensitive code, so it is fine -- just trying to align our thinking a bit more for future.

[Darin Adler]

Comment on attachment 440064 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440064&action=review

>>>>> Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.h:156
>>>>> +    StdMap<WebCore::LibWebRTCSocketIdentifier, std::unique_ptr<Socket>, SocketComparator> m_sockets;
>>>> 
>>>> Can we use ListHashSet instead?
>>> 
>>> We could, I think StdMap is closer to what we want: a map (we very often query the map by ID) which happens to have a key order (useful to close the socket with a given order).
>> 
>> I think ListHashSet also does what you want and it uses fastMalloc.  StdMap seems to only be used when we need to allow all possible keys (including 0 and -1) but that isn't the case here.
> 
> ListHashSet in that case would require to use a std::pair, provide a specific Hash...
> StdMap is using FastAllocator so we should be good.

Alex, I don’t think your concept expressed here for when we’d use std::map is quote right. You said we would use it when we need to allow all possible keys. If the issue is truly only that we need to use a scalar key but allow all possible keys, we can solve that easily with HashMap, using a key that is a structure that contains both the scalar and an empty/deleted indicator, and it should still be quite efficient. I think it’s a mistake to dump hash tables entirely just because our hash table implementation is optimized for cases where we can reserve an empty and deleted value.

I would suggest using std::map, a tree rather than a hash table, when we need an ordered collection, ordered by the keys. That’s something a tree does well but a hash table does not.

I am not so sure std::map here is great. Does indeed sound like the kind of thing we use ListHashSet for, but I guess we made the "ergonomics" of that too inelegant for a case like this.