| Message ID | 9a2de7f9-7ac3-4d4a-a4cd-6a379c4217bc@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add __cow_string C string constructor | expand |
On Sun, 7 Jan 2024 at 12:57, François Dumont <frs.dumont@gmail.com> wrote: > > Hi > > While working on the patch to use the cxx11 abi in gnu version namespace > mode I got a small problem with this missing constructor. I'm not sure > that the main patch will be integrated in gcc 14 so I think it is better > if I propose this patch independently. > > libstdc++: Add __cow_string constructor from C string > > The __cow_string is instantiated from a C string in > cow-stdexcept.cc. At the moment > the constructor from std::string is being used with the drawback of > an intermediate > potential allocation/deallocation and copy. With the C string > constructor we bypass > all those operations. But in that file, the std::string is the COW string, which means that when we construct a std::string and copy it, it's cheap. It's just a reference count increment/decrement. There should be no additional allocation or deallocation. Am I missing something? > > libstdc++-v3/ChangeLog: > > * include/std/stdexcept (__cow_string(const char*)): New > definition. > * src/c++11/cow-stdexcept.cc (__cow_string(const char*)): > New definition and > declaration. > > Tested under Linux x64, ok to commit ? > > François >
On 07/01/2024 17:34, Jonathan Wakely wrote: > On Sun, 7 Jan 2024 at 12:57, François Dumont <frs.dumont@gmail.com> wrote: >> Hi >> >> While working on the patch to use the cxx11 abi in gnu version namespace >> mode I got a small problem with this missing constructor. I'm not sure >> that the main patch will be integrated in gcc 14 so I think it is better >> if I propose this patch independently. >> >> libstdc++: Add __cow_string constructor from C string >> >> The __cow_string is instantiated from a C string in >> cow-stdexcept.cc. At the moment >> the constructor from std::string is being used with the drawback of >> an intermediate >> potential allocation/deallocation and copy. With the C string >> constructor we bypass >> all those operations. > But in that file, the std::string is the COW string, which means that > when we construct a std::string and copy it, it's cheap. It's just a > reference count increment/decrement. There should be no additional > allocation or deallocation. Good remark but AFAI understand in this case std::string is the cxx11 one. I'll take a second look. Clearly in my gnu version namespace patch it is the cxx11 implementation. Even if so, why do we want to do those additional operations ? Adding this C string constructor will make sure that no useless operations will be done. > > Am I missing something? > > >> libstdc++-v3/ChangeLog: >> >> * include/std/stdexcept (__cow_string(const char*)): New >> definition. >> * src/c++11/cow-stdexcept.cc (__cow_string(const char*)): >> New definition and >> declaration. >> >> Tested under Linux x64, ok to commit ? >> >> François >>
On Sun, 7 Jan 2024 at 18:50, François Dumont <frs.dumont@gmail.com> wrote: > > > On 07/01/2024 17:34, Jonathan Wakely wrote: > > On Sun, 7 Jan 2024 at 12:57, François Dumont <frs.dumont@gmail.com> wrote: > >> Hi > >> > >> While working on the patch to use the cxx11 abi in gnu version namespace > >> mode I got a small problem with this missing constructor. I'm not sure > >> that the main patch will be integrated in gcc 14 so I think it is better > >> if I propose this patch independently. > >> > >> libstdc++: Add __cow_string constructor from C string > >> > >> The __cow_string is instantiated from a C string in > >> cow-stdexcept.cc. At the moment > >> the constructor from std::string is being used with the drawback of > >> an intermediate > >> potential allocation/deallocation and copy. With the C string > >> constructor we bypass > >> all those operations. > > But in that file, the std::string is the COW string, which means that > > when we construct a std::string and copy it, it's cheap. It's just a > > reference count increment/decrement. There should be no additional > > allocation or deallocation. > > Good remark but AFAI understand in this case std::string is the cxx11 > one. I'll take a second look. > > Clearly in my gnu version namespace patch it is the cxx11 implementation. I hope not! The whole point of that type is to always be a COW string, which it does by storing a COW std::basic_string in the union, but wrapping it in a class with a different name, __cow_string. If your patch to use the SSO string in the versioned namespace doesn't change that file to guarantee that __cow_string is still a copy-on-write type then the patch is wrong and must be fixed. > > Even if so, why do we want to do those additional operations ? Adding > this C string constructor will make sure that no useless operations will > be done. Yes, we could avoid an atomic increment and decrement, but that type is only used when throwing an exception so the overhead of allocating memory and calling __cxa_throw etc. is far higher than an atomic inc/dec pair. I was going to say that the new constructor would need to be exported from the shared lib, but I think the new constructor is only ever used in these two places, both defined in that same file: logic_error::logic_error(const char* __arg) : exception(), _M_msg(__arg) { } runtime_error::runtime_error(const char* __arg) : exception(), _M_msg(__arg) { } So I think the change is safe, but I don't think it's urgent, and certainly not needed for the reasons claimed in the patch description.
On 07/01/2024 21:53, Jonathan Wakely wrote: > On Sun, 7 Jan 2024 at 18:50, François Dumont <frs.dumont@gmail.com> wrote: >> >> On 07/01/2024 17:34, Jonathan Wakely wrote: >>> On Sun, 7 Jan 2024 at 12:57, François Dumont <frs.dumont@gmail.com> wrote: >>>> Hi >>>> >>>> While working on the patch to use the cxx11 abi in gnu version namespace >>>> mode I got a small problem with this missing constructor. I'm not sure >>>> that the main patch will be integrated in gcc 14 so I think it is better >>>> if I propose this patch independently. >>>> >>>> libstdc++: Add __cow_string constructor from C string >>>> >>>> The __cow_string is instantiated from a C string in >>>> cow-stdexcept.cc. At the moment >>>> the constructor from std::string is being used with the drawback of >>>> an intermediate >>>> potential allocation/deallocation and copy. With the C string >>>> constructor we bypass >>>> all those operations. >>> But in that file, the std::string is the COW string, which means that >>> when we construct a std::string and copy it, it's cheap. It's just a >>> reference count increment/decrement. There should be no additional >>> allocation or deallocation. >> Good remark but AFAI understand in this case std::string is the cxx11 >> one. I'll take a second look. >> >> Clearly in my gnu version namespace patch it is the cxx11 implementation. > I hope not! The whole point of that type is to always be a COW string, > which it does by storing a COW std::basic_string in the union, but > wrapping it in a class with a different name, __cow_string. > > If your patch to use the SSO string in the versioned namespace doesn't > change that file to guarantee that __cow_string is still a > copy-on-write type then the patch is wrong and must be fixed. Don't worry, __cow_string is indeed wrapping a COW string. What I meant is that in this constructor in <stdexcept>: __cow_string(const std::string&); The std::string parameter is the SSO string. However, as you said, in cow-stdexcept.cc the similar constructor is in fact taking a COW string so it has less importance. It's just a ODR issue. In my gnu version namespace patch however this type is still the SSO string in cow-stdexcept.cc so I'll keep it in this context. >> Even if so, why do we want to do those additional operations ? Adding >> this C string constructor will make sure that no useless operations will >> be done. > Yes, we could avoid an atomic increment and decrement, but that type > is only used when throwing an exception so the overhead of allocating > memory and calling __cxa_throw etc. is far higher than an atomic > inc/dec pair. > > I was going to say that the new constructor would need to be exported > from the shared lib, but I think the new constructor is only ever used > in these two places, both defined in that same file: > > logic_error::logic_error(const char* __arg) > : exception(), _M_msg(__arg) { } > > runtime_error::runtime_error(const char* __arg) > : exception(), _M_msg(__arg) { } > > So I think the change is safe, but I don't think it's urgent, and > certainly not needed for the reasons claimed in the patch description. > The ODR violation has no side effect, it confirms your statement, looks like the __cow_string(const std::string&) could be removed from <stdexcept>.
diff --git a/libstdc++-v3/include/std/stdexcept b/libstdc++-v3/include/std/stdexcept index 66c8572d0cd..2e3c9f3bf71 100644 --- a/libstdc++-v3/include/std/stdexcept +++ b/libstdc++-v3/include/std/stdexcept @@ -54,6 +54,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __cow_string(); __cow_string(const std::string&); + __cow_string(const char*); __cow_string(const char*, size_t); __cow_string(const __cow_string&) _GLIBCXX_NOTHROW; __cow_string& operator=(const __cow_string&) _GLIBCXX_NOTHROW; diff --git a/libstdc++-v3/src/c++11/cow-stdexcept.cc b/libstdc++-v3/src/c++11/cow-stdexcept.cc index 8d1cc4605d4..12b189b43b5 100644 --- a/libstdc++-v3/src/c++11/cow-stdexcept.cc +++ b/libstdc++-v3/src/c++11/cow-stdexcept.cc @@ -127,6 +127,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __cow_string(); __cow_string(const std::string& s); + __cow_string(const char*); __cow_string(const char*, size_t n); __cow_string(const __cow_string&) noexcept; __cow_string& operator=(const __cow_string&) noexcept; @@ -139,6 +140,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __cow_string::__cow_string(const std::string& s) : _M_str(s) { } + __cow_string::__cow_string(const char* s) : _M_str(s) { } + __cow_string::__cow_string(const char* s, size_t n) : _M_str(s, n) { } __cow_string::__cow_string(const __cow_string& s) noexcept