Walter Bright Wrote:

> Leandro Lucarella wrote:
> > I want to be sure the bug is present before the patch and fixed after the patch before submiting the patch via Bugzilla.
> 
> It's a good fix, I checked in a patch for it.

Thanx :)

I tested it with patched phobos on winxp/centos and it works.

Sean Kelly wrote:
> Walter Bright Wrote:
> 
>> Sean Kelly wrote:
>>> Leandro Lucarella Wrote:
>>> 
>>>> Bane, el 11 de diciembre a las 06:00 me escribiste:
>>>>> Bug with GC fullCollect() in multithreaded environment.
>>>>> Grauzone explained it here http://www.digitalmars.com/webnews/newsgroups.php?art_group=digitalmars.D&article_id=99610
>>>>> 
>>>>> 
>>> To be perfectly safe, it's a tiny bit trickier than that.  You
>>> have to always lock on allocations instead of just locking the
>>> collection cycle in single-threaded apps.  I think the reason for
>>> this is that once the lock is acquired it must be held until the
>>> current thread exits the GC code, and the "synchronized"
>>> statement only allows you to lock for the length of the collect()
>>> call (because the start and end must be within the same scope).
>>> However, this /may/ actually be a separate bug with the D 1.0 GC.
>>> I really can't recall for sure if this was an issue with
>>> collecting when all the other threads were suspended.
>> Yes, the lock on the gc must still be held until the collection
>> work is completed, but the other threads can be unfrozen once the
>> mark phase is complete.
> 
> Hm... so if a dtor creates a new thread then that thread could enter
> and "lock" the GC while the first thread is still completing its
> allocation.  Couldn't this cause problems?

No, because the thread doing the GC collection has locked the GC. Any other thread attempting to alloc will wait on that lock. There is only one GC lock.

Leandro Lucarella wrote:
> If you could add a small description of what the commit is about besides
> pointing the NG discussion/Bugzilla number, as you are doing with DMD now,
> it would be really great! =)

Just click on the link! (At least, that's how the changelog works.) Also, I added a comment in the source code that explains the why.

Walter Bright Wrote:

> Sean Kelly wrote:
> > Walter Bright Wrote:
> > 
> >> Sean Kelly wrote:
> >>> Leandro Lucarella Wrote:
> >>> 
> >>>> Bane, el 11 de diciembre a las 06:00 me escribiste:
> >>>>> Bug with GC fullCollect() in multithreaded environment.
> >>>>> Grauzone explained it here
> >>>>> http://www.digitalmars.com/webnews/newsgroups.php?art_group=digitalmars.D&article_id=99610
> >>>>> 
> >>>>> 
> >>> To be perfectly safe, it's a tiny bit trickier than that.  You have to always lock on allocations instead of just locking the collection cycle in single-threaded apps.  I think the reason for this is that once the lock is acquired it must be held until the current thread exits the GC code, and the "synchronized" statement only allows you to lock for the length of the collect() call (because the start and end must be within the same scope). However, this /may/ actually be a separate bug with the D 1.0 GC. I really can't recall for sure if this was an issue with collecting when all the other threads were suspended.
> >> Yes, the lock on the gc must still be held until the collection work is completed, but the other threads can be unfrozen once the mark phase is complete.
> > 
> > Hm... so if a dtor creates a new thread then that thread could enter and "lock" the GC while the first thread is still completing its allocation.  Couldn't this cause problems?
> 
> No, because the thread doing the GC collection has locked the GC. Any other thread attempting to alloc will wait on that lock. There is only one GC lock.

Yeah, but it releases the lock when the collection completes, and the it retries the allocation to actually obtain memory.  If the lock were held until the thread exited the GC code completely there wouldn't be a problem.  I've thought about switching to using a Mutex in D2 so the lock could be acquired at the collection and released at the end of GC.malloc(), but this isn't an option in D1.

Sean Kelly wrote:
> Walter Bright Wrote:
>> No, because the thread doing the GC collection has locked the GC.
>> Any other thread attempting to alloc will wait on that lock. There
>> is only one GC lock.
> 
> Yeah, but it releases the lock when the collection completes, and the
> it retries the allocation to actually obtain memory.  If the lock
> were held until the thread exited the GC code completely there
> wouldn't be a problem.

I don't understand why there is a problem with this. When the collection is complete, release lock, then get lock again for allocation, if the allocation fails, then go for another collection.

Walter Bright Wrote:

> Sean Kelly wrote:
> > Walter Bright Wrote:
> >> No, because the thread doing the GC collection has locked the GC. Any other thread attempting to alloc will wait on that lock. There is only one GC lock.
> > 
> > Yeah, but it releases the lock when the collection completes, and the it retries the allocation to actually obtain memory.  If the lock were held until the thread exited the GC code completely there wouldn't be a problem.
> 
> I don't understand why there is a problem with this. When the collection is complete, release lock, then get lock again for allocation, if the allocation fails, then go for another collection.

Ah, I didn't realize the allocation call backed up that far after the collection.  It's been a while since I looked at the code.

Walter Bright, el 12 de diciembre a las 10:29 me escribiste:
> Leandro Lucarella wrote:
> >If you could add a small description of what the commit is about besides pointing the NG discussion/Bugzilla number, as you are doing with DMD now, it would be really great! =)
> 
> Just click on the link! (At least, that's how the changelog works.) Also, I added a comment in the source code that explains the why.

There are no links in the commit messages, and my RSS feed gets only the commit message, not the diff, so to actually see the diff I have to open the RSS "link" to go to the full changeset information. So, if there are 10 changes, I have to open 10 new web pages to see what the changes are about. It would be nice to know what the changes are about *before* opening any new web pages, so I can only click in the changes I'm interested in seeing fully.

Thanks.

-- 
Leandro Lucarella (AKA luca)                     http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145  104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
Si pusieramos la máquina de cortar boludos dentro de la máquina del tiempo
y se pusiera a cortar boludos históricos con retroactividad,
otra sería la historieta hoy.
	-- Tato Bores