Use Interlocked* functions
To see how far we are, I tried grepping to get This->refs stuff grep -r -n -e 'TRACE(.*This->ref)' * grep -r -n "++(This->ref)" * grep -r -n -- "--(This->ref)" * I hope there are a lot of false positives there :-/ regards, Joris
On Mon, 10 Jan 2005 11:38:20 +0100, Joris Huizer wrote:
To see how far we are, I tried grepping to get This->refs stuff grep -r -n -e 'TRACE(.*This->ref)' * grep -r -n "++(This->ref)" * grep -r -n -- "--(This->ref)" *
I hope there are a lot of false positives there :-/
You realise that not every object has to be thread safe, right? In fact quite a few don't. Also some COM objects are internal, and others inc the lock count inside a mutex. Finally the first grep doesn't matter as reading a refcount is atomic anyway (but not adding or subtracting). To be quite honest I'm not convinced this janitorial task is a good idea. It makes the code look at first glance like it's thread safe when it actually isn't. A better janitorial task would simply be "Make free-threaded COM objects thread safe" except that making code thread safe can actually be quite a lot of work, it's not just copy/paste. thanks -mike
Mike Hearn wrote:
On Mon, 10 Jan 2005 11:38:20 +0100, Joris Huizer wrote:
To see how far we are, I tried grepping to get This->refs stuff grep -r -n -e 'TRACE(.*This->ref)' * grep -r -n "++(This->ref)" * grep -r -n -- "--(This->ref)" *
I hope there are a lot of false positives there :-/
You realise that not every object has to be thread safe, right? In fact quite a few don't. Also some COM objects are internal, and others inc the lock count inside a mutex. Finally the first grep doesn't matter as reading a refcount is atomic anyway (but not adding or subtracting).
Paul Vriens was posting patches in which references to This->ref in TRACE calls were replaced by a variable containing the value of This->ref - I can't remember why this was necessary, but I don't think he's doing it just for the fun of it; I also saw, at least some of the patches were in files not listed on the janiturial page; should that list be updated? how could we see wether a given dll directory needs cleaning up? regards, Joris
On Mon, 10 Jan 2005 18:03:30 +0100, Joris Huizer wrote:
Paul Vriens was posting patches in which references to This->ref in TRACE calls were replaced by a variable containing the value of This->ref - I can't remember why this was necessary, but I don't think he's doing it just for the fun of it
Right, in the case where you do this: DWORD refs = InterlockedDecrement(&This->refs); TRACE("refcount is now %d\n", This->refs); that's clearly wrong, it should be refs in the second line. But refcounts are dumped all over the place, I have a feeling that grep will give a *lot* of false positives ....
Mike Hearn wrote:
On Mon, 10 Jan 2005 18:03:30 +0100, Joris Huizer wrote:
Paul Vriens was posting patches in which references to This->ref in TRACE calls were replaced by a variable containing the value of This->ref - I can't remember why this was necessary, but I don't think he's doing it just for the fun of it
Right, in the case where you do this:
DWORD refs = InterlockedDecrement(&This->refs);
TRACE("refcount is now %d\n", This->refs);
that's clearly wrong, it should be refs in the second line. But refcounts are dumped all over the place, I have a feeling that grep will give a *lot* of false positives ....
Yeah, I'm sure you are right; I'm sorry, I didn't really think hard I but later on I noticed myself it's in other places as well; I now guess/think looking at grep -r -n "_AddRef(LPCLASSFACTORY" * is the way to go - which gave me 71 hits, of which a number is already done; Would this just hit on (all?) the correct functions ? sorry for the confusion regards, Joris
Mike Hearn wrote:
To be quite honest I'm not convinced this janitorial task is a good idea. It makes the code look at first glance like it's thread safe when it actually isn't. A better janitorial task would simply be "Make free-threaded COM objects thread safe" except that making code thread safe can actually be quite a lot of work, it's not just copy/paste.
Well, it's true that some objects don't need to be thread safe, however it's not recorded anywhere in the Wine source which object need to be thread safe and which don't. The most important reason in my mind to do this is so that people implementing objects in the future copy code that uses the Interlocked* functions. There's no overhead to using those functions... no penalty if we use them in code that doesn't need to be thread safe, so we should just use them everywhere. The janitorial task doesn't say "Make ole32 objects thread safe" because that's alot more work. It is one small step in the process of making objects thread safe... an introduction if you like. Hopefully it helps some newcomers to Wine understand thread safety and OLE objects a little. Maybe then oneday they'll can that the code isn't thread safe, and undertake to fix some of it, which is a much bigger and harder task. Until then this is something easy for people to do to help out. Mike
Mike McCormack wrote:
The most important reason in my mind to do this is so that people implementing objects in the future copy code that uses the Interlocked* functions. There's no overhead to using those functions... no penalty if we use them in code that doesn't need to be thread safe, so we should just use them everywhere.
On uniprocessor machines this is true, but I don't think the same can be said for SMP/SMT machines. The "lock" prefix for the instructions causes the processor to emit a signal that tells all other processors to not access memory for a period of time. I haven't seen any benchmarks that say how much this costs, but it isn't "no overhead." Rob
On Mon, 10 Jan 2005 22:42:22 +0000, Mike McCormack wrote:
Well, it's true that some objects don't need to be thread safe, however it's not recorded anywhere in the Wine source which object need to be thread safe and which don't.
That info is usually available in MSDN or the registry. But I admit it's not always obvious at first sight.
The most important reason in my mind to do this is so that people implementing objects in the future copy code that uses the Interlocked* functions. There's no overhead to using those functions... no penalty if we use them in code that doesn't need to be thread safe, so we should just use them everywhere.
Technically there is as it causes a bus assert and a dcache flush on SMP machines but I doubt we care about tiny hits.
The janitorial task doesn't say "Make ole32 objects thread safe" because that's alot more work. It is one small step in the process of making objects thread safe... an introduction if you like. Hopefully it helps some newcomers to Wine understand thread safety and OLE objects a little. Maybe then oneday they'll can that the code isn't thread safe, and undertake to fix some of it, which is a much bigger and harder task.
Until then this is something easy for people to do to help out.
Yes fair enough, it is work that would otherwise have to be done some other time so it's good to get it done now. thanks -mike
participants (5)
-
Joris Huizer -
Mike Hearn -
Mike Hearn -
Mike McCormack -
Rob Shearman