+1<div>Haris<br><br><div class="gmail_quote">On Fri, Jan 22, 2010 at 5:08 PM, Orest Halustchak <span dir="ltr"><<a href="mailto:orest.halustchak@autodesk.com">orest.halustchak@autodesk.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+1 Orest.<br>
<div class="im"><br>
-----Original Message-----<br>
From: <a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a> [mailto:<a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a>] On Behalf Of Traian Stanev<br>
</div><div class="im">Sent: Friday, January 22, 2010 11:08 AM<br>
To: FDO Internals Mail List<br>
</div><div><div></div><div class="h5">Subject: RE: [fdo-internals] MOTION: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release<br>
<br>
<br>
+1 Traian<br>
<br>
<br>
-----Original Message-----<br>
From: <a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a> [mailto:<a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a>] On Behalf Of Greg Boone<br>
Sent: Thursday, January 21, 2010 1:34 PM<br>
To: FDO Internals Mail List<br>
Subject: [fdo-internals] MOTION: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release<br>
<br>
Hi All,<br>
<br>
If there are no additional comments, I would like to propose a vote on RFC 47. <a href="http://trac.osgeo.org/fdo/wiki/FDORfc47" target="_blank">http://trac.osgeo.org/fdo/wiki/FDORfc47</a><br>
<br>
The plan at this time is to proceed with the alternate solution as outlined by Traian, with atomic operations being used on Linux instead of a Mutex.<br>
<br>
Regards,<br>
Greg<br>
<br>
<br>
-----Original Message-----<br>
From: <a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a> [mailto:<a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a>] On Behalf Of Greg Boone<br>
Sent: Tuesday, January 19, 2010 3:55 PM<br>
To: FDO Internals Mail List<br>
Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release<br>
<br>
For those interested in a detailed explanation of the asm code, refer to...<br>
<br>
<a href="http://www.experts-exchange.com/Programming/Languages/Assembly/Q_23571643.html" target="_blank">http://www.experts-exchange.com/Programming/Languages/Assembly/Q_23571643.html</a><br>
<br>
Regards,<br>
Greg<br>
<br>
<br>
-----Original Message-----<br>
From: <a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a> [mailto:<a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a>] On Behalf Of Traian Stanev<br>
Sent: Tuesday, January 19, 2010 2:04 PM<br>
To: FDO Internals Mail List<br>
Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release<br>
<br>
<br>
Small correction for the asm call, in case we end up using it:<br>
<br>
{<br>
int ret = 1;<br>
__asm__ __volatile__ ("lock xaddl %0,%1" : "=r"(ret), "=m"(m_refCount) : "0"(ret), "m"(m_refCount) : "memory");<br>
return ret+1;<br>
}<br>
<br>
This one compiles ok on 4.4 also. I'm no expert on GCC inline assembly, but it may be somehow possible to have the instruction set the new value to ret instead of having to increment it in a second instruction before returning it. Returning the refcount from AddRef is for debugging only, so that +1 feels like kind of a waste... Same applies to the decrement that happens in Release. Fortunately this snippet only really needs to be used for the older GCC.<br>
<br>
For Release(), we can use the same asm, but set ret=-1 initially, so that it subtracts 1 from the refcount.<br>
<br>
Traian<br>
<br>
<br>
-----Original Message-----<br>
From: <a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a> [mailto:<a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a>] On Behalf Of Greg Boone<br>
Sent: Tuesday, January 19, 2010 1:38 PM<br>
To: FDO Internals Mail List<br>
Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release<br>
<br>
I agree that we should really get 4.1.2 installed on the build machine.<br>
<br>
Tom... Can we make this upgrade?<br>
<br>
-----Original Message-----<br>
From: <a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a> [mailto:<a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a>] On Behalf Of Traian Stanev<br>
Sent: Tuesday, January 19, 2010 1:28 PM<br>
To: FDO Internals Mail List<br>
Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release<br>
<br>
<br>
Dang, 3.4 is ancient... Even 4.1.2, where those were introduced is like 3 years old. Well, it means we need more ifdefs, something like calling the atomic instruction directly with inline assembly:<br>
<br>
<br>
int AddRef(){<br>
#if defined(__GNUC__) && defined (__GNUC_MINOR__) && ((__GNUC__ < 4) || (__GNUC__ == 4 && __GNUC_MINOR__ < 1))<br>
int ret=1;<br>
__asm__ __volatile__ ("lock\n\t"<br>
"xaddl %0,%1\n\t" : "=r"(ret), "=m"(m_refCount) : "0"(m_refCount), "m"(val) : "memory");<br>
return ret+1;<br>
#elif defined (__GNUC__)<br>
return __sync_add_and_fetch(&m_refCount,1);<br>
#elif defined(WIN32)<br>
return InterlockedAdd(&m_refCount,1);<br>
#else<br>
return ++val;<br>
#endif<br>
}<br>
<br>
Note I didn't check the correctness of the first ifdef, but it looks about right...<br>
<br>
We also need to determine which version of GCC/G++ we should target in the next release, since 3.4 is really really really old.<br>
<br>
Traian<br>
<br>
<br>
<br>
<br>
From: <a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a> [mailto:<a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a>] On Behalf Of Greg Boone<br>
Sent: Tuesday, January 19, 2010 1:25 PM<br>
To: FDO Internals Mail List<br>
Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release<br>
<br>
A slight issue to using these would be that they are not included in earlier version of gcc, such as 3.4.3, which is still being used on our build machines. Gah...<br>
<br>
<br>
From: <a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a> [mailto:<a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a>] On Behalf Of Traian Stanev<br>
Sent: Tuesday, January 19, 2010 1:13 PM<br>
To: FDO Internals Mail List<br>
Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release<br>
<br>
In my Ubuntu 32 bit machine with a relatively newer GCC, the following compiles without having to include any headers at all, since the Intel atomic functions are built into GCC itself.<br>
<br>
FDO_API_COMMON virtual FdoInt32 AddRef()<br>
{<br>
return __sync_add_and_fetch(&m_refCount, 1); <br>
// return ++m_refCount;<br>
}<br>
<br>
Here is the documentation for those:<br>
<br>
<a href="http://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Atomic-Builtins.html" target="_blank">http://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Atomic-Builtins.html</a><br>
<br>
<br>
Traian<br>
<br>
<br>
<br>
<br>
From: <a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a> [mailto:<a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a>] On Behalf Of Greg Boone<br>
Sent: Tuesday, January 19, 2010 1:13 PM<br>
To: FDO Internals Mail List<br>
Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release<br>
<br>
Do you know which function/header file to use?<br>
<br>
/usr/include/alsa/iatomic.h?<br>
<br>
Greg<br>
<br>
From: <a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a> [mailto:<a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a>] On Behalf Of Traian Stanev<br>
Sent: Tuesday, January 19, 2010 12:55 PM<br>
To: FDO Internals Mail List<br>
Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release<br>
<br>
<br>
This is correct if atomic_inc is in a kernel header, that header should not be included from user-space.<br>
<br>
However, GCC does have other headers defining atomic intrinsics, which can be included from user space.<br>
<br>
My point was about using mutex vs. atomic, not specifically about which header (kernel or gcc) to include for the atomic functions.<br>
<br>
Traian<br>
<br>
<br>
From: <a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a> [mailto:<a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a>] On Behalf Of Greg Boone<br>
Sent: Tuesday, January 19, 2010 12:46 PM<br>
To: FDO Internals Mail List<br>
Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release<br>
<br>
When exploring various posts on the usage of atomic_inc, I often see a warning not to include kernel headers from userspace. There is even a pragma warning to this effect in atomic.h.<br>
<br>
Are you saying that it is safe to ignore this advice?<br>
<br>
Greg<br>
<br>
From: <a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a> [mailto:<a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a>] On Behalf Of Traian Stanev<br>
Sent: Friday, January 15, 2010 12:22 PM<br>
To: FDO Internals Mail List<br>
Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release<br>
<br>
<br>
Theoretically both the GCC and the MS VC++ implementations would be calling the same underlying Intel CPU dedicated instruction that does the atomic operation. Chatter on the web is not always reliable., and I would personally doubt threading on Windows before I doubt threading on Linux, given that Linux is used a lot more in thread-heavy environments.<br>
<br>
Traian<br>
<br>
From: <a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a> [mailto:<a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a>] On Behalf Of Greg Boone<br>
Sent: Friday, January 15, 2010 12:17 PM<br>
To: FDO Internals Mail List<br>
Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release<br>
<br>
I took a look at functions such as atomic_dec_and_test and atomic_inc_and_test, but there was a lot of chatter on the web that these calls were not sufficient to provide full thread synchronization. In that light I chose to adopt the strategy MapGuide used and implement the solution on Linux using a mutex.<br>
<br>
Not being a 100% familiar with these operations, I welcome any feedback on their use and the pros and cons of doing so.<br>
<br>
NOTE: I do agree that having a mutex per object is significant overhead on Linux.<br>
<br>
Greg<br>
<br>
From: <a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a> [mailto:<a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a>] On Behalf Of Traian Stanev<br>
Sent: Friday, January 15, 2010 12:12 PM<br>
To: 'FDO Internals Mail List'<br>
Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release<br>
<br>
<br>
Hi Greg,<br>
<br>
What is the reason for using a mutex on Linux instead of atomic operations like on Windows?<br>
<br>
Traian<br>
<br>
<br>
From: <a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a> [mailto:<a href="mailto:fdo-internals-bounces@lists.osgeo.org">fdo-internals-bounces@lists.osgeo.org</a>] On Behalf Of Greg Boone<br>
Sent: Friday, January 15, 2010 11:53 AM<br>
To: FDO Internals Mail List<br>
Subject: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release<br>
<br>
Hi All,<br>
<br>
I have written and posted FDO RFC 47: Add Thread Support to FdoIDisposable AddRef/Release, accessible at:<br>
<br>
<a href="http://trac.osgeo.org/fdo/wiki/FDORfc47" target="_blank">http://trac.osgeo.org/fdo/wiki/FDORfc47</a><br>
<br>
Overview:<br>
<br>
The AddRef and Release methods on FdoIDisposable are not currently thread-safe. As a consequence the m_refCount data member can get corrupted if two separate threads simultaneously call any of these methods on the same object. This is known to happen during garbage collection of Managed FDO objects. Because the managed objects implement a finalizer, they are processed by a separate GC finalizer thread. So while the GC finalizer thread is releasing ref counts on unmanaged objects, the main thread can be simultaneously modifying ref-counts on those same objects.<br>
<br>
Proposed Solution:<br>
<br>
In MapGuide the refcount threading issue is addressed by having the refcounted classes extend from MgGuardDisposable, which in turn uses a mutex to limit access to the refcount member variable to one thread at a time. A similar solution will be implemented for the FDO API, with a slight variation. On Linux, a mutex will be used. On Windows, the FDO API will use the InterlockedIncrement and InterlockedDecrement methods<br>
<br>
On Windows, the use of thread safe functions will always be activated in case where unmanaged FDO objects are wrapped by a Managed object wrapper. Otherwise, API callers will have the option of setting a per-object or global flag that will activate thread-safe access of the reference count member variable.<br>
<br>
<br>
Please refer to the RFC for a full description on the change and a working draft of the proposed solution.<br>
<br>
All comments are welcome.<br>
<br>
Regards,<br>
Greg<br>
_______________________________________________<br>
fdo-internals mailing list<br>
<a href="mailto:fdo-internals@lists.osgeo.org">fdo-internals@lists.osgeo.org</a><br>
<a href="http://lists.osgeo.org/mailman/listinfo/fdo-internals" target="_blank">http://lists.osgeo.org/mailman/listinfo/fdo-internals</a><br>
_______________________________________________<br>
fdo-internals mailing list<br>
<a href="mailto:fdo-internals@lists.osgeo.org">fdo-internals@lists.osgeo.org</a><br>
<a href="http://lists.osgeo.org/mailman/listinfo/fdo-internals" target="_blank">http://lists.osgeo.org/mailman/listinfo/fdo-internals</a><br>
_______________________________________________<br>
fdo-internals mailing list<br>
<a href="mailto:fdo-internals@lists.osgeo.org">fdo-internals@lists.osgeo.org</a><br>
<a href="http://lists.osgeo.org/mailman/listinfo/fdo-internals" target="_blank">http://lists.osgeo.org/mailman/listinfo/fdo-internals</a><br>
_______________________________________________<br>
fdo-internals mailing list<br>
<a href="mailto:fdo-internals@lists.osgeo.org">fdo-internals@lists.osgeo.org</a><br>
<a href="http://lists.osgeo.org/mailman/listinfo/fdo-internals" target="_blank">http://lists.osgeo.org/mailman/listinfo/fdo-internals</a><br>
_______________________________________________<br>
fdo-internals mailing list<br>
<a href="mailto:fdo-internals@lists.osgeo.org">fdo-internals@lists.osgeo.org</a><br>
<a href="http://lists.osgeo.org/mailman/listinfo/fdo-internals" target="_blank">http://lists.osgeo.org/mailman/listinfo/fdo-internals</a><br>
_______________________________________________<br>
fdo-internals mailing list<br>
<a href="mailto:fdo-internals@lists.osgeo.org">fdo-internals@lists.osgeo.org</a><br>
<a href="http://lists.osgeo.org/mailman/listinfo/fdo-internals" target="_blank">http://lists.osgeo.org/mailman/listinfo/fdo-internals</a><br>
_______________________________________________<br>
fdo-internals mailing list<br>
<a href="mailto:fdo-internals@lists.osgeo.org">fdo-internals@lists.osgeo.org</a><br>
<a href="http://lists.osgeo.org/mailman/listinfo/fdo-internals" target="_blank">http://lists.osgeo.org/mailman/listinfo/fdo-internals</a><br>
</div></div></blockquote></div><br></div>