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

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;
}

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

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()

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 collection like C#'s GC:
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) }));

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 collection like C#'s GC:
> 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));

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

Am Fri, 11 Aug 2017 18:44:56 +0000
schrieb bitwise <bitwise.pvt@gmail.com>:

> […]

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.


-- 
Marco

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

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