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

Nested =destroy call for "seqsv2" doesn't seem to find its custom =destroy implementation... #12588

Closed
GordonBGood opened this issue Nov 4, 2019 · 2 comments

Comments

@GordonBGood
Copy link
Contributor

Example

# compile with "--gc:destructors" to see the behavior,
# as GC's will compensate automatically using legacy seq's...

# own defined MySeq, if it works it means related to compiler `seq` manipulation trickery;
# if it doesn't, it means that somehow nested calls to `=destroy` aren't working...
type
  MySeq[T] = object
    len: int
    data: ptr UncheckedArray[T]
proc newMySeq[T](n: int): MySeq[T] =
  if n <= 0: return
  MySeq[T](len: n, data: cast[ptr UncheckedArray[T]]((T.sizeof * n).allocShared0))
# [ may custom "hooks" somehow disables the proper destruction of the inner object?
proc `=destroy`[T](x: var MySeq[T]) =
  # to show when destroyed...
  echo "destroying MySeq of len:  ", x.len
  if x.data != nil: x.data.deallocShared
  x.wasMoved # system doesn't do this zeroing for us!
proc `=`[T](dst: var MySeq[T]; src: MySeq[T]) =
  echo "copying MySeq of len from/to:  ", src.len, " ", dst.len
  if dst.data != src.data:
    dst.`=destroy`; dst.len = src.len; dst.data = src.data
proc `=sink`[T](dst: var MySeq[T]; src: MySeq[T]) =
  echo "moving MySeq of len from/to:  ", src.len, " ", dst.len
  dst.`=destroy`; dst.len = src.len; dst.data = src.data
# ]#

type
  TestObj = object 
#    big: MySeq[byte] # custom MySeq works;
    big: seq[byte] # regular seq doesn't, even for seqsv2!
    cntnts: int
# the custom "hooks" cause the memory leak behavior,
# the custom "hooks" are not really required here as the generated `=destroy` would do the job
# this is to show that a nested call to a `=destroy` doesn't work and is likely just "no-oping"
# as it somehow disables the proper destruction of the inner seq by its custom destructor...
# [ remove the space before the preceeding `[` to comment out the following custom "hooks"...
proc `=destroy`(x: var TestObj) =
  # to show when destroyed...
  echo "destroying TestObj containing:  ", x.cntnts
  x.big.`=destroy` # nested destructor call!
  x.wasMoved # system doesn't do this zeroing for us!
proc `=`(dst: var TestObj; src: TestObj) =
  echo "copying TestObj containing from/to:  ", src.cntnts, " ", dst.cntnts
  dst.`=destroy`; dst.big = src.big; dst.cntnts = src.cntnts
proc `=sink`(dst: var TestObj; src: TestObj) =
  echo "moving TestObj containing from/to:  ", src.cntnts, " ", dst.cntnts
  dst.`=destroy`; dst.big = src.big; dst.cntnts = src.cntnts
# ]#

proc main() =
  # use outside of nesting in another object works, even if the `=destroy` is manually called:
#  var sq = newSeq[byte](1024*1024); sq.len.echo; sq.`=destroy`
  # may be related to the nested call to `=destroy` in the TestObj destructor?
#  var to = TestObj(big: newMySeq[byte](1024*1024), cntnts: 7); echo to. # custom MySeq, - no problem
  var to = TestObj(big: newSeq[byte](1024*1024), cntnts: 7); echo to.cntnts # standard seqsv2 -problem
  
for _ in 1 .. 10:
  let strtmem = getOccupiedMem()
  main()
  echo "change in occupied memory:  ", (getOccupiedMem() - strtmem)

# Use of custom MySeq doesn't leak, built-in seq does (seqsv2),
# showing it is a problem with how `seq`'s are handled!

Current Output

Repeated for every loop:

moving TestObj containing from/to:  7 0
destroying TestObj containing:  0
7
destroying TestObj containing:  7
change in occupied memory:  1081344

The above memory leak was confirmed with external memory resource use tools with the number of loops increased to 1000, with memory use increasing per loop until the program ends, when it drops back to the original levels.

Expected Output

Repeated for every loop:

moving TestObj containing from/to:  7 0
destroying TestObj containing:  0
7
destroying TestObj containing:  7
change in occupied memory:  0

Possible Solution

It may be caused by the call to seq's (seqsv2) custom destructor, which appears to have a "no-op" version auto generated when the call is nested instead of calling the custom =destroy that is a part of newSeqV2?

Additional Information

This issue needs to be fixed for "seqsv2" to be used generally across all GC's (along with some other changes).
This was tested on the latest devel branch as of 3-11-2019 including the ref counting patches:

$ nim -v
Nim Compiler Version 1.1.0 [Windows: amd64]
Compiled at 2019-11-03
Copyright (c) 2006-2019 by Andreas Rumpf

active boot switches: -d:release
@ringabout
Copy link
Member

ringabout commented Mar 29, 2021

It works for ARC or --gc:destructors.

@GordonBGood GordonBGood changed the title Nested =destroy call for "seqsv2" doesn't seem to find its custom =destroy imlementation... Nested =destroy call for "seqsv2" doesn't seem to find its custom =destroy implementation... Jul 10, 2021
@GordonBGood
Copy link
Contributor Author

Confirmed, it now works for all of --gc:destructors, --gc: arc, and --gc:orc on devel, which was the goal when the issue was filed; changes in the new memory management implementations seemingly have fixed the issue in the intervened time since "newruntime" as become "Arc/Orc".

Closing Issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants