fix #14421 items uses lent T

[timotheecour]

• fixes items is 20%~30% slower than iteration via an index #14421
  items now has no overhead over mitems or over access via index myseq[i]

looks like only 1 test fails:

/Users/runner/runners/2.262.1/work/Nim/Nim/pkgstemp/zero_functional/zero_functional.nim(1624, 16)
  Error: '__it__0' is of type <lent seq[uint8]> which cannot be captured as it would violate memory safety, declared here: 
/Users/runner/runners/2.262.1/work/Nim/Nim/pkgstemp/zero_functional/zero_functional.nim(1624, 16)

note

• this causes alloc/dealloc in stat reports to decrease, as expected

future PR's

• extend lent T to more iterators and procs
• refine logic to only use lent when T.sizeof is small (eg, fits in a register) since pass by value won't incur cost in this case;
  eg: ptr|ref|pointer|cstring
• auto-box for var/lent refs fix #14421 items uses lent T #14447 (comment)

[mratsim]

I think NimVersion should be its own PR and lent iterators should be serated to depend on it.

[timotheecour]

ya I'll do that; i can use "yet another" condsym until nimVersion+nimVersionCT

[timotheecour]

@michael72 this PR enables faster items but the only test failing with this PR is nim c -r zero-functional/test

Error: '__it__0' is of type <lent seq[uint8]> which cannot be captured as it would violate memory safety

/Users/timothee/git_clone/nim/zero-functional/test.nim(19, 5) template/generic instantiation from here
/Users/timothee/git_clone/nim/zero-functional/zero_functional.nim(1625, 16) Error: '__it__0' is of type <lent seq[uint8]> which cannot be captured as it would violate memory safety, declared here: /Users/timothee/git_clone/nim/zero-functional/zero_functional.nim(1625, 16)
      ext.node = quote do:

I'd appreciate your help here.

I've narrowed it down to this:

import zero_functional

proc main =
  let bu = @[@[1u8, 2u8], @[3u8]]
  let b = bu --> map(it --> to(seq[int], true))
  # let b = bu --> map(it --> it)
main()

zero-functional code is rather hairy (macros calling macros calling macros) so it's hard to debug; After inserting some repr in your macros I'm suspecting it's happening for this:

(proc (): auto =
  var res`gensym36580008: seq[int]
  result = zfInit(res`gensym36580008)
  for __it__0 in __it__0:
    discard
    let __it__1 = __it__0
    zfAddItemChk(result, -1, __it__1, "seq[uint8]", "seq[int]", true))()

any help would be appreciated, including a minimal example that doesn't involve zero-functional code;

[mratsim]

__it__0 should become an argument to the proc (to avoid closure check) and zero-functional can call anonymousProc(__it__0) instead of anonymousProc()

[Araq]

Does this imply we want the borrowing rules in-place as soon as possible?

[timotheecour]

well I'm not sure how fast is "as soon as possible" so how about I introduce a temporary opt-out (as opposed to opt-in), eg: nim c -r -d:nimWorkaround14447 zero-functional/test ?
so most code will benefit from speedup, and code that breaks as above can use -d:nimWorkaround14447 ; and once the compiler allows that, -d:nimWorkaround14447 becomes a noop.

-d:nimWorkaround14447 would simply prevent lent in iterators.nim

and I can mention -d:nimWorkaround14447 in error message which cannot be captured as it would violate memory safety as a possible action (even though it won't apply to all cases)

[michael72]

@timotheecour

import zero_functional

proc main =
  let bu = @[@[1u8, 2u8], @[3u8]]
  let b = bu --> map(it --> to(seq[int], true))
  # let b = bu --> map(it --> it)
main()

In order to help I need some more information - could you be more specific?
I downloaded your branch, compiled it, installed it as my main nim, recompiled test.nim in zero_functional - no error, just a warning. I'll look into that.

But could you point me to the specific build error - I don't get it. There seem to be several libraries involved.
Your test program also compiles.

Btw - there is a simple macro ->> that prints better debug output.
https://github.com/zero-functional/zero-functional#debugging-using---

[timotheecour]

In order to help I need some more information - could you be more specific?

• are you sure you're using zero-functional HEAD and not some installed version?
• what's nim -v ?

here are detailed instructions to reproduce:

git clone https://github.com/alehander42/zero-functional && cd zero-functional
nimble develop # makes sure it uses head and not some installed version

cd pathto/nim 
git fetch origin pull/14447/head && git checkout FETCH_HEAD # fetch my branch
# rebuild nim either with `./koch boot -d:release` if you're patient or `bin/nim_csources c -o:bin/nim_temp -d:release --lib:lib --hints:off compiler/nim.nim` if you're not

bin/nim_temp r t10841.nim # test case I mentioned above in https://github.com/nim-lang/Nim/pull/14447#issuecomment-634627267
=> `Error: '__it__0' is of type <lent seq[uint8]> which cannot be captured as it would violate memory safety, declared here: /Users/timothee/git_clone/nim/zero-functional/zero_functional.nim(1624, 16); using '-d:nimWorkaround14447' helps in some cases`

bin/nim_temp r -d:nimWorkaround14447 t10841.nim # works (but disabled lent optimization globally)

no error, just a warning

i get no warning, just an error ;-) (and so did CI until I added -d:nimWorkaround14447); I'm on OSX but shouldn't matter

Btw - there is a simple macro ->> that prints better debug output.

thanks that helps, it shows:

# t10841.nim:17
# bu.zfun:
#   map(it -->> to(seq[int], true))
(proc (): auto =
  {.push, hint[XDeclaredButNotUsed]: off.}
  proc helperProc(): auto =
    for it0 in bu:
      let it1 = it0 -->> to(seq[int], true)
      result = it1

  {.pop.}
  var res_47365200: seq[type(helperProc())]
  result = zfInit(res_47365200)
  for it0 in bu:
    let it1 = it0 -->> to(seq[int], true)
    zfAddItemChk(result, -1, it1, "seq[seq[uint8]]", "", false))()

# t10841.nim:17
# __it__0.zfun:
#   to(seq[int], true)
(proc (): auto =
  var res_47390009: seq[int]
  result = zfInit(res_47390009)
  for it0 in it0:
    let it1 = it0
    zfAddItemChk(result, -1, it1, "seq[uint8]", "seq[int]", true))()

# t10841.nim:17
# __it__0.zfun:
#   to(seq[int], true)
(proc (): auto =
  var res_47570008: seq[int]
  result = zfInit(res_47570008)
  for it0 in it0:
    let it1 = it0
    zfAddItemChk(result, -1, it1, "seq[uint8]", "seq[int]", true))()

[Araq]

well I'm not sure how fast is "as soon as possible" so how about I introduce a temporary opt-out (as opposed to opt-in), eg: nim c -r -d:nimWorkaround14447 zero-functional/test ?

We can also patch zero-functional instead. I didn't know that the compiler produces an error message, I thought it produced a corruption. So the borrow rules can wait.

[timotheecour]

• turns out there was also ggplotnim, I just sent a PR there fix in preparation for https://github.com/nim-lang/Nim/pull/14447 Vindaar/ggplotnim#74
• @michael72 is there any way to do a similar fix in your code ? sorry it's still quite a bit hairy to dig in because of multiple levels of macros (even if
  -->> helps a lot); basically the simplest (but not necessarily most efficient) fix is to try this:

for it0 in bu:
  let it0 = it0 # <= add this
  ... rest of code

(but that code is generated code, not sure where)

[michael72]

@timotheecour I'm not really into quick quickfixes, but here we go.
Seems to be working on your branch - I bumped zero_functional to v1.1.1
hope depending libraries are fixed as well

Cheers

[michael72]

@timotheecour @Araq
I tried to create an example that fails with plain nim code - although atmittedly it is somewhat construed - but hey - that's what zero_functional creates ;-)

I've tried to make it as simple as possible - also using the example code that lead to the crash in the first place:

proc main =
  let orig = @[@[1u8, 2u8], @[3u8]]
  let converted = (proc (): seq[seq[int]] =
    result = @[]
    for it0 in orig:
        let it00 = it0
        let sublist = (proc (): seq[int] =
            result = @[]
            for i in it0: # it00: # would work!
                result.add(int(i)))()
        result.add(sublist))() 
  echo(converted)
main()

So this is: take a seq[seq[uint8]] and convert it to seq[seq[int]] - yes, this is more like a "Hey - we can do that to!"

we get:

 Error: 'it0' is of type <lent seq[uint8]> which cannot be captured as it would violate memory safety, declared here: /home/michael/projects/zero-functional/testwas.nim(5, 9); using '-d:nimWorkaround14447' helps in some cases

The problematic part I think - and this is regarding this PR - is the inner iterator that accesses the outer iterator it0 directly from an inner function - needing a direct capture - which with this PR does not work any more. If we assign it0 explicitly to it00 and use it00 inside the inner function then it works.

I kind of "fixed" the problem on zero_functional side by adding this additional indirection - but it is somewhat a workaround, right? On the other hand it is an esoteric use case and iterators, anonymous functions and keeping all the use cases efficiently together probably very tough.

So - think about it. If it leads to speed-ups it's maybe worth it, but in edge cases it is a bit weird behaviour.

[Araq]

I kind of "fixed" the problem on zero_functional side by adding this additional indirection - but it is somewhat a workaround, right? On the other hand it is an esoteric use case and iterators, anonymous functions and keeping all the use cases efficiently together probably very tough.

One solution is to make the compiler auto-box for var/lent types.

[timotheecour]

One solution is to make the compiler auto-box for var/lent types.

• so including result magic variable, so that this would work and be safe?

proc main(): int =
  proc bar() =
    result = 1 # Error: 'result' is of type <int> which cannot be captured as it would violate memory safety
  bar()
echo main()

IMO this is safe so long bar delegate doesn't escape main, and that should use the exact same escape analysis as for var result

• oh and here's a minimal example:
  @michael72

I've tried to make it as simple as possible - also using the example code that lead to the crash in the first place:

minimized further:

proc main() =
  for a in @[1,2]:
    # let a = a # uncomment removes error
    proc bar() =
      discard a # Error: 'a' is of type <lent int> which cannot be captured as it would violate memory safety,
    bar()
main()

but it is somewhat a workaround, right?

yes but there should be exactly 0 performance penalty from before PR since it results in a copy here let it00 = it0 instead of here for it0 in orig:.

The ideal fix is what detecting when it's safe (no escape) and allowing it.

In the meantime we could have a template ensureCopy(a): untyped that I think is generally useful and would only return a copy if a is lent|var, else would return it as is (no double copy).

[Araq]

so including result magic variable, so that this would work and be safe?

Yeah, so that the error message is no more. Let's shift the complexity to the optimizer where it belongs.

[ghost]

Is it intended that this now works (unless you pass -d:nimWorkaround14447 of course)?

var a = @['a', 'b', 'c', 'd', 'e', 'f']


for x in a:
  x = 'c'

echo a

I think it should still somehow be caught at CT and not work

[timotheecour]

no it should give CT error; looking into it more, it looks like lent is pretty buggy and violates the contract:
=> #14498