www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - D1 garbage collector + threads + malloc = garbage?

reply Bane <branimir.milosavljevic gmail.com> writes:
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

Test case that freezes app (tested on 1.053):


import std.c.stdlib;
import std.stdio;
import std.gc;
import std.thread;
import std.c.time;


class Foo {
  void* p;
  this(){
    synchronized(this.classinfo) {
      writefln("malloc begin");
      p = std.c.stdlib.malloc(1024*1024);
      writefln("malloc finished");
    }
  }

  ~this(){
    synchronized(this.classinfo){
      writefln("free begin");
      std.c.stdlib.free(p);
      writefln("free finished");
    }
  }
}

class MyThread : Thread {
  int run(){
    while(true){
      new Foo;
      msleep(1);
    }
    return 0;
  }
}

void main(){
  for(int i=0; i<10; i++){
    (new MyThread).start;
  }
  
  while(true){
    writefln("collect begin");
    std.gc.fullCollect;
    writefln("collect finished");
    msleep(1000);
  }
}



Can it be fixed or adleast mentioned somewhere in docs as known limitation of
D1 GC? It would save a lot of time to some people (too late for me :).
Dec 11 2009
next sibling parent reply Leandro Lucarella <llucax gmail.com> writes:
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
 
 Test case that freezes app (tested on 1.053):
 
 
 import std.c.stdlib;
 import std.stdio;
 import std.gc;
 import std.thread;
 import std.c.time;
 
 
 class Foo {
   void* p;
   this(){
     synchronized(this.classinfo) {
       writefln("malloc begin");
       p = std.c.stdlib.malloc(1024*1024);
       writefln("malloc finished");
     }
   }
 
   ~this(){
     synchronized(this.classinfo){
       writefln("free begin");
       std.c.stdlib.free(p);
       writefln("free finished");
     }
   }
 }
 
 class MyThread : Thread {
   int run(){
     while(true){
       new Foo;
       msleep(1);
     }
     return 0;
   }
 }
 
 void main(){
   for(int i=0; i<10; i++){
     (new MyThread).start;
   }
   
   while(true){
     writefln("collect begin");
     std.gc.fullCollect;
     writefln("collect finished");
     msleep(1000);
   }
 }
 
 
 
 Can it be fixed or adleast mentioned somewhere in docs as known limitation of
D1 GC? It would save a lot of time to some people (too late for me :).

The patch is very simple (as pointed by Grauzone), I guess it should make it into D1 (I hope): ------------------------------------------------------------ diff --git a/phobos/internal/gc/gcx.d b/phobos/internal/gc/gcx.d index b0d2821..7b1a7a3 100644 --- a/phobos/internal/gc/gcx.d +++ b/phobos/internal/gc/gcx.d -2033,8 +2033,6 struct Gcx } } - Thread.resumeAll(); - // Free up everything not marked debug(COLLECT_PRINTF) printf("\tfree'ing\n"); size_t freedpages = 0; -2194,6 +2192,8 struct Gcx debug(COLLECT_PRINTF) printf("recovered pages = %d\n", recoveredpages); debug(COLLECT_PRINTF) printf("\tfree'd %u bytes, %u pages from %u pools\ + Thread.resumeAll(); + return freedpages + recoveredpages; } ------------------------------------------------------------ I wanted to reproduce it to make a bug report to attach the patch but I couldn't. I replaced msleep() with usleep() (and multiplied the values by 1000) because I'm in Linux, but that's all I changed. I have an old AMD Athlon 1.8GHz. How do you reproduce it? I want to be sure the bug is present before the patch and fixed after the patch before submiting the patch via Bugzilla. Thanks! -- Leandro Lucarella (AKA luca) http://llucax.com.ar/ ---------------------------------------------------------------------- GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05) ---------------------------------------------------------------------- Nos retiramos hasta la semana que viene reflexionando sobre nuestras vidas: "Qué vida de mier'... Qué vida de mier'!" -- Sidharta Kiwi
Dec 11 2009
next sibling parent reply Sean Kelly <sean invisibleduck.org> writes:
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.
Dec 11 2009
parent reply Walter Bright <newshound1 digitalmars.com> writes:
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.
Dec 11 2009
parent reply Sean Kelly <sean invisibleduck.org> writes:
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?
Dec 12 2009
parent reply Walter Bright <newshound1 digitalmars.com> writes:
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
 
 


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.

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.
Dec 12 2009
parent reply Sean Kelly <sean invisibleduck.org> writes:
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
 
 


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.

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.
Dec 12 2009
parent reply Walter Bright <newshound1 digitalmars.com> writes:
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.
Dec 12 2009
parent Sean Kelly <sean invisibleduck.org> writes:
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.
Dec 12 2009
prev sibling next sibling parent reply Walter Bright <newshound1 digitalmars.com> writes:
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.
Dec 11 2009
next sibling parent Bane <branimir.milosavljevic gmail.com> writes:
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.
Dec 12 2009
prev sibling parent Walter Bright <newshound1 digitalmars.com> writes:
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.
Dec 12 2009
prev sibling next sibling parent Bane <branimir.milosavljevic gmail.com> writes:
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
 
 Test case that freezes app (tested on 1.053):
 
 
 import std.c.stdlib;
 import std.stdio;
 import std.gc;
 import std.thread;
 import std.c.time;
 
 
 class Foo {
   void* p;
   this(){
     synchronized(this.classinfo) {
       writefln("malloc begin");
       p = std.c.stdlib.malloc(1024*1024);
       writefln("malloc finished");
     }
   }
 
   ~this(){
     synchronized(this.classinfo){
       writefln("free begin");
       std.c.stdlib.free(p);
       writefln("free finished");
     }
   }
 }
 
 class MyThread : Thread {
   int run(){
     while(true){
       new Foo;
       msleep(1);
     }
     return 0;
   }
 }
 
 void main(){
   for(int i=0; i<10; i++){
     (new MyThread).start;
   }
   
   while(true){
     writefln("collect begin");
     std.gc.fullCollect;
     writefln("collect finished");
     msleep(1000);
   }
 }
 
 
 
 Can it be fixed or adleast mentioned somewhere in docs as known limitation of
D1 GC? It would save a lot of time to some people (too late for me :).

The patch is very simple (as pointed by Grauzone), I guess it should make it into D1 (I hope): ------------------------------------------------------------ diff --git a/phobos/internal/gc/gcx.d b/phobos/internal/gc/gcx.d index b0d2821..7b1a7a3 100644 --- a/phobos/internal/gc/gcx.d +++ b/phobos/internal/gc/gcx.d -2033,8 +2033,6 struct Gcx } } - Thread.resumeAll(); - // Free up everything not marked debug(COLLECT_PRINTF) printf("\tfree'ing\n"); size_t freedpages = 0; -2194,6 +2192,8 struct Gcx debug(COLLECT_PRINTF) printf("recovered pages = %d\n", recoveredpages); debug(COLLECT_PRINTF) printf("\tfree'd %u bytes, %u pages from %u pools\ + Thread.resumeAll(); + return freedpages + recoveredpages; } ------------------------------------------------------------ I wanted to reproduce it to make a bug report to attach the patch but I couldn't. I replaced msleep() with usleep() (and multiplied the values by 1000) because I'm in Linux, but that's all I changed. I have an old AMD Athlon 1.8GHz. How do you reproduce it? I want to be sure the bug is present before the patch and fixed after the patch before submiting the patch via Bugzilla. Thanks! -- Leandro Lucarella (AKA luca) http://llucax.com.ar/ ---------------------------------------------------------------------- GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05) ---------------------------------------------------------------------- Nos retiramos hasta la semana que viene reflexionando sobre nuestras vidas: "Qué vida de mier'... Qué vida de mier'!" -- Sidharta Kiwi

Home computer I tested it in is winxp with phenom quad core, but this bug is confirmed with centos 5.2 + intel dual core and centos 5.2 + intel xeon. Bug is present for a long time, thats for sure, It took me a while to track it because I thought its just me abusing keyboard, not a bug in phobos. Worst thing with this bug is that there is no output or exception when it happens - just that dreaded deadlock :) Once thing I noted - if you reduce memory required in that malloc call from 1 MB to 1 KB or less (or increase sleep time between calls), chances for bug to occur drop drastically, so in most applications this probably will never happen.
Dec 12 2009
prev sibling parent Bane <branimir.milosavljevic gmail.com> writes:
 I wanted to reproduce it to make a bug report to attach the patch but
 I couldn't. I replaced msleep() with usleep() (and multiplied the values
 by 1000) because I'm in Linux, but that's all I changed. I have an old AMD
 Athlon 1.8GHz. How do you reproduce it?
 
 I want to be sure the bug is present before the patch and fixed after the
 patch before submiting the patch via Bugzilla.
 
 Thanks!

I forgot: for linux just remove msleep lines completely. It will deadlock soon after program start. Seems that linux is more resistant to this bug than windows.
Dec 12 2009
prev sibling next sibling parent Leandro Lucarella <llucax gmail.com> writes:
Walter Bright, el 11 de diciembre a las 20:51 me escribiste:
 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.

Great, thanks! -- Leandro Lucarella (AKA luca) http://llucax.com.ar/ ---------------------------------------------------------------------- GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05) ---------------------------------------------------------------------- You should've seen her face. It was the exact same look my father gave me when I told him I wanted to be a ventriloquist. -- George Constanza
Dec 12 2009
prev sibling next sibling parent Leandro Lucarella <llucax gmail.com> writes:
Leandro Lucarella, el 12 de diciembre a las 11:18 me escribiste:
 Walter Bright, el 11 de diciembre a las 20:51 me escribiste:
 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.

Great, thanks!

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! =) -- Leandro Lucarella (AKA luca) http://llucax.com.ar/ ---------------------------------------------------------------------- GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145 104C 949E BFB6 5F5A 8D05) ---------------------------------------------------------------------- Le pedí que me enseñe a usar el mouse Pero solo quiere hablarme del Bauhaus Le pregunté si era chorra o rockera Me dijo "Gertrude Stein era re-tortillera" Me hizo mucho mal la cumbiera intelectual
Dec 12 2009
prev sibling parent Leandro Lucarella <llucax gmail.com> writes:
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
Dec 13 2009