Richard Frith-Macdonald schrieb: > > On 4 Sep 2009, at 08:29, Wolfgang Lux wrote: > >> Fred Kiefer wrote: >> >>>> The old version used objc_mutex_t, which was a void*. A mutex is >>>> typically either one or two words, depending on the implementation. >>>> Using malloc for this is very wasteful, both in terms of speed, cache >>>> usage, and memory footprint. The objc_mutex_alloc() function was doing >>>> malloc(sizeof(pthread_mutex_t)); on some platforms (e.g. FreeBSD) >>>> pthread_mutex_t is a pointer to some other structure, so we were tying >>>> up three cache lines for a two word data structure. This is far from >>>> ideal. >>> >>> Not sure if I getting this correctly but on my 64-bit Linux system I >>> have # define __SIZEOF_PTHREAD_MUTEX_T 40 and 24 for a 32-bit system. >>> We are rather talking about 12 to 20 words here not one or two. >> >> Indeed your are getting this correct. Using the test program >> #include <pthread.h> >> int main() >> { >> pthread_mutex_t mutex; >> printf("sizeof(mutex) = %d\n", sizeof(mutex)); >> return 0; >> } >> I've collected the size of a pthread mutex on a few x86 platforms: >> OS X 10.5 (x86): 44 >> Solaris 10 (x86): 24 >> NetBSD 5.0 (x86): 28 >> OpenBSD 4.5 (x86): 4 >> FreeBSD 7.2 (x86): 4 >> OpenSUSE 11.1 (x86): 24 >> So it looks like David is attempting to optimize code for his platform >> while making things worse for everybody else :-(. > > Fred was IMO completely correct to point out that putting pthread.h and > types declared in it in NSLock.h exposes internal workings ... and we > have a policy of not doing that as a general principle. However, I > think it's an unjustified jump to saying this is making things worse for > everyone else ... you need to look at the practical effects of the > change. ... se below. > >>> What do others think, is it worthwhile to hide these implementation >>> details via another indirection or not? I am still in favour of using an >>> opaque data type here. On systems like FreeBSD where pthread_mutex_t is >>> itself a pointer we could use that directly and on other systems we have >>> one additional malloc and free call per mutex. And where the structure >>> fits into your one or two words we could even put the value into the >>> ivar directly. >> >> I absolutely agree with you here. The interface should not expose any of >> the implementation details unless there is a really pressing need to do >> so. I understand David's reasoning that the approach with an opaque >> pointer may lead to additional cache misses on FreeBSD (and apparently >> OpenBSD as well), but without hard figures -- i.e., benchmarks for real >> world programs making intensive use of NSLocks that show a substantial >> performance improvement -- I consider this kind of coding premature >> optimization which should be avoided. > > OK ... I agree with this principle, but look at why ... > > 1. we don't want to expose internal workings because we don't want > developers to start to depend on those internals in such a way that it's > hard for us to change things later without breaking existing applications. > 2. we don't want to expose internal workings because changes to them may > break API and mean that apps need to be recompiled. > > Issue 1 is real, but we can't quantify how important it is as it's a > amatter of perceptions rather than a true technical issue. Luckily it's > quite easily largely fixable, simply by removing pthread.h form the > header and replacing the types from pthread.h with opaque dummy types of > the same size, so that there is no temptation for developers to use them > directly. So I did that, though really, I'm not sure that was worth the > bother, since the ivars concerned were already clearly marked as > private. I guess it's just good to do it to avoid having the extra > symbols polluting developers namespaces. > > Issue 2 is a technical problem ... if someone subclasses one of the lock > classes, their compiled code is obviously dependent on the size of the > superclass and if that size changes (eg due to using a different pthread > implementation) then the size of the superclass would change even though > the version of the base library is unchanged. So potentially an app > linked with one copy of the base library would fail to run properly with > another copy of the library even though it was the same version! > > That sounds really bad, until you think about the chances of it actually > happening ... you would have to have two versions of the pthread > library, with different sized pthread_mutext_t types available on the > same system. You would have to have apps which are subclassing NSLock > or NSRecursiveLock or NSCondition. You would have to have built those > apps with a base library built using one version of the pthread library, > and then you would have to be running with a base library built with the > other version of the pthread library. In reality, I don't expect that > would ever happen (unless perhaps, a developer is playing with thread > library development themselves ... in which case they are unlikely to > have any trouble spotting/resolving the issue on their own). > > Balanced against that is the modest complexity of allocating the mutex > using malloc and freeing it when the NSLock is deallocated, and the > performance hit of doing that, and of performing an extra indirection on > every locking operation. That's a small performance penalty ... but > probably worth avoiding when you consider that locking is hugely heavily > used in any threaded application. > > The trade-off (source code simplicity and improved performance against > ABI stability) is difficult to call, but I think David made the right > design decision here.
Richards reasoning looks sound to me and with his alterations I am now happy with David's change. It is not completely the way I would have loved to see it, but it is far better than our old code and still hiding most of the details from users. Fred _______________________________________________ Discuss-gnustep mailing list Discuss-gnustep@gnu.org http://lists.gnu.org/mailman/listinfo/discuss-gnustep