Skip to content

Commit

Permalink
Use vmstate for memory leak regress tests (re: ad9a921)
Browse files Browse the repository at this point in the history
'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.
  • Loading branch information
McDutchie committed Jul 8, 2020
1 parent 9a9da2c commit bf79131
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 7 deletions.
1 change: 1 addition & 0 deletions src/cmd/ksh93/data/builtins.c
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ const struct shtable3 shtab_builtins[] =
CMDLIST(uname)
CMDLIST(wc)
CMDLIST(sync)
CMDLIST(vmstate)
#endif
#if SHOPT_REGRESS
"__regress__", NV_BLTIN|BLT_ENV, bltin(__regress__),
Expand Down
22 changes: 15 additions & 7 deletions src/cmd/ksh93/tests/leaks.sh
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,18 @@ Command=${0##*/}
integer Errors=0

[[ -d $tmp && -w $tmp ]] || { err\_exit "$LINENO" '$tmp not set; run this from shtests. Aborting.'; exit 1; }
builtin vmstate 2>/dev/null || { err\_exit "$LINENO" 'vmstate built-in command not compiled in; skipping tests'; exit 0; }

# Get the current amount of memory usage
function getmem
{
UNIX95=1 ps -p "$$" -o vsz=
vmstate --format='%(busy_size)u'
}
unit=bytes
n=$(getmem)
if ! let "$n == $n" 2>/dev/null # not a number?
then err\_exit "$LINENO" "'ps' not POSIX-compliant; skipping tests"
exit 0
then err\_exit "$LINENO" "'vmstate' output unexpected; tests cannot be run. (expected a number; got $(printf %q "$n"))"
exit 1
fi

# test for variable reset leak #
Expand All @@ -53,6 +55,9 @@ function test_reset
done
}

# Initialise variables used below to avoid false leaks
before=0 after=0 i=0 u=0

N=1000

# one round to get to steady state -- sensitive to -x
Expand All @@ -64,7 +69,7 @@ test_reset $N
after=$(getmem)

if (( after > before ))
then err_exit "variable value reset memory leak -- $((after - before)) KiB after $N iterations"
then err_exit "variable value reset memory leak -- $((after - before)) $unit after $N iterations"
fi

# buffer boundary tests
Expand All @@ -83,14 +88,17 @@ done | while read -u$n -C stat
do :
done {n}<&0-
after=$(getmem)
(( after > before )) && err_exit "memory leak with read -C when deleting compound variable (leaked $((after - before)) KiB)"
(( after > before )) && err_exit "memory leak with read -C when deleting compound variable (leaked $((after - before)) $unit)"

read -C stat <<< "$data"
# extra 'read's to get to steady state
for ((i=0; i < 10; i++))
do read -C stat <<< "$data"
done
before=$(getmem)
for ((i=0; i < 500; i++))
do read -C stat <<< "$data"
done
after=$(getmem)
(( after > before )) && err_exit "memory leak with read -C when using <<< (leaked $((after - before)) KiB)"
(( after > before )) && err_exit "memory leak with read -C when using <<< (leaked $((after - before)) $unit)"

exit $((Errors<125?Errors:125))

0 comments on commit bf79131

Please sign in to comment.