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 2019
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 2019
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 2019
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 2019
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 2019
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 2019
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 2019
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 2019
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 2019
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 2019
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 2019
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 2019