libstdc++/7445: poor performance of std::locale::classic() in multi-threaded applications

[Andrew Pollard]

Benjamin Kosnik wrote:
>>There was a reason that I locked the entire method, so that _S_classic
>>is guaranteed to only be set once. With the code right now, there are
>>a couple of problems....
> 
> 
> Yikes.
> 
> I think the original patch was better. Sorry. Here's an update on
> current sources. Let me know if this is what will work.
> 
> Index: src/locale.cc
> ===================================================================
> RCS file: /cvs/gcc/gcc/libstdc++-v3/src/locale.cc,v
> retrieving revision 1.62
> diff -c -p -r1.62 locale.cc
> *** src/locale.cc	31 Jul 2002 19:34:08 -0000	1.62
> --- src/locale.cc	2 Aug 2002 16:20:20 -0000
> *************** namespace std 
> *** 284,294 ****
>     const locale&
>     locale::classic()
>     {
>       if (!_S_classic)
>         {
> - 	static _STL_mutex_lock __lock __STL_MUTEX_INITIALIZER;
>   	_STL_auto_lock __auto(__lock);
> - 
>   	try 
>   	  {
>   	    // 26 Standard facets, 2 references.
> --- 284,293 ----
>     const locale&
>     locale::classic()
>     {
> +     static _STL_mutex_lock __lock __STL_MUTEX_INITIALIZER;
>       if (!_S_classic)
>         {
>   	_STL_auto_lock __auto(__lock);
>   	try 
>   	  {
>   	    // 26 Standard facets, 2 references.
> 
> 
> 

I still think the two problems I mentioned are still there even with
this version of the patch, namely

a) Two threads can enter the method at once, the first will get the
    lock, set up _S_classic, etc, and release the lock, then the 2nd
    will do exactly the same things again.
    This can be fixed by checking _S_classic again after the auto lock

b) If a second thread enters while another is in the process of setting
    up _S_classic, ie it has assigned _S_classic, but not initialised
    c_locale, the 2nd thread will return the unconstructed c_locale
    object.
    This can be fixed by setting _S_classic as the last thing in the
    method

I think the location of the static mutex doesn't matter.

So, again, I think the entire routine needs to be changed to something
like this... (comments included to indicate reasoning...)

<code>
   const locale&
   locale::classic()
   {
     // libstdc++/7445 - Optimisation to avoid the auto-lock once the initial
     // creation has been done
     if (_S_classic)
       return c_locale;

     static _STL_mutex_lock __lock __STL_MUTEX_INITIALIZER;
     _STL_auto_lock __auto(__lock);

     // In case multiple threads call locale::classic() at the same time with
     // _S_classic unset, check if another thread might have already done the
     // work
     if (_S_classic)
       return c_locale;

     locale::_Impl* __tmp_S_classic = 0;
     try
       {
         // 26 Standard facets, 2 references.
         // One reference for _M_classic, one for _M_global
         facet** f = new(&facet_vec) facet*[_GLIBCPP_NUM_FACETS];
         for (size_t __i = 0; __i < _GLIBCPP_NUM_FACETS; ++__i)
           f[__i] = 0;

         __tmp_S_classic = new (&c_locale_impl) _Impl(f, 2, true);
         new (&c_locale) locale(__tmp_S_classic);

         // Set _S_classic last when we know we have done all the initialisation
         _S_classic = _S_global = __tmp_S_classic;
       }
     catch(...)
       {
         // Just call destructor, so that locale_impl_c's memory is
         // not deallocated via a call to delete.
         if (tmp_S_classic)
           tmp_S_classic->~_Impl();
         _S_classic = _S_global = 0;
         __throw_exception_again;
       }
     return c_locale;
   }
</code>
Andrew.
-- 
        Andrew Pollard - Senior Software Engineer (APF)
    Brooks-PRI Automation - Planning and Logistics Solutions
Email: Andrew.Pollard@brooks-pri.com - Tel: +44 (0)118 9215603