incubator-stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Liviu Nicoara <nikko...@hates.ms>
Subject Re: STDCXX-1056 [was: Re: STDCXX forks]
Date Mon, 17 Sep 2012 12:46:03 GMT
On 09/16/12 22:15, Stefan Teleman wrote:
> On Sun, Sep 16, 2012 at 7:44 PM, Liviu Nicoara <nikkoara@hates.ms> wrote:
>
>>> http://s247136804.onlinehome.us/22.locale.numpunct.mt.1.er.ts/
>>
>> Unfortunately, can't do the same here. Could you please refresh my memory
>> what does the patch contain? This patch is not part of the patch set you
>> published here earlier (http://tinyurl.com/8pyql4g)?
>
> It's not *quite* that one. I completely removed the test for the
> (_C_flags &) and instead do this:
>
>   template <class _CharT>
> inline string numpunct<_CharT>::grouping () const
> {
>      _RWSTD_MT_CLASS_GUARD (_Type);
>
>      return do_grouping();
> }
>
> [ etc etc etc ] for all the public interfaces.

Got it.

>
> This is, essentially, your perfect forwarding patch with a mutex lock
> plastered on top.
>
>> If that is the case I would not ascribe much importance to these numbers.
>
> I cannot simply discount the race conditions reported by a thread
> analyzer. The fact that it happens to be the SunPro thread analyzer is
> not that important to me (although I know it to be quite accurate). I
> am aware that all thread analyzers report false positives.
>
> What causes me concern are two things:
>
> 1. the fact that the number of race conditions reported drops by a
> factor of 10 when mutex locking is applied. This can't be just a
> timing change coincidence caused by changes in scheduling.

If you found the differences to be stable, I'll take your word for that.

>
> 2. an out-of-line virtual function call is not (AFAIK) a thread
> concurrency synchronization primitive.

It is not, but that is not my point. See below.

>
> But, I am also unwilling to give SunPro infallible status. So, here
> are the results for the same exact test cases, this time from the
> Intel Inspector 2003:
>
> 1. no mutex locking (with your perfect forwarding patch);
>
> http://s247136804.onlinehome.us/22.locale.numpunct.mt.intel-inspector-xe/22.locale.numpunct.mt.nts.r000ti3.inspxez
>
> 2. mutex locking (your perfect forwarding patch, preceded by mutex
> lock plastered on top), in every public interface function:
>
> http://s247136804.onlinehome.us/22.locale.numpunct.mt.intel-inspector-xe/22.locale.numpunct.mt.ts.r001ti3.inspxez
>
> These are Inspector XE 2013 binary blobs which can be opened by the
> Intel inspector: /opt/intel/inspector_xe_2013/bin64/inspxe-gui (if
> it's installed in /opt/intel).
>
> These blobs contain the source files as well.

Excellent, I will take a look at them.

In the meantime I would like to stress again that __rw_get_numpunct is perfectly thread-safe
and does not need extra locking for perfect forwarding. There are only two places in __rw_get_numpunct
in which the facet data is initialized and both are correctly protected by appropriate mutex
locking: lines 80 and 134. See the facet management code and the __rw_setlocale class ctor.
Btw, I think the analyzers fail to see the protection offered by the mutex locking across
a recursive call. I don't see any other explanation for the numbers.

The reading of the facet data is again thread-safe, although the reading does not need any
synchronization. Both these assessments are supported by evidence from successfully running
the numpunct MT test and the other, ad-hoc MT tests we spread across this thread.

It continues to be my opinion that the synchronization via the `class' guard is unnecessary
and excessive, although correct. Unnecessary because of the above, excessive because the mutex
is shared across all numpunct facets instantiated in the program.

I will summarize all the findings shortly, but I think the way to go forward is with a perfectly
forwarding public interface, with no additional locking and no caching for reasons of both
simplicity and performance in MT builds. Btw, all this work bears fruits for future std::string
work.

Thanks.

Liviu


Mime
View raw message