https://issues.dlang.org/show_bug.cgi?id=21550

On Friday, 15 January 2021 at 01:28:55 UTC, Imperatorn wrote:
> https://issues.dlang.org/show_bug.cgi?id=21550

From that issue, it's surprising that delete frees array memory even when the ptr field isn't the start of the allocation. I would've guessed calling a GC function to look up the start of the allocation would be needed first. That function might be useful in other contexts too.

On Friday, 15 January 2021 at 13:15:12 UTC, Nick Treleaven wrote:
> On Friday, 15 January 2021 at 01:28:55 UTC, Imperatorn wrote:
>> https://issues.dlang.org/show_bug.cgi?id=21550
>
> From that issue, it's surprising that delete frees array memory even when the ptr field isn't the start of the allocation. I would've guessed calling a GC function to look up the start of the allocation would be needed first. That function might be useful in other contexts too.

Yeah, I wonder how many use this new approach. They would suffer in silence 🤐

Should be an easy fix tho. Could maybe get it into the next upcoming release even.

On Friday, 15 January 2021 at 13:15:12 UTC, Nick Treleaven wrote:
> From that issue, it's surprising that delete frees array memory even when the ptr field isn't the start of the allocation.

while(true) {
  int[] a = new int[](whatever);
  delete a;
}

That will not run out of memory. Change it to `__delete(a);` or `GC.free(a.ptr)` like the docs tell you to do and it will.

It is true that if you slice a and delete it, it still blasts the whole block which might be surprising.... but like plain simple new/delete pair actually working certainly isn't surprising and plain simple new/__delete pair doing absolutely nothing makes the whole thing pretty worthless. Hence the bug report.

On 1/15/21 8:59 AM, Adam D. Ruppe wrote:
> On Friday, 15 January 2021 at 13:15:12 UTC, Nick Treleaven wrote:
>> From that issue, it's surprising that delete frees array memory even when the ptr field isn't the start of the allocation.
> 
> while(true) {
>    int[] a = new int[](whatever);
>    delete a;
> }
> 
> That will not run out of memory. Change it to `__delete(a);` or `GC.free(a.ptr)` like the docs tell you to do and it will.

To clarify, as long as whatever is larger than 2k, it will. And that's a bug for sure (the reason for this is based on the GC implementation).

> It is true that if you slice a and delete it, it still blasts the whole block which might be surprising.... but like plain simple new/delete pair actually working certainly isn't surprising and plain simple new/__delete pair doing absolutely nothing makes the whole thing pretty worthless. Hence the bug report.

I'm unsure if deleting from the middle is expected or desired.

I would first fix it to do what the delete call does, and then possibly "fix" that behavior, as I wouldn't expect it either. If you pass a slice into a function, that function probably shouldn't affect everything in the block from that slice.

But clearly a bug in the first respect.

-Steve

On Friday, 15 January 2021 at 14:38:55 UTC, Steven Schveighoffer wrote:
> I'm unsure if deleting from the middle is expected or desired.

Yeah, in my private implementation, I have a sanity check that the pointer and the base aren't too far apart. I figured they didn't line up due to a capacity cache or similar metadata at the beginning of the block (though I didn't prove this) and thus the pointers shouldn't be more than like 16 bytes apart and anything more than that is suspicious.

So far in my tests that appears to be exactly the case - 16 bytes on both 32 and 64 bit.

But while that might be good enough for my barely-documented "WARNING: calling this function might break your code" thing, idk if that is the right answer for druntime itself.

However if I can prove it is correct, I do think it is reasonable to go back to silently doing nothing if the pointers are further apart since freeing the whole block from a partial slice is a bit iffy in terms of correctness. You wouldn't do that with malloc.

On Saturday, 16 January 2021 at 04:52:05 UTC, Adam D. Ruppe wrote:
> On Friday, 15 January 2021 at 14:38:55 UTC, Steven Schveighoffer wrote:
>> [...]
>
> Yeah, in my private implementation, I have a sanity check that the pointer and the base aren't too far apart. I figured they didn't line up due to a capacity cache or similar metadata at the beginning of the block (though I didn't prove this) and thus the pointers shouldn't be more than like 16 bytes apart and anything more than that is suspicious.
>
> [...]

Any way, this should be included in the next release imo.

It doesn't look good for D to have memory leaks when using the recommended alternative to delete...