Fix memory leak on unset of associative array #64
Conversation
|
For me, the regression test is still reporting a leak. After applying this diff: diff --git a/src/cmd/ksh93/tests/leaks.sh b/src/cmd/ksh93/tests/leaks.sh
index 6a915c7..93a2ae8 100755
--- a/src/cmd/ksh93/tests/leaks.sh
+++ b/src/cmd/ksh93/tests/leaks.sh
@@ -110,7 +110,8 @@ do
stuff[xyz][elem4]="data4"
done
after=$(getmem)
-(( after > before )) && err_exit 'unset of associative array causes memory leak'
+(( after > before )) && err_exit 'unset of associative array causes memory leak' \
+ "(leaked $((after - before)) KiB)"
# ======
exit $((Errors<125?Errors:125))the output is: The amount of leak reported varies each time I run the test; I get anywhere between 9216 KiB and 36864 KiB. This is on macOS 10.14.6. And no, I did not forget to recompile :) |
|
Can confirm that this fixes it on FreeBSD (x86_64) and Linux (Debian 10, x86_64). Also, when I run the test before recompiling, the reported memory leak is much smaller, and consistent (832 KiB on FreeBSD, 864 KiB on Debian). Which makes me wonder if something is wrong with ps on macOS. But then why don't the other leaks.sh regression tests fail? |
|
Does building ksh with ksh/src/lib/libast/vmalloc/malloc.c Lines 81 to 89 in 9a9da2c ksh/src/lib/libast/vmalloc/vmhdr.h Lines 89 to 92 in 9a9da2c On Arch Linux the reported memory leak goes down from 800KiB to 780KiB (before patching) when |
|
Yes, I'd say that |
|
Even if I change |
|
If |
|
The shortest output I can get If I change getmem() in leaks.sh to extract the total RESIDENT PAGES number using awk, as below, then the results are nondeterministic again, different for every test run. Then I tried getting the RESIDENT PAGES column from the line that starts with (note: below, |
'ps' does not always give reliable results; on macOS, 'ps' appears to produce nondeterministic (i.e. randomly varying) results for 'vsz' and 'rss', making it unusable for memory leak tests. See: #64 (comment) and further comments. So let's compile in the vmstate builtin so that we can make sure to measure things properly. It also reports bytes instead of 1024-byte blocks, so smaller leaks can be detected. To be decided: whether or not to disable the vmstate builtin for release builds in order to save about 12K in the ksh binary. src/cmd/ksh93/data/builtins.c: - Add vmstate to the list of builtins that are compiled in. src/cmd/ksh93/tests/leaks.sh: - getmem(): get size using: vmstate --format='%(busy_size)u' (Using busy_size instead of size seems to make more sense as it excludes freed blocks. See vmstate --man) - Introduce a $unit variable for reporting leaks and set it to 'bytes'; this makes it easier to change the unit in future. - Since the tests are now more sensitive, initialise all variables before use to avoid false leak detections. - The last test seemed to need a few more 'read' invocations in order to get memory usage to a steady state before the test.
src/cmd/ksh93/sh/name.c: - Associative arrays weren't being properly freed from memory, which was causing a memory leak. This is solved by properly freeing associative arrays from memory in 'nv_delete'. Reproducer from https://www.mail-archive.com/ast-users@lists.research.att.com/msg01016.html: $ typeset -A stuff $ typeset -lui i=0 $ for (( i=0; i<1000; i++ )); do $ unset stuff[xyz] $ typeset -A stuff[xyz] $ stuff[xyz][elem0]="data0" $ stuff[xyz][elem1]="data1" $ stuff[xyz][elem2]="data2" $ stuff[xyz][elem3]="data3" $ stuff[xyz][elem4]="data4" $ done src/cmd/ksh93/tests/leaks.sh: - Add a regression test for memory leaking when an associative array is unset.
src/cmd/ksh93/tests/leaks.sh: - The associative array wasn't being unset after the loop finished, which caused a spurious failure when the more precise vmstate builtin is used instead of ps(1). Fix this with an additional unset placed after the loop.
Associative arrays aren't completely freed from memory in
nv_delete, which causes a memory leak to occur when an associative array is unset. This is fixed by freeing all elements of an associative array with a loop before they become inaccessible. Reproducer from https://www.mail-archive.com/ast-users@lists.research.att.com/msg01016.html: