Fixes for memory leaks on thread termination + refactoring

[dayllenger]

When a thread terminates, global malloc-allocated things remain. Furthermore, destructors of thread-local objects aren't invoked either!

Tested on serve-d using Valgrind Massif.

I hope you won't mind that I've done some refactoring and other stuff in the meantime.

[Hackerpilot]

I hope you won't mind that I've done some refactoring and other stuff in the meantime.

This way I need to review the refactoring and the memory management stuff at the same time.

[Hackerpilot]

Regarding the CI failure: I think we're past the point where depending on an old version of stdx.allocator makes sense.

[Hackerpilot]

Actually, the CI failure seems to be a bug in Phobos. The testAllocator template is declared inside of a version (StdUnittest) block, but not all of the tests that use testAllocator are inside of version (StdUnittest) block the way that they should be.

[Hackerpilot]

Related: dlang/phobos#7353

[Hackerpilot]

More strangeness: I'm not able to replicate the CI failure on my desktop. Never mind. Meson is using ldc even though I ran it with DC=dmd. I can confirm that this is a phobos bug.

[Hackerpilot]

Looks good to me, but I'm not sure what to do about the CI issue.

[dayllenger]

This is strange to me: why does meson ever compile Phobos unittests we don't import?

[Hackerpilot]

This is strange to me: why does meson ever compile Phobos unittests we don't import?

Not sure. I fixed this in dlang/phobos#7419 but that won't get the tests green until we either change the CI or a new DMD release is out.

[dayllenger]

Is it a just minor inconvenience or it blocks merging?

[Hackerpilot]

Is it a just minor inconvenience or it blocks merging?

I think that we can still merge. We'll have to watch the CI failures carefully until the next DMD release.

@WebFreak001 Can you take another look?

[WebFreak001]

I like how the changes look but I haven't actually tested the code yet.

Add a basic unittest for the thread dtor stuff pls (creating a thread, doing some stuff, destroying the thread, and then outside doing more stuff)

[dayllenger]

I have an idea to actually check for a memory leak:

version (linux)
unittest
{
    // ...imports

    // create and destroy a lot of dummy threads
    foreach (j; 0 .. 100)
    {
        Thread[100] arr;
        foreach (i; 0 .. 100)
            arr[i] = new Thread({}).start();
        foreach (i; 0 .. 100)
            arr[i].join();
    }

    GC.collect();
    GC.minimize();

    // get Linux process statistics
    const txt = fs.readText("/proc/self/stat");
    const parts = split(txt);
    // get the resident set size
    const rss = to!long(parts[23]);
    // check that it is less than some eyeballed threshold
    assert(rss < 10_000);
}

It spends 3 seconds to create and join 10000 threads, then checks if they inflated the process. This catches moderate leaks (> 4 kB). If threads allocate > 300 kB each, CI will kill the process ruthlessly.

[dayllenger]

It works!

4/4 dsymbol / test_dsymbol                  OK       3.65 s 

[WebFreak001]

well anyway lgtm now

[Hackerpilot]

I'm going to override the CI given that the failure is unrelated.

[WebFreak001]

actually using the API now I'm not sure if it was the best idea to make istring have opCmp now with ptr comparision. Before it would have defaulted to the string (through alias this) and sorted by ascii ordering instead of random memory order. So this definitely might have changed some existing code depending on the order of istrings.

[dayllenger]

It can be renamed to opCmpFast. There are 2 places in the library which use this operator. I'm busy right now, maybe I'll make a patch soon.

[WebFreak001]

hm not sure if that's the best solution either. Currently there are 2 releases made with it already too so there should be some solid plan that integrates well

[Hackerpilot]

We could at least start by documenting this in the release descriptions.

[WebFreak001]

ok actually it's actually changing in a few ways:

• std.algorithm:cmp no longer works with istring, because _data is private. For some reason even if doing alias this on the data getter, it doesn't work as long as _data is private (but suddenly begins to work on the getter when the variable is public) - this is better than silently breaking
• a<b still compiles but now changed behavior

[dayllenger]

I don't want breakages, but if you leave your fields public and so vulnerable - you will never be sure that your abstraction works everywhere as intended:

void f()
{
    g(FUNCTION_SYMBOL_NAME);
}
void g(ref string str)
{
    str = "fuck";
}

dcd and dscanner compile, so the breakage is not as bad.