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

regression: memory leak with default GC #10488

Closed
timotheecour opened this issue Jan 29, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@timotheecour
Copy link
Contributor

commented Jan 29, 2019

@iffy was noticing something odd on his end and after further diagnostic it appears there's a memory leak regression introduced recently

current devel has regression at a58f5b6
devel at 7920dc2 (2019-01-15) didn't have it (nor did 0.19.2)

see timotheecour/vitanim#6 for full context with minimal reproducing example; in short it's just this:

clib.nim

proc hello_echo*() {.exportc.} =
  echo GC_getStatistics()

proc main()=
  # warmup period => no short term leak
  for i in 0..<300:
    hello_echo()

main()

test2.c

void hello_echo();

int main() {
  // for (int i = 0; i < 100000; i++) {
  for (int i = 0; i < 1000; i++) {
    hello_echo();
  }
  return 0;
}

commands:

nim c  -o:$temp_D/libclib.dylib --app:lib --nimcache:$temp_D clib.nim
clang -o $temp_D/test test2.c -lclib -L$temp_D/
$temp_D/test

diagnostic

seems like there's short-term leak that's being corrected on a long term cycle, but results in large memory usage where it should'nt be (4MB) when all there is is echo GC_getStatistics() in a loop which doesn't allocate much

see https://user-images.githubusercontent.com/2194784/51887771-bbb07800-2349-11e9-8236-c0a9af338689.png + other graphs in timotheecour/vitanim#6

EDIT

/cc @Araq
ok I did a bisection to find the culprit commit, turns out it's this one:
3c10654 Increase verbosity of logGC (#10449)

ironically, it's the one introduced by @iffy to investigate GC ;-)

I don't know if these changes would be valuable to anyone else, but they have been of some benefit to me. AFAICT this is a non-breaking change.

btw, beyond fixing that regression, can we please fix that PR to use nimLogGC instead of logGC; we really need some notion of sane namespacing to avoid conflict with other nim libraries as much as possible.

@iffy

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

How embarrassing...

Do you know yet which part is leaking? I thought my change introduced code that only ran when logGC was defined, but the leak is present even without logGC defined, right?

timotheecour added a commit to timotheecour/Nim that referenced this issue Jan 30, 2019

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

but the leak is present even without logGC defined, right?

yes

Do you know yet which part is leaking? I thought my change introduced code that only ran when logGC was defined,

see #10498

@Araq Araq closed this in #10498 Jan 30, 2019

Araq added a commit that referenced this issue Jan 30, 2019

fix #10488 GC memory leak regression (#10498)
* fix #10488 GC memory leak regression

* re-enable gch.stack.bottom.repr but only inside when defined(logGC)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.