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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 10 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/syncstream:288
+inline _LIBCPP_INLINE_VISIBILITY
+basic_syncbuf<_CharT, _Traits, _Allocator>::basic_syncbuf(basic_syncbuf&& __other)
+    : __wrapped(_VSTD::move(__other.get_wrapped())),
----------------
joe.sylve wrote:
> This move constructor will lose the existing buffered contents.  Is this intended?  I'd expect that the __str buffer would also be moved and the base class's pointers adjusted.
This comment seems no longer be attached to its original location so not sure what you're referring to. I have noticed several issues with the move assignment and constructor.


================
Comment at: libcxx/include/syncstream:220
+{
+    mutex_ptr_type __mtx = __mtx_ptr_map[__obuf];
+
----------------
mclow.lists wrote:
> zoecarver wrote:
> > zoecarver wrote:
> > > mclow.lists wrote:
> > > > How do entries get removed from `__mtx_ptr_map`?
> > > > 
> > > They don't. I don't think there is any way to say "will any threads ever need to access this streambuf again?" 
> > > 
> > > If there was a way to only keep the streambuf in the static map while the object is active, I would be open to that. But, I am hesitant to do that because I want to keep the read/writes to the map as low as possible. 
> > > 
> > > What would you suggest? 
> > Given the "new" structure of `__mtx_ptr_map`, I think I can add a function to remove elements from the map safely. Would this be a good thing to add (the only reason it might not is for performance but, I think stale memory outweighs performance here)?
> You could store a count, like shared_ptr does.  How many sync streams are using this streambuf?
> 
> Increment the count on construction, decrement on destruction, remove at zero.
I implemented this part and indeed use a count to. Not cleaning up sound like a memory leak which might affect long running applications that create a lot syncbufs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67086