Hi Tanu,
On Thu, Sep 10, 2015 at 05:02:28PM +0300, Tanu Kaskinen wrote:
> On Sat, 2015-08-29 at 12:43 +0200, Ahmed S. Darwish wrote:
> >
> > Add shell script to sample PulseAudio memory usage (VmSize,
> > Private_Dirty + Shared_Dirty) while increasing the number of connected
> > 'paplay' clients over time.
> >
> > Linux kernel /proc/$PID/smaps fields Private and Shared_Dirty are used
> > to accurately measure the total size of used dirty pages over time.
> > This shall be useful for benchmarking the PA daemon's memory while
> > introducing new features like the exclusively per-client SHM access.
> >
> > Also add an empty benchmarks collection directory 'benchmarks/'. All
> > output from the benchmarking tools shall be saved in this place, with
> > timestamps and symbolic links to the newest versions.
> >
> > Signed-off-by: Ahmed S. Darwish <darwish.07 at gmail.com>
> > ---
> > scripts/benchmark_memory_usage.sh | 87 +++++++++++++++++++++++++++++++++++++++
> > scripts/benchmarks/.gitignore | 10 +++++
> > scripts/benchmarks/README | 4 ++
> > 3 files changed, 101 insertions(+)
> > create mode 100755 scripts/benchmark_memory_usage.sh
> > create mode 100644 scripts/benchmarks/.gitignore
> > create mode 100644 scripts/benchmarks/README
>
> Thanks! This seems like a useful script.
Great, thanks for your review :-)
> I had a few problems, which I'll point out below.
>
> > diff --git a/scripts/benchmark_memory_usage.sh b/scripts/benchmark_memory_usage.sh
> > new file mode 100755
> > index 0000000..3687793
> > --- /dev/null
> > +++ b/scripts/benchmark_memory_usage.sh
> > @@ -0,0 +1,87 @@
> > +#!/bin/bash
> > +#
> > +# Measure PulseAudio memory usage (VmSize, Private+Shared_Dirty)
> > +# while increasing the number of connected clients over time.
> > +#
> > +# To avoid premature client exits, please ensure giving a long
> > +# wave file as the script's first parameter.
> > +#
> > +
> > +_basename=$(basename $0)
> > +PROMPT="\033[1m[$_basename]\033[0m"
> > +error() {
> > + echo -e "$PROMPT: ** $1" >&2; exit -1
> > +}
> > +msg() {
> > + echo -e "$PROMPT: $1"
> > +}
> > +
> > +_absolute_dirname="$(dirname `which $0`)"
>
> I guess the purpose of "which" is to get the absolute path of the
> script, but it doesn't do that at least on my machine. "readlink -f $0"
> should work better.
>
I agree; that's not a very standard use of "which". Let's use readlink
as you suggest.
> > +PA_HOME="${_absolute_dirname%/scripts}"
> > +[ -d "$PA_HOME/src" -a -d "$PA_HOME/scripts" ] ||
> > + error "This script can only be executed from PulseAudio source tree"
> > +
> > +PA=${PA_HOME}/src/pulseaudio
> > +PA_PLAY=${PA_HOME}/src/paplay
>
> paplay doesn't exist in the source tree. paplay is a symlink to pacat,
> and the symlink is created only during installation. I would suggest
> using pacat here, but pacat assumes that the file format is raw by
> default, and the --file-format option doesn't seem to allow telling
> pacat to autodetect the file format. I think the --file-format option
> should be modified to support "auto" as one of the recognized values.
>
Just creating the symlink manually by the script (if it did not
originally exist) should be sufficient?
> > +PA_FLAGS="-n -F ${PA_HOME}/src/default.pa -p ${PA_HOME}/src/"
> > +
> > +OUTPUT_DIR=${PA_HOME}/scripts/benchmarks
> > +OUTPUT_FILE=${OUTPUT_DIR}/MEMORY-USAGE-`date -Iseconds`.txt
> > +SYMLINK_LATEST_OUTPUT_FILE=${OUTPUT_DIR}/MEMORY-USAGE-LATEST.txt
> > +
> > +MAX_CLIENTS=30
> > +
> > +[ -e "$PA" ] || error "$PA does not exist. Compile PulseAudio tree first."
> > +[ -x "$PA" ] || error "$PA cannot be executed"
> > +[ -x "$PA_PLAY" ] || error "$PA_PLAY cannot be executed"
> > +
> > +AUDIO_FILE="$1"
> > +[ -n "$AUDIO_FILE" ] || error "Usage: $_basename AUDIO_FILE"
> > +[ -e "$AUDIO_FILE" ] || error "$AUDIO_FILE does not exist"
> > +[ -f "$AUDIO_FILE" ] || error "$AUDIO_FILE is not a regular file"
> > +
> > +echo "# ClientCount VmSize (KiB) DirtyRSS (KiB)" >$OUTPUT_FILE
> > +
> > +msg "Hello. Benchmarking PulseAudio daemon memory usage over time"
> > +
> > +# Check Linux Kernel's Documentation/sysctl/vm.txt for details.
> > +msg "Ensuring consistent results by dropping all VM caches!"
> > +sudo bash -c "sync && echo 3 >/proc/sys/vm/drop_caches"
> > +
> > +export PULSE_LOG=1 PULSE_LOG_COLORS= PULSE_LOG_META=
> > +
> > +msg "Starting PulseAudio daemon"
> > +$PA $PA_FLAGS &
> > +_pid=$!
> > +
> > +# Give PA daemon time to initialize everything and vacuum. We want
> > +# to make the _starting_ dirty RSS memory usage (0 clients) as
> > +# equivalent as possible for multiple trials.
> > +sleep 12
> > +
> > +msg "Starting PulseAudio clients"
> > +_n=0;
> > +while true; do
> > + [ "$_n" -le "$MAX_CLIENTS" ] || break
> > +
> > + _vmsize=`ps -o vsize= $_pid`
> > + _drss=`awk '/(Shared|Private)_Dirty:/{ sum += $2 } END { print sum }' /proc/$_pid/smaps`
>
> This failed for me at first, because I had pulseaudio already running,
> so starting the test instance failed, and therefore /proc/$_pid/smaps
> didn't exist. I think it would be a good idea to fail early in the
> script with a clear error message if pulseaudio is already running.
>
I agree; will add such a check early on.
Regards,
--
Darwish
http://darwish.chasingpointers.com
