Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

variables in closure iterators loop are not correctly unassigned #19193

Closed
Menduist opened this issue Nov 25, 2021 · 2 comments · Fixed by #19195
Closed

variables in closure iterators loop are not correctly unassigned #19193

Menduist opened this issue Nov 25, 2021 · 2 comments · Fixed by #19195

Comments

@Menduist
Copy link
Contributor

Menduist commented Nov 25, 2021

Example

iterator test(): int {.closure.} =
  for i in 0..<1000000:
    var heavy: seq[int]
    heavy = newSeq[int](10000)
    yield heavy.len

GC_disableMarkAndSweep()

for i in test():
  discard

will use a lot of memory, because the lines:

var heavy: seq[int]
heavy = newSeq[int](10000)

will generate:

(*colonenvP_).heavy2 = (tySequence__qwqHTkRvwhrRyENtudHQ7g*)0;
asgnRef((void**) (&(*colonenvP_).heavy2), newSeq__eA9b5cYyFZe7gRm4F9aRTKlA(((NI) 10000)));

which erases the last reference before calling asgnRef.

Assigning directly avoids this issue:
var heavy = newSeq[int](10000)
Because the heavy2 = 0 line is not generated, and asgnRef will call decRef on the last value.

Possible Solution

It seems that in regular procs, the var line will be translated to:

asgnRef((void**) (&fut__JYhQvJxHhtC8fq7FzTRUVQ), NIM_NIL);
asgnRef((void**) (&fut__JYhQvJxHhtC8fq7FzTRUVQ), testInner__bhBT5he32hyARROogQacKg());

Since we assign to NIL instead of setting to 0 manually, the reference count is correctly updated

Additional Information

Happens both on 1.2.6 & devel.
Obviously, the mark & sweep will catch that, but as part of optimizing ram consumption in nimbus, we're trying to rely on it as little as possible

@Araq
Copy link
Member

Araq commented Nov 26, 2021

I cannot prioritize this bug, as a workaround exists and IMHO in order to optimize memory consumption you should compile with --gc:orc and 1.6.0 and run the sanitizers, esp. valgrind on it.

@arnetheduck
Copy link
Contributor

optimize memory consumption

our issue is not so much in finding memory issues, that's not too hard :) This issue is about addressing the ones we do find that are systemic with the default GC - all this while we wait for ORC to work well enough that we reliably can use it.

Araq pushed a commit that referenced this issue Dec 7, 2021
narimiran pushed a commit that referenced this issue Dec 7, 2021
(cherry picked from commit cd592ed)
narimiran pushed a commit that referenced this issue Dec 8, 2021
(cherry picked from commit cd592ed)
narimiran pushed a commit that referenced this issue Dec 10, 2021
(cherry picked from commit cd592ed)
PMunch pushed a commit to PMunch/Nim that referenced this issue Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants