www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Dynamic array leak?

reply bitwise <bitwise.pvt gmail.com> writes:
struct S {
     static int count = 0;
     this(int x) { ++count; }
     this(this) { ++count; }
     ~this() { --count; }
}

int main(string[] argv)
{
     S[] x = [S(1), S(1)];
     writeln("GC allocated: ", (GC.addrOf(x.ptr) !is null));
     x = null;
     GC.collect();
     writeln("live objects: ", S.count);
     return 0;
}

output:
GC allocated: true
live objects: 2

expected:
GC allocated: true
live objects: 0

Is this a bug?

I thought that the first writeln() may be leaving a copy of the 
pointer lingering on the stack somewhere, but the output is still 
"live objects: 2" with that line commented out.

   Thanks
Aug 11 2017
next sibling parent reply Yuxuan Shui <yshuiv7 gmail.com> writes:
On Friday, 11 August 2017 at 18:44:56 UTC, bitwise wrote:
 struct S {
     static int count = 0;
     this(int x) { ++count; }
     this(this) { ++count; }
     ~this() { --count; }
 }

 int main(string[] argv)
 {
     S[] x = [S(1), S(1)];
     writeln("GC allocated: ", (GC.addrOf(x.ptr) !is null));
     x = null;
     GC.collect();
     writeln("live objects: ", S.count);
     return 0;
 }

 output:
 GC allocated: true
 live objects: 2

 expected:
 GC allocated: true
 live objects: 0

 Is this a bug?

 I thought that the first writeln() may be leaving a copy of the 
 pointer lingering on the stack somewhere, but the output is 
 still "live objects: 2" with that line commented out.

   Thanks
My guess is a pointer to the array still lives somewhere on the stack. This gives the expected output: void f() { S[] x = [S(1), S(1)]; writeln("GC allocated: ", (GC.addrOf(x.ptr) !is null)); x = null; } int main(string[] argv) { f(); GC.collect(); writeln("live objects: ", S.count); return 0; }
Aug 11 2017
parent reply bitwise <bitwise.pvt gmail.com> writes:
On Friday, 11 August 2017 at 19:01:44 UTC, Yuxuan Shui wrote:
 On Friday, 11 August 2017 at 18:44:56 UTC, bitwise wrote:
 [...]

 My guess is a pointer to the array still lives somewhere on the 
 stack. This gives the expected output:

 void f()
 {
     S[] x = [S(1), S(1)];
     writeln("GC allocated: ", (GC.addrOf(x.ptr) !is null));
     x = null;
 }

 int main(string[] argv)
 {
     f();
     GC.collect();
     writeln("live objects: ", S.count);
     return 0;
 }
Makes sense. I was uncommenting unit tests one-by-one after making some changes when I triggered this. I guess they were passing before because subsequent unit tests cleared the pointers off the stack. I guess I can just call a function that allocates a large zeroed-out array on the stack in the last unit test before checking the count if this happens again. Thanks
Aug 11 2017
parent reply Temtaime <temtaime gmail.com> writes:
On Friday, 11 August 2017 at 22:36:27 UTC, bitwise wrote:
 On Friday, 11 August 2017 at 19:01:44 UTC, Yuxuan Shui wrote:
 On Friday, 11 August 2017 at 18:44:56 UTC, bitwise wrote:
 [...]

 My guess is a pointer to the array still lives somewhere on 
 the stack. This gives the expected output:

 void f()
 {
     S[] x = [S(1), S(1)];
     writeln("GC allocated: ", (GC.addrOf(x.ptr) !is null));
     x = null;
 }

 int main(string[] argv)
 {
     f();
     GC.collect();
     writeln("live objects: ", S.count);
     return 0;
 }
Makes sense. I was uncommenting unit tests one-by-one after making some changes when I triggered this. I guess they were passing before because subsequent unit tests cleared the pointers off the stack. I guess I can just call a function that allocates a large zeroed-out array on the stack in the last unit test before checking the count if this happens again. Thanks
Collect - is a hit to the GC, not an order. It can ignore this request. Also do not rely on the gc calling a dtor - it is not safe and can be called totally randomed, so use RC instead or expicit destroy()
Aug 12 2017
next sibling parent reply bitwise <bitwise.pvt gmail.com> writes:
On Saturday, 12 August 2017 at 08:16:56 UTC, Temtaime wrote:
 Collect - is a hint to the GC, not an order. It can ignore this 
 request.
If this is the case, then D's GC should have an option to force https://msdn.microsoft.com/en-us/library/bb495757(v=vs.110).aspx
 Also do not rely on the gc calling a dtor - it is not safe and 
 can be called totally randomed, so use RC instead or expicit 
 destroy()
RC is not applicable. I'm doing unit tests for a non-GC container and trying to make sure all destructors are called properly. Example: unittest { auto a = List!int([S(0), S(1), S(2)]); a.popBack(); assert(equal(a[], [S(0), S(1)])); } // lots of similar unittests unittest { import std.stdio; GC.collect(); assert(S.count == 0); } So if all goes well, S.count should be zero, but the arrays I'm testing against are being allocated on the heap. Given the conditions of the tests, it seems like GC.collect should be able to reclaim those arrays after the unit tests have exited, and in most cases does. The ideal solution though, would be to allocate those arrays on the stack and avoid the problem altogether. There doesn't seem to be any reasonable way to do it though. // won't this allocate anyways? S[2] b = [S(0), S(1)]; assert(equal(a[], b[])); // why can't I just declare a static array inline? assert(equal(a[], int[2]{ S(0), S(1) }));
Aug 12 2017
parent reply Dgame <r.schuett.1987 gmail.com> writes:
On Saturday, 12 August 2017 at 17:25:36 UTC, bitwise wrote:
 On Saturday, 12 August 2017 at 08:16:56 UTC, Temtaime wrote:
 Collect - is a hint to the GC, not an order. It can ignore 
 this request.
If this is the case, then D's GC should have an option to force https://msdn.microsoft.com/en-us/library/bb495757(v=vs.110).aspx
 Also do not rely on the gc calling a dtor - it is not safe and 
 can be called totally randomed, so use RC instead or expicit 
 destroy()
RC is not applicable. I'm doing unit tests for a non-GC container and trying to make sure all destructors are called properly. Example: unittest { auto a = List!int([S(0), S(1), S(2)]); a.popBack(); assert(equal(a[], [S(0), S(1)])); } // lots of similar unittests unittest { import std.stdio; GC.collect(); assert(S.count == 0); } So if all goes well, S.count should be zero, but the arrays I'm testing against are being allocated on the heap. Given the conditions of the tests, it seems like GC.collect should be able to reclaim those arrays after the unit tests have exited, and in most cases does. The ideal solution though, would be to allocate those arrays on the stack and avoid the problem altogether. There doesn't seem to be any reasonable way to do it though. // won't this allocate anyways? S[2] b = [S(0), S(1)]; assert(equal(a[], b[])); // why can't I just declare a static array inline? assert(equal(a[], int[2]{ S(0), S(1) }));
auto s(T, size_t n)(T[n] values) { return values; } assert(equal(a[], [S(0), S(1)].s));
Aug 12 2017
parent bitwise <bitwise.pvt gmail.com> writes:
On Saturday, 12 August 2017 at 17:52:47 UTC, Dgame wrote:
 [...]

 auto s(T, size_t n)(T[n] values) {
 	return values;
 }

 assert(equal(a[], [S(0), S(1)].s));
This seems to work, but I'm trying to determine if it's 100% guaranteed safe. Tacking on nogc doesn't seem to stop it from working, so that checks out, but my example print's "postblit" three times: struct S { int x; this(int x) nogc { this.x = x; } this(this) nogc { printf("postblit\n"); } } auto s(T, size_t n)(T[n] values) nogc { return values; } void main(string[] args) nogc { foreach(ref s; [S(0), S(1), S(2)].s) printf("%d\n", s.x); } So I think what's happening is that the array is moved into the argument of s(), then copied to the return value, which resides on the parent stack frame of s(), meaning that it's safe from the body of the foreach clobbering it...is that correct? Assuming this is safe though, it would be nice to eliminate the postblits too. It seems like it should be move-move instead of a move-copy. I tried "return move(values);" but that still didn't help. Thanks
Aug 12 2017
prev sibling next sibling parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/12/17 4:16 AM, Temtaime wrote:
 Collect - is a hit to the GC, not an order. It can ignore this request.
Hm.. where is this documented? I agree that the GC may not clean up all memory that could be legitimately cleaned up, but that's not because it didn't do a collection. -Steve
Aug 14 2017
prev sibling parent Meta <jared771 gmail.com> writes:
On Saturday, 12 August 2017 at 08:16:56 UTC, Temtaime wrote:
 Collect - is a hit to the GC, not an order. It can ignore this 
 request.
As far as I can tell, it is an order. From the GC code[1]: void collect() nothrow { fullCollect(); } ... size_t fullCollect() nothrow { debug(PRINTF) printf("GC.fullCollect()\n"); // Since a finalizer could launch a new thread, we always need to lock // when collecting. static size_t go(Gcx* gcx) nothrow { return gcx.fullcollect(); } immutable result = runLocked!go(gcx); version (none) { GCStats stats; getStats(stats); debug(PRINTF) printf("heapSize = %zx, freeSize = %zx\n", stats.heapSize, stats.freeSize); } gcx.log_collect(); return result; } ... //Gcx.fullcollect size_t fullcollect(bool nostack = false) nothrow { MonoTime start, stop, begin; if (config.profile) { begin = start = currTime; } debug(COLLECT_PRINTF) printf("Gcx.fullcollect()\n"); //printf("\tpool address range = %p .. %p\n", minAddr, maxAddr); { // lock roots and ranges around suspending threads b/c they're not reentrant safe rangesLock.lock(); rootsLock.lock(); scope (exit) { rangesLock.unlock(); rootsLock.unlock(); } thread_suspendAll(); prepare(); if (config.profile) { stop = currTime; prepTime += (stop - start); start = stop; } markAll(nostack); thread_processGCMarks(&isMarked); thread_resumeAll(); } if (config.profile) { stop = currTime; markTime += (stop - start); Duration pause = stop - begin; if (pause > maxPauseTime) maxPauseTime = pause; start = stop; } ConservativeGC._inFinalizer = true; size_t freedLargePages=void; { scope (failure) ConservativeGC._inFinalizer = false; freedLargePages = sweep(); ConservativeGC._inFinalizer = false; } if (config.profile) { stop = currTime; sweepTime += (stop - start); start = stop; } immutable freedSmallPages = recover(); if (config.profile) { stop = currTime; recoverTime += (stop - start); ++numCollections; } updateCollectThresholds(); return freedLargePages + freedSmallPages; } 1. https://github.com/dlang/druntime/blob/master/src/gc/impl/conservative/gc.d
Aug 14 2017
prev sibling parent Marco Leise <Marco.Leise gmx.de> writes:
Am Fri, 11 Aug 2017 18:44:56 +0000
schrieb bitwise <bitwise.pvt gmail.com>:

 [=E2=80=A6]
That can't work and here is why: Druntime employs a conservative GC that will treat several things as potential pointers to GC memory. From the top of my head, the entire stack as well as void[] arrays and unions that contain pointers. Some integer variable on the stack or a chunk of a void[] that happens to have the same value as your GC pointer will keep it alive. Same for a union of an integer and a pointer where you have set the integer part to the address of your GC memory chunk. These misidentifications (false pointers) can become a real problem on 32-bit systems, where due to the small address space many things can look like valid pointers and keep GC memory alive that should long since have been recycled. P.S.: Also keep in mind that if you were to run multi-threaded, the ptr you test for could have been recycled and reassigned between GC.collect() and GC.addrOf(). Some unittesting frameworks for example run the tests in parallel. --=20 Marco
Aug 12 2017