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

[Andrew Pollard]

Benjamin Kosnik wrote:
>>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...)
> 
> 
> I'm not excited by this patch. There has got to be a better way. Loren,
> can you commet on this? I should have just waited for you before I
> checked this in anyway. 

Fair enough. It was just a quick patch to fix the problems introduced by
moving the location of the locking. I agree that there is probably a better
way to do it though.

> 
>>   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;

> weak

>>     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;

> really weak

>>     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;
>>   }

Might be "weak", should do the job though :-)

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