[libcxx-commits] [PATCH] D67086: Implement syncstream (p0053)

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked an inline comment as done.
zoecarver added inline comments.


================
Comment at: libcxx/include/syncstream:208
+template <class _CharT, class _Traits, class _Allocator>
+_VSTD::unordered_map<
+    typename basic_syncbuf<_CharT, _Traits, _Allocator>::streambuf_type*,
----------------
mclow.lists wrote:
> zoecarver wrote:
> > mclow.lists wrote:
> > > Why `unordered_map` as opposed to `map`? Does `std::hash< streambuf_type*>` have some behavior that you want?  [ Note: `unordered_map` might be the right choice. But you should say //why// it is the right choice somewhere. ]
> > The pointer specialization of `std::hash` should help with memory usage. Unordered map has a find complexity of O(1) which is the real reason I think it should be used. This stream should be CPU performant above all else, with hundreds of threads trying to access an element, I think the benefit of find speed outweighs the slight increase in memory consumption. Do you agree?
> > The pointer specialization of std::hash should help with memory usage. 
> 
> How?
> 
> >Unordered map has a find complexity of O(1) which is the real reason I think it should be used.
> Unordered map has a find complexity of O(1) in the best case. O(N) in the worst case. Which one this?
> Unordered map has a find complexity of O(1) in the best case. 

Sorry, yes, that is what I meant. I suspect that I will be closer to O(1) because there won't be very many elements. 

I ran some basic benchmarks and came up with very similar results. `std::unordered_map` seems to be roughly 150ms slower (total time 16675ms vs 16521ms). Given this, I will update to use `std::map` to shrink the memory footprint. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67086/new/

https://reviews.llvm.org/D67086