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

sink paramers not moved into closure (refc) #22173

Open
arnetheduck opened this issue Jun 27, 2023 · 0 comments · Fixed by #22359
Open

sink paramers not moved into closure (refc) #22173

arnetheduck opened this issue Jun 27, 2023 · 0 comments · Fixed by #22359
Labels
refc refc issues

Comments

@arnetheduck
Copy link
Contributor

arnetheduck commented Jun 27, 2023

Description

proc fff(v: sink string): iterator(): char =
  return iterator(): char =
    for c in v:
      yield c

var tmp = newString(100*1024*1024)

let iter = fff(move(tmp))

var i = 0
while true:
  let v = iter()
  if finished(iter):
    break

  i+=1
  discard

echo i
echo GC_getStatistics()

The above program should take no more than 100mb + some dust - ie the v parameter clearly can be moved into the iterator but isn't.

The copy happens during environment construction here:

 T1_ = NIM_NIL;  T1_ = (*colonenv_).v1; (*colonenv_).v1 = copyStringRC1(v);      if (T1_) nimGCunrefNoCycle(T1_);

Notably, this only affects refc - devel detects the sink parameter and moves the instance correctly.

This issue in particular can be seen with the {.async.} transformation for both std and chronos - solving it would cut memory usage in something like #13923 by half, most probably.

Nim Version

1.6.12, devel (refc)

Current Output

No response

Expected Output

No response

Possible Solution

No response

Additional Information

No response

Araq pushed a commit that referenced this issue Aug 2, 2023
* use genRefAssign when assign to sink string

* add test case
bung87 added a commit that referenced this issue Aug 3, 2023
Araq pushed a commit that referenced this issue Aug 3, 2023
#22376)

Revert "fix #22173 `sink` paramers not moved into closure (refc) (#22359)"

This reverts commit b40da81.
@bung87 bung87 reopened this Aug 3, 2023
@ringabout ringabout added the refc refc issues label Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refc refc issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants