| Message ID | 20180926161722.GA31395@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | PR libstdc++/59439 optimize uses of classic ("C") std::locale | expand |
* Jonathan Wakely:
> Does anybody see any problems with this change?
Can you put a debug assert into _M_add_reference and
_M_remove_reference, to check that they are never called on an
_S_classic object? Or put the check directly into those functions?
Thanks,
Florian
On 26/09/18 19:31 +0200, Florian Weimer wrote: >* Jonathan Wakely: > >> Does anybody see any problems with this change? > >Can you put a debug assert into _M_add_reference and >_M_remove_reference, to check that they are never called on an >_S_classic object? Or put the check directly into those functions? I thought about putting it directly in those functions, as that would ensure we don't get it wrong. But those functions are defined inline in the header, so there is already user code calling them that has inlined the functions. That existing code won't contain the checks, and so would continue to update reference counts on the _S_classic object, potentially deleting it while it's still in use. We could put debug assertions in there, but I prefer not to pessimise user code with checks that our implementation is correct. Maybe in this case the benefits outweight the cost. This code rarely changes though, so I am not very concerned that somebody will add new updates of the reference counts and forget to check for == _S_classic. I did review all uses of locale::_Impl:_M_(add|remove)_reference, and the only one that I didn't change (and the only one which is in a header and so can't be altered in existing user code) is in the locale(const locale& __other, _Facet* __f) constructor in <bits/locale_classes.tcc>. We know we've just created a new _Impl with a reference count of 1, so it's not the _S_classic object, and decrementing the reference count unconditionally is correct.
On 26/09/18 17:17 +0100, Jonathan Wakely wrote: >The global locale::_Impl that represents the "C" locale is never >destroyed, so there is no need to keep track of reference count updates >for that object. This greatly reduce contention between threads that >refer to the classic locale. Since the global std::locale initially uses >the classic locale, this benefits the common case for any code using the >global locale, such as construction/destruction of iostream objects. > >All these updates are done inside libstdc++.so so there's no need to >worry about users' objects having inlined old versions of the code which >still update the reference count for the classic locale. > > PR libstdc++/59439 > * src/c++98/locale.cc (locale::locale(const locale&)): Bypass > reference count updates for the classic locale. > (locale::~locale()): Likewise. (locale::operator=(const > locale&)): Likewise. > * src/c++98/locale_init.cc (locale::locale()): Likewise. > (locale::global(const locale&)): Likewise. > >Tested x86_64-linux, not committed yet. > >Does anybody see any problems with this change? Committed to trunk. >commit 131d4c26876a5a884fe4408deaf054e01ba90ffb >Author: Jonathan Wakely <jwakely@redhat.com> >Date: Wed Sep 26 16:34:42 2018 +0100 > > PR libstdc++/59439 optimize uses of classic ("C") std::locale > > The global locale::_Impl that represents the "C" locale is never > destroyed, so there is no need to keep track of reference count updates > for that object. This greatly reduce contention between threads that > refer to the classic locale. Since the global std::locale initially uses > the classic locale, this benefits the common case for any code using the > global locale, such as construction/destruction of iostream objects. > > All these updates are done inside libstdc++.so so there's no need to > worry about users' objects having inlined old versions of the code which > still update the reference count for the classic locale. > > PR libstdc++/59439 > * src/c++98/locale.cc (locale::locale(const locale&)): Bypass > reference count updates for the classic locale. > (locale::~locale()): Likewise. (locale::operator=(const > locale&)): Likewise. > * src/c++98/locale_init.cc (locale::locale()): Likewise. > (locale::global(const locale&)): Likewise. > >diff --git a/libstdc++-v3/src/c++98/locale.cc b/libstdc++-v3/src/c++98/locale.cc >index 148bf59658e..fe06d297039 100644 >--- a/libstdc++-v3/src/c++98/locale.cc >+++ b/libstdc++-v3/src/c++98/locale.cc >@@ -77,7 +77,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > locale::locale(const locale& __other) throw() > : _M_impl(__other._M_impl) >- { _M_impl->_M_add_reference(); } >+ { >+ if (_M_impl != _S_classic) >+ _M_impl->_M_add_reference(); >+ } > > // This is used to initialize global and classic locales, and > // assumes that the _Impl objects are constructed correctly. >@@ -86,7 +89,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > { } > > locale::~locale() throw() >- { _M_impl->_M_remove_reference(); } >+ { >+ if (_M_impl != _S_classic) >+ _M_impl->_M_remove_reference(); >+ } > > bool > locale::operator==(const locale& __rhs) const throw() >@@ -112,8 +118,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > const locale& > locale::operator=(const locale& __other) throw() > { >- __other._M_impl->_M_add_reference(); >- _M_impl->_M_remove_reference(); >+ if (__other._M_impl != _S_classic) >+ __other._M_impl->_M_add_reference(); >+ if (_M_impl != _S_classic) >+ _M_impl->_M_remove_reference(); > _M_impl = __other._M_impl; > return *this; > } >diff --git a/libstdc++-v3/src/c++98/locale_init.cc b/libstdc++-v3/src/c++98/locale_init.cc >index c9078c015c3..b580a9f9d58 100644 >--- a/libstdc++-v3/src/c++98/locale_init.cc >+++ b/libstdc++-v3/src/c++98/locale_init.cc >@@ -257,9 +257,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > // fall back to lock protected access to both _S_global and > // its reference count. > _M_impl = _S_global; >- if (_M_impl == _S_classic) >- _M_impl->_M_add_reference(); >- else >+ if (_M_impl != _S_classic) > { > __gnu_cxx::__scoped_lock sentry(get_locale_mutex()); > _S_global->_M_add_reference(); >@@ -275,7 +273,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > { > __gnu_cxx::__scoped_lock sentry(get_locale_mutex()); > __old = _S_global; >- __other._M_impl->_M_add_reference(); >+ if (__other._M_impl != _S_classic) >+ __other._M_impl->_M_add_reference(); > _S_global = __other._M_impl; > const string __other_name = __other.name(); > if (__other_name != "*") >@@ -284,7 +283,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > // Reference count sanity check: one reference removed for the > // subsition of __other locale, one added by return-by-value. Net >- // difference: zero. When the returned locale object's destrutor >+ // difference: zero. When the returned locale object's destructor > // is called, then the reference count is decremented and possibly > // destroyed. > return locale(__old);
diff --git a/libstdc++-v3/src/c++98/locale.cc b/libstdc++-v3/src/c++98/locale.cc index 148bf59658e..fe06d297039 100644 --- a/libstdc++-v3/src/c++98/locale.cc +++ b/libstdc++-v3/src/c++98/locale.cc @@ -77,7 +77,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION locale::locale(const locale& __other) throw() : _M_impl(__other._M_impl) - { _M_impl->_M_add_reference(); } + { + if (_M_impl != _S_classic) + _M_impl->_M_add_reference(); + } // This is used to initialize global and classic locales, and // assumes that the _Impl objects are constructed correctly. @@ -86,7 +89,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { } locale::~locale() throw() - { _M_impl->_M_remove_reference(); } + { + if (_M_impl != _S_classic) + _M_impl->_M_remove_reference(); + } bool locale::operator==(const locale& __rhs) const throw() @@ -112,8 +118,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION const locale& locale::operator=(const locale& __other) throw() { - __other._M_impl->_M_add_reference(); - _M_impl->_M_remove_reference(); + if (__other._M_impl != _S_classic) + __other._M_impl->_M_add_reference(); + if (_M_impl != _S_classic) + _M_impl->_M_remove_reference(); _M_impl = __other._M_impl; return *this; } diff --git a/libstdc++-v3/src/c++98/locale_init.cc b/libstdc++-v3/src/c++98/locale_init.cc index c9078c015c3..b580a9f9d58 100644 --- a/libstdc++-v3/src/c++98/locale_init.cc +++ b/libstdc++-v3/src/c++98/locale_init.cc @@ -257,9 +257,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // fall back to lock protected access to both _S_global and // its reference count. _M_impl = _S_global; - if (_M_impl == _S_classic) - _M_impl->_M_add_reference(); - else + if (_M_impl != _S_classic) { __gnu_cxx::__scoped_lock sentry(get_locale_mutex()); _S_global->_M_add_reference(); @@ -275,7 +273,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { __gnu_cxx::__scoped_lock sentry(get_locale_mutex()); __old = _S_global; - __other._M_impl->_M_add_reference(); + if (__other._M_impl != _S_classic) + __other._M_impl->_M_add_reference(); _S_global = __other._M_impl; const string __other_name = __other.name(); if (__other_name != "*") @@ -284,7 +283,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // Reference count sanity check: one reference removed for the // subsition of __other locale, one added by return-by-value. Net - // difference: zero. When the returned locale object's destrutor + // difference: zero. When the returned locale object's destructor // is called, then the reference count is decremented and possibly // destroyed. return locale(__old);
The global locale::_Impl that represents the "C" locale is never destroyed, so there is no need to keep track of reference count updates for that object. This greatly reduce contention between threads that refer to the classic locale. Since the global std::locale initially uses the classic locale, this benefits the common case for any code using the global locale, such as construction/destruction of iostream objects. All these updates are done inside libstdc++.so so there's no need to worry about users' objects having inlined old versions of the code which still update the reference count for the classic locale. PR libstdc++/59439 * src/c++98/locale.cc (locale::locale(const locale&)): Bypass reference count updates for the classic locale. (locale::~locale()): Likewise. (locale::operator=(const locale&)): Likewise. * src/c++98/locale_init.cc (locale::locale()): Likewise. (locale::global(const locale&)): Likewise. Tested x86_64-linux, not committed yet. Does anybody see any problems with this change? commit 131d4c26876a5a884fe4408deaf054e01ba90ffb Author: Jonathan Wakely <jwakely@redhat.com> Date: Wed Sep 26 16:34:42 2018 +0100 PR libstdc++/59439 optimize uses of classic ("C") std::locale The global locale::_Impl that represents the "C" locale is never destroyed, so there is no need to keep track of reference count updates for that object. This greatly reduce contention between threads that refer to the classic locale. Since the global std::locale initially uses the classic locale, this benefits the common case for any code using the global locale, such as construction/destruction of iostream objects. All these updates are done inside libstdc++.so so there's no need to worry about users' objects having inlined old versions of the code which still update the reference count for the classic locale. PR libstdc++/59439 * src/c++98/locale.cc (locale::locale(const locale&)): Bypass reference count updates for the classic locale. (locale::~locale()): Likewise. (locale::operator=(const locale&)): Likewise. * src/c++98/locale_init.cc (locale::locale()): Likewise. (locale::global(const locale&)): Likewise.