Re: [PATCH] Add __cow_string C string constructor
On 07/01/2024 21:53, Jonathan Wakely wrote:
On Sun, 7 Jan 2024 at 18:50, François Dumont wrote:
On 07/01/2024 17:34, Jonathan Wakely wrote:
On Sun, 7 Jan 2024 at 12:57, François Dumont 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 :
__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 .
Re: [PATCH] Add __cow_string C string constructor
On Sun, 7 Jan 2024 at 18:50, François Dumont wrote:
>
>
> On 07/01/2024 17:34, Jonathan Wakely wrote:
> > On Sun, 7 Jan 2024 at 12:57, François Dumont 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.
Re: [PATCH] Add __cow_string C string constructor
On 07/01/2024 17:34, Jonathan Wakely wrote: On Sun, 7 Jan 2024 at 12:57, François Dumont 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
Re: [PATCH] Add __cow_string C string constructor
On Sun, 7 Jan 2024 at 12:57, François Dumont 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 >
[PATCH] Add __cow_string C string constructor
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.
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
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
