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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/syncstream:206
+
+  [[nodiscard]] static _LIBCPP_HIDE_FROM_ABI __wrapped_streambuf_mutex& __instance() noexcept {
+    static __wrapped_streambuf_mutex __result;
----------------
ldionne wrote:
> It *might* be possible to use a bit in the `locale` to implement a lock for all streambufs. Something like
> 
> ```
> diff --git a/libcxx/include/__locale b/libcxx/include/__locale
> index ccac748c44e4..4942808ac2bc 100644
> --- a/libcxx/include/__locale
> +++ b/libcxx/include/__locale
> @@ -131,9 +131,11 @@ private:
>      static locale& __global();
>      bool has_facet(id&) const;
>      const facet* use_facet(id&) const;
> +};
>  
> -    template <class _Facet> friend bool has_facet(const locale&)  _NOEXCEPT;
> -    template <class _Facet> friend const _Facet& use_facet(const locale&);
> +template <>
> +struct __free_bit_traits<locale> {
> +    // expose the free bit in `__imp*`
>  };
>  
>  class _LIBCPP_EXPORTED_FROM_ABI locale::facet
> diff --git a/libcxx/include/streambuf b/libcxx/include/streambuf
> index 095ac7d3ad83..b42fae608a5a 100644
> --- a/libcxx/include/streambuf
> +++ b/libcxx/include/streambuf
> @@ -296,7 +296,9 @@ protected:
>      virtual int_type overflow(int_type __c = traits_type::eof());
>  
>  private:
> -    locale __loc_;
> +    static_assert(__has_free_bits<locale>);
> +    __free_bit_pair<locale, 1> __loc_;
> +    // Now implement a lock using that single free bit.
>      char_type* __binp_;
>      char_type* __ninp_;
>      char_type* __einp_;
> diff --git a/libcxx/src/locale.cpp b/libcxx/src/locale.cpp
> index 59c7ce4d43d6..ade1aa688c4d 100644
> --- a/libcxx/src/locale.cpp
> +++ b/libcxx/src/locale.cpp
> @@ -543,7 +543,7 @@ locale::__imp::make_classic()
>      // only one thread can get in here and it only gets in once
>      static aligned_storage<sizeof(locale)>::type buf;
>      locale* c = reinterpret_cast<locale*>(&buf);
> -    c->__locale_ = &make<__imp>(1u);
> +    c->__get_locale() = &make<__imp>(1u);
>      return *c;
>  }
>  
> ```
> 
> Do you know what other implementations have done? Have they gone through the trouble of finding a lock inside streambufs or do they use a global map?
They use "global" maps. As discussed I think it makes more sense not to do this and mark the feature as experimental. Then investigate whether this alternative approach is worth the hassle. I still think we should finish it before LLVM 18.


================
Comment at: libcxx/include/syncstream:386
+
+  basic_string<_CharT, _Traits, _Allocator> __str_;
+  bool __emit_on_sync_{false};
----------------
ldionne wrote:
> Do we take advantage of what `std::string` provides (i.e. growth characteristics, SBO, etc..). If we do take advantage of those, then it might be worth using `std::string`, otherwise I think we might as well use a character buffer we manually manage to save on the duplication of begin/end information.
No basically the SBO is "killed" by "__str_.resize(__str_.capacity() + 1);" This could be smarter and keep the SBO. I'm not sure how useful SBO is. It only helps if all data flushed fits in the SBO area.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67086