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

memory allocation during {.global.} init breaks GC #17085

Closed
arnetheduck opened this issue Feb 18, 2021 · 6 comments · Fixed by #17469
Closed

memory allocation during {.global.} init breaks GC #17085

arnetheduck opened this issue Feb 18, 2021 · 6 comments · Fixed by #17469

Comments

@arnetheduck
Copy link
Contributor

[arnetheduck@tempus tmp]$ more testit.nim
proc init(): string =
  for a in 0..<10000000:
    result.add 'c'

proc f() =
  var a {.global.} = init()
  var b {.global.} = init()
  var c {.global.} = init()

  echo a.len
  echo b.len
  echo c.len

f()
[arnetheduck@tempus tmp]$ nim c --hints:off -r testit.nim 
9920232
9920232
10000000
[arnetheduck@tempus tmp]$ nim --version
Nim Compiler Version 1.4.2 [Linux: amd64]
Compiled at 2020-11-30
Copyright (c) 2006-2020 by Andreas Rumpf

git hash: 3fb5157ab1b666a5a5c34efde0f357a82d433d04
active boot switches: -d:release

this is because globals are registered to the global variable table after init is run, meaning that any memory allocated while initializing globals will be collected by the gc

@arnetheduck
Copy link
Contributor Author

v1.2.6 also affected

Araq added a commit that referenced this issue Feb 19, 2021
@Araq Araq closed this as completed in 4395a26 Feb 19, 2021
narimiran pushed a commit that referenced this issue Feb 19, 2021
(cherry picked from commit 4395a26)
narimiran pushed a commit that referenced this issue Feb 19, 2021
(cherry picked from commit 4395a26)
@timotheecour
Copy link
Member

this bug was closed but https://dev.azure.com/nim-lang/255dfe86-e590-40bb-a8a2-3c0295ebdeb1/_apis/build/builds/13586/logs/89 logs shows:

hostOS: linux, hostCPU: i386, int: 4, float: 8, cpuEndian: littleEndian, cwd: /home/vsts/work/1/s
...
2021-02-25T13:48:39.2508482Z �[32mPASS: �[36mtests/gc/trace_globals c  -d:release --gc:markAndSweep      �[34m ( 2.22 sec)�[0m
2021-02-25T13:48:39.2509276Z �[1m�[31mFAIL: �[36mtests/gc/trace_globals c  --gc:boehm�[0m
2021-02-25T13:48:39.2510185Z �[1m�[36mTest "tests/gc/trace_globals" in category "gc"�[0m
2021-02-25T13:48:39.2510880Z �[1m�[31mFailure: reOutputsDiffer�[0m
2021-02-25T13:48:39.2511671Z �[33mExpected:�[0m
2021-02-25T13:48:39.2512205Z �[1m10000000
2021-02-25T13:48:39.2512640Z 10000000
2021-02-25T13:48:39.2512961Z 10000000
2021-02-25T13:48:39.2513436Z �[0m
2021-02-25T13:48:39.2513947Z �[33mGotten:�[0m
2021-02-25T13:48:39.2514669Z �[1mGC Warning: Repeated allocation of very large block (appr. size 9920512):
2021-02-25T13:48:40.1763012Z 	May lead to memory leak and poor performance.
2021-02-25T13:48:40.1764453Z 10000000
2021-02-25T13:48:40.1765312Z 10000000
2021-02-25T13:48:40.1766131Z 10000000
2021-02-25T13:48:40.1766665Z 
2021-02-25T13:48:40.1768272Z �[0m
2021-02-25T13:48:40.1769928Z �[32mPASS: �[36mtests/gc/trace_globals c  -d:release --gc:boehm             �[34m ( 0.95 sec)�[0m
2021-02-25T13:48:40.1771306Z FAILURE! total: 126 passed: 119 skipped: 6 failed: 1
2021-02-25T13:48:40.1811421Z progress[all]: 79/126 starting: cat: pragmas

@Araq
Copy link
Member

Araq commented Feb 26, 2021

This is a Boehm GC specific problem and not a bug. We can disable the test for --gc:boehm.

timotheecour added a commit to timotheecour/Nim that referenced this issue Mar 23, 2021
… a warning causing CI to fail with mismatched output
timotheecour added a commit to timotheecour/Nim that referenced this issue Mar 23, 2021
… a warning causing CI to fail with mismatched output
Araq added a commit that referenced this issue Mar 23, 2021
@timotheecour
Copy link
Member

I can't reproduce the original bug report (tried 1.2.6 and 1.4.2, on both osx and linux via docker
Linux f27b084ca41f 4.19.121-linuxkit #1 SMP Tue Dec 1 17:50:32 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

@arnetheduck what system were you using?

@Araq Araq closed this as completed in 8ccde68 Mar 24, 2021
timotheecour added a commit to timotheecour/Nim that referenced this issue Mar 24, 2021
… a warning causing CI to fail with mismatched output
@arnetheduck
Copy link
Contributor Author

linux/fedora - add a 0 to the loop count - this relies on the gc triggering at the right moment - it's not arch specific, it's a bug that's fairly easy to see in the logic of the init order, pre-fix

@arnetheduck
Copy link
Contributor Author

narimiran pushed a commit that referenced this issue Mar 24, 2021
…ected and we don't care (#17487)

(cherry picked from commit 8ccde68)
ardek66 pushed a commit to ardek66/Nim that referenced this issue Mar 26, 2021
ardek66 pushed a commit to ardek66/Nim that referenced this issue Mar 26, 2021
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.

4 participants