Bind the number of WebRTC sockets opened per process as a healthy measure.
<rdar://problem/83610575>
Created attachment 440064 [details] Patch
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?
(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).
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.
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.
(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.
Patch 440064 does not build
Created attachment 440588 [details] Rebasing
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].
> 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.
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.