www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Memory leak in foreach

reply Andrea Fontana <nospam example.org> writes:
This subtle bug[1] and its memory leak are still here because my 
pull request[2] had no success.

It worths noting that since each() in std.algorithm uses a 
foreach+ref to iterate  structs[3], this memory leak probably is 
silently affecting not only me :)

I hope someone could fix this.

[1] https://issues.dlang.org/show_bug.cgi?id=11934
[2] https://github.com/dlang/dmd/pull/8437
[3] 
https://github.com/dlang/phobos/blob/master/std/algorithm/iteration.d#L983
Feb 20
next sibling parent reply aliak <something something.com> writes:
On Wednesday, 20 February 2019 at 09:31:30 UTC, Andrea Fontana 
wrote:
 This subtle bug[1] and its memory leak are still here because 
 my pull request[2] had no success.

 It worths noting that since each() in std.algorithm uses a 
 foreach+ref to iterate  structs[3], this memory leak probably 
 is silently affecting not only me :)

 I hope someone could fix this.

 [1] https://issues.dlang.org/show_bug.cgi?id=11934
 [2] https://github.com/dlang/dmd/pull/8437
 [3] 
 https://github.com/dlang/phobos/blob/master/std/algorithm/iteration.d#L983
Does that patch ignore the ref when you specify it in a foreach loop? If it does, then that sounds like it should be a compiler error and not ignored. A compiler should not change the meaning of my code silently just to make compilation work. We have javascript for that no? Or did I misunderstand the patch?
Feb 20
parent reply Andrea Fontana <nospam example.org> writes:
On Wednesday, 20 February 2019 at 10:50:50 UTC, aliak wrote:
 On Wednesday, 20 February 2019 at 09:31:30 UTC, Andrea Fontana 
 wrote:
 This subtle bug[1] and its memory leak are still here because 
 my pull request[2] had no success.

 It worths noting that since each() in std.algorithm uses a 
 foreach+ref to iterate  structs[3], this memory leak probably 
 is silently affecting not only me :)

 I hope someone could fix this.

 [1] https://issues.dlang.org/show_bug.cgi?id=11934
 [2] https://github.com/dlang/dmd/pull/8437
 [3] 
 https://github.com/dlang/phobos/blob/master/std/algorithm/iteration.d#L983
Does that patch ignore the ref when you specify it in a foreach loop? If it does, then that sounds like it should be a compiler error and not ignored. A compiler should not change the meaning of my code silently just to make compilation work. We have javascript for that no? Or did I misunderstand the patch?
Let me explain better. Currently this is what is happening (if I'm right!): - You have a range with struct elements - You try to iterate this range using a foreach+ref. - This range returns a copy of each element (a temporary value) - You work on this copy by ref (it doesn't even make sense to use a ref to a temporary value) - Compiler doens't free this copy because it thinks it's a reference! <- leak My idea: fix the current behaviour, freeing the elements created and then fix the language with a warning/error/deprecation/DIP/whatever Please consider that throwing an error is going to brake all code using (for example) each() to iterate a range returning a struct, probably. Should it throw an error? Don't know. Probably yes. But I feel it could add a lot of complexity to code: if you're writing generic code you have always to check if you're iterating a ref-returning range or not. In first case you can use foreach(ref) in the second case you have to use foreach(). Anyway I think it's still better to fix current behaviour and waiting for a decision than leaving this leak open for who knows how long...
Feb 20
next sibling parent reply "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
On Wed, Feb 20, 2019 at 02:22:51PM +0000, Andrea Fontana via Digitalmars-d
wrote:
[...]
 Currently this is what is happening (if I'm right!):
 - You have a range with struct elements
 - You try to iterate this range using a foreach+ref.
 - This range returns a copy of each element (a temporary value)
 - You work on this copy by ref (it doesn't even make sense to use a ref to a
 temporary value)
 - Compiler doens't free this copy because it thinks it's a reference! <-
 leak
Confirmed: if I have a range of structs that have a dtor, then foreach over the range (without `ref`) correctly calls the dtor for each returned struct, but foreach with `ref` fails to invoke the dtor for the returned struct, even though .front is returning by value. I think this is a bug. It should be fixed, and you shouldn't need to do any deprecation cycle because not calling the dtor is wrong behaviour that fails to implement the language spec.
 My idea: fix the current behaviour, freeing the elements created and
 then fix the language with a warning/error/deprecation/DIP/whatever
The dtor should be called for structs that have a dtor. Classes are supposed to be managed by the GC and shouldn't need any change. No warning/deprecation needs to be done because this is a bug. To be more precise, if .front returns by value a struct that has a dtor, then it needs to be destructed whether or not `foreach(ref...)` is used. If .front returns something by reference, then it should NOT be destructed regardless of whether you use `foreach` or `foreach(ref...)`.
 Please consider that throwing an error is going to brake all code
 using (for example) each() to iterate a range returning a struct,
 probably.
No, it should not throw anything.
 Should it throw an error? Don't know. Probably yes. But I feel it
 could add a lot of complexity to code: if you're writing generic code
 you have always to check if you're iterating a ref-returning range or
 not. In first case you can use foreach(ref) in the second case you
 have to use foreach().
No, it shouldn't throw an error. All that's needed is to invoke the dtor when it needs to be invoked, that's all. Generic code shouldn't have to care whether or not the dtor is invoked -- that's the whole point of the user type defining a dtor; callers shouldn't have to know how to destroy the type themselves, the type should handle it. Similarly, generic code shouldn't care whether there is a dtor; the language should invoke the dtor where necessary.
 Anyway I think it's still better to fix current behaviour and waiting
 for a decision than leaving this leak open for who knows how long...
[...] The fix is just to invoke the dtor based on whether .front returns by ref or not, rather than whether `foreach(ref)` was used. It's a different question of whether it should be a compile error to use `foreach(ref)` on a range whose .front doesn't return by ref. That's a much more sticky issue that you may not want to get into. :-D Fix the dtor bug first, then we can decide whether we want to pursue this question. T -- To provoke is to call someone stupid; to argue is to call each other stupid.
Feb 20
parent reply Andrea Fontana <nospam example.org> writes:
On Wednesday, 20 February 2019 at 15:52:06 UTC, H. S. Teoh wrote:
 It's a different question of whether it should be a compile 
 error to use `foreach(ref)` on a range whose .front doesn't 
 return by ref.  That's a much more sticky issue that you may 
 not want to get into. :-D  Fix the dtor bug first, then we can 
 decide whether we want to pursue this question.
Eh, you summed up my idea perfectly. Isn't exactly what my pull request is doing? Andrea
Feb 20
parent Andrea Fontana <nospam example.org> writes:
On Wednesday, 20 February 2019 at 16:16:27 UTC, Andrea Fontana 
wrote:
 On Wednesday, 20 February 2019 at 15:52:06 UTC, H. S. Teoh 
 wrote:
 It's a different question of whether it should be a compile 
 error to use `foreach(ref)` on a range whose .front doesn't 
 return by ref.  That's a much more sticky issue that you may 
 not want to get into. :-D  Fix the dtor bug first, then we can 
 decide whether we want to pursue this question.
Eh, you summed up my idea perfectly. Isn't exactly what my pull request is doing? Andrea
(My pr ignores ref for that case so dtor is called and memory freed)
Feb 20
prev sibling next sibling parent reply "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
P.S. And BTW, this bug is not directly related to memory leaks. It
relates to the dtor not being called when it should be.  Whether or not
it's a memory leak depends on what the dtor is doing (e.g. it may be
leaking other resources than memory).  In your PR / bugzilla issue you
should probably phrase it in terms of dtors rather than memory leaks,
which may cause misunderstandings.


T

-- 
What is Matter, what is Mind? Never Mind, it doesn't Matter.
Feb 20
parent Andrea Fontana <nospam example.org> writes:
On Wednesday, 20 February 2019 at 15:53:54 UTC, H. S. Teoh wrote:
 P.S. And BTW, this bug is not directly related to memory leaks. 
 It relates to the dtor not being called when it should be.  
 Whether or not it's a memory leak depends on what the dtor is 
 doing (e.g. it may be leaking other resources than memory).  In 
 your PR / bugzilla issue you should probably phrase it in terms 
 of dtors rather than memory leaks, which may cause 
 misunderstandings.


 T
You're right. Indeed I found this bug checking for non-released resources.
Feb 20
prev sibling parent aliak <something something.com> writes:
On Wednesday, 20 February 2019 at 14:22:51 UTC, Andrea Fontana 
wrote:
 Anyway I think it's still better to fix current behaviour and 
 waiting for a decision than leaving this leak open for who 
 knows how long...
Yeah I guess you're right. Makes sense! Thanks :)
Feb 22
prev sibling parent reply Dein <dwin d.com> writes:
On Wednesday, 20 February 2019 at 09:31:30 UTC, Andrea Fontana 
wrote:
 This subtle bug[1] and its memory leak are still here because 
 my pull request[2] had no success.

 It worths noting that since each() in std.algorithm uses a 
 foreach+ref to iterate  structs[3], this memory leak probably 
 is silently affecting not only me :)

 I hope someone could fix this.

 [1] https://issues.dlang.org/show_bug.cgi?id=11934
 [2] https://github.com/dlang/dmd/pull/8437
 [3] 
 https://github.com/dlang/phobos/blob/master/std/algorithm/iteration.d#L983
That's not a memory leak, you are creating a GC array, the GC might never free the array its not deterministic. Use nogc and staticArray() if you want to destructors to be called deterministically.
Feb 20
next sibling parent Andrea Fontana <nospam example.org> writes:
On Wednesday, 20 February 2019 at 11:05:31 UTC, Dein wrote:
 That's not a memory leak, you are creating a GC array, the GC 
 might never free the array its not deterministic. Use  nogc and 
 staticArray() if you want to destructors to be called 
 deterministically.
No: GC will not free those elements in this case. There's a bug. The only way to free those elements is to kill the application. GC doesn't track them at all.
Feb 20
prev sibling parent Andrea Fontana <nospam example.org> writes:
On Wednesday, 20 February 2019 at 11:05:31 UTC, Dein wrote:
 On Wednesday, 20 February 2019 at 09:31:30 UTC, Andrea Fontana 
 wrote:
 This subtle bug[1] and its memory leak are still here because 
 my pull request[2] had no success.

 It worths noting that since each() in std.algorithm uses a 
 foreach+ref to iterate  structs[3], this memory leak probably 
 is silently affecting not only me :)

 I hope someone could fix this.

 [1] https://issues.dlang.org/show_bug.cgi?id=11934
 [2] https://github.com/dlang/dmd/pull/8437
 [3] 
 https://github.com/dlang/phobos/blob/master/std/algorithm/iteration.d#L983
That's not a memory leak, you are creating a GC array, the GC might never free the array its not deterministic. Use nogc and staticArray() if you want to destructors to be called deterministically.
If you check the example you will notice that for each element inside the array is created a copy by foreach. Those temporary copies are never freed (bug). So each time you are going to iterate through your array (using ref) you are filling memory with additional copies of your elements.
Feb 20