Improving 'bmake wrapper' speed

[Jonathan Perkin]

I recently got fed up with how long the 'wrapper' phase was taking,
especially on packages with a large number of dependencies, so decided
to take a look.

Breaking out DTrace on an otherwise idle system running just the
'wrapper' phase ('patch' had already been done) showed that we are
running a huge number of exec'd processes:

# dtrace -n 'syscall::exece:return { @num[execname] = count(); }'
[ full output snipped for brevity ]
grep 94
sort 164
nbsed 241
mkdir 399
bash 912
cat 3893
ln 7631
rm 7766
dirname 7769

Tracking these down, the 'cat's are primarily from
mk/wrapper/gen-transform.sh, and the ln/rm/dirname are from each
symlink done by mk/buildlink3/bsd.buildlink3.mk.

I have a proposed patch which significantly reduces the number of
execs we need to perform:

http://us-east.manta.joyent.com/jperkin/public/patches/wrapper-perf.diff

The patch reduces the runtime for 'bmake wrapper' in net/kppp on a
SmartOS zone from this:

real 2:26.094442113
user 32.463077360
sys 1:48.647178135

to this:

real 49.648642097
user 14.952946135
sys 33.989975053

with DTrace confirming the change:

# dtrace -n 'syscall::exece:return { @num[execname] = count(); }'
[ full output snipped for brevity ]
grep 94
cat 106
sort 164
nbsed 241
mkdir 399
bash 912
ln 7631

On a Linux system it reduces the runtime from this:

real 1m3.939s
user 0m17.437s
sys 0m43.280s

to this:

real 0m25.436s
user 0m8.816s
sys 0m15.406s

So between a 2.5x and 3x speedup.

I have verified that the WRKDIR is identical after this change, but
will run it through a full bulk build anyway.

Comments? There are a lot of other 'useless use of cat' cleanups that
can be done, but they do not have such a significant performance
impact.

Regards,

[Thomas Klausner]

Post by Jonathan Perkin
I have a proposed patch which significantly reduces the number of
http://us-east.manta.joyent.com/jperkin/public/patches/wrapper-perf.diff

These are impressive numbers.

Looking at the patch, it consists mostly of changes like these:

- $cat << EOF
-s|\([$_sep]\)$1\([$_sep]\)|\1$2\2|g
-s|\([$_sep]\)$1\([$_sep]\)|\1$2\2|g
-s|\([$_sep]\)$1$|\1$2|g
-s|^$1\([$_sep]\)|$2\1|g
-s|^$1$|$2|g
-EOF
+ echo "s|\([$_sep]\)$1\([$_sep]\)|\1$2\2|g"
+ echo "s|\([$_sep]\)$1\([$_sep]\)|\1$2\2|g"
+ echo "s|\([$_sep]\)$1$|\1$2|g"
+ echo "s|^$1\([$_sep]\)|$2\1|g"
+ echo "s|^$1$|$2|g"

The only I thing I wonder about this is if there is a quoting
difference between the two versions that might be relevant for pkgsrc,
or not.
Thomas

[Jonathan Perkin]

Post by Thomas Klausner

Post by Jonathan Perkin
I have a proposed patch which significantly reduces the number of
http://us-east.manta.joyent.com/jperkin/public/patches/wrapper-perf.diff

These are impressive numbers.
- $cat << EOF
-s|\([$_sep]\)$1\([$_sep]\)|\1$2\2|g
-s|\([$_sep]\)$1\([$_sep]\)|\1$2\2|g
-s|\([$_sep]\)$1$|\1$2|g
-s|^$1\([$_sep]\)|$2\1|g
-s|^$1$|$2|g
-EOF
+ echo "s|\([$_sep]\)$1\([$_sep]\)|\1$2\2|g"
+ echo "s|\([$_sep]\)$1\([$_sep]\)|\1$2\2|g"
+ echo "s|\([$_sep]\)$1$|\1$2|g"
+ echo "s|^$1\([$_sep]\)|$2\1|g"
+ echo "s|^$1$|$2|g"

For clarity, the changes are:

- use ${file%/*} instead of `dirname $file`, this doesn't handle all
cases that dirname does, e.g. if file doesn't contain any "/"s, but
as all ours should be full paths then I think this is ok.

- test for file's existence before calling rm(1) on it - it is highly
likely in this case that the file does not exist, so we save
thousands of unnecessary execs of rm(1).

- replace cat <<EOF with echo, again avoiding an exec.

The first two provide the bulk of the speedup (2x), the cat->echo
changes give us an extra 60% or so on top.

Post by Thomas Klausner
The only I thing I wonder about this is if there is a quoting
difference between the two versions that might be relevant for pkgsrc,
or not.

Possibly, hopefully the full bulk build will shake out any issues with
this patch.

[Alan Barrett]

Post by Thomas Klausner
- $cat << EOF
-s|\([$_sep]\)$1\([$_sep]\)|\1$2\2|g
-s|\([$_sep]\)$1\([$_sep]\)|\1$2\2|g
-s|\([$_sep]\)$1$|\1$2|g
-s|^$1\([$_sep]\)|$2\1|g
-s|^$1$|$2|g
-EOF
+ echo "s|\([$_sep]\)$1\([$_sep]\)|\1$2\2|g"
+ echo "s|\([$_sep]\)$1\([$_sep]\)|\1$2\2|g"
+ echo "s|\([$_sep]\)$1$|\1$2|g"
+ echo "s|^$1\([$_sep]\)|$2\1|g"
+ echo "s|^$1$|$2|g"
The only I thing I wonder about this is if there is a quoting
difference between the two versions that might be relevant for pkgsrc,
or not.

I think the quoting is fine. The here document in cat <<EOF should be
handled by the shell like a double-quoted string, except that quotation
marks are not special inside a here document.

[Jonathan Perkin]

Post by Jonathan Perkin
The patch reduces the runtime for 'bmake wrapper' in net/kppp on a
real 2:26.094442113
user 32.463077360
sys 1:48.647178135
real 49.648642097
user 14.952946135
sys 33.989975053

I've performed two from-scratch bulk builds with and without this
change to see what the real world performance difference is across all
of pkgsrc:

Without: 11hr 2m
With: 9hr 55m

I'll commit this patch this week unless there are any objections.

[Thomas Klausner]

Post by Jonathan Perkin
I've performed two from-scratch bulk builds with and without this
change to see what the real world performance difference is across all
Without: 11hr 2m
With: 9hr 55m
I'll commit this patch this week unless there are any objections.

Very nice! Please commit.
Thomas

[Joerg Sonnenberger]

Post by Jonathan Perkin

Post by Jonathan Perkin
The patch reduces the runtime for 'bmake wrapper' in net/kppp on a
real 2:26.094442113
user 32.463077360
sys 1:48.647178135
real 49.648642097
user 14.952946135
sys 33.989975053

I've performed two from-scratch bulk builds with and without this
change to see what the real world performance difference is across all
Without: 11hr 2m
With: 9hr 55m
I'll commit this patch this week unless there are any objections.

There are some parts I dislike quite a bit. Will go into details
tomorrow.

Joerg

[Joerg Sonnenberger]

Post by Jonathan Perkin
Tracking these down, the 'cat's are primarily from
mk/wrapper/gen-transform.sh, and the ln/rm/dirname are from each
symlink done by mk/buildlink3/bsd.buildlink3.mk.

Changing the cats to echo is fine, but I would prefer to use '' for
quoting. The dirname invocations are more tricky, since it could change
the behavior if a case of trailing slashes ever come through. I would
prefer to leave that one out.

Joerg

[David Laight]

Post by Joerg Sonnenberger

Post by Jonathan Perkin
Tracking these down, the 'cat's are primarily from
mk/wrapper/gen-transform.sh, and the ln/rm/dirname are from each
symlink done by mk/buildlink3/bsd.buildlink3.mk.

Changing the cats to echo is fine, but I would prefer to use '' for
quoting.

The shell variable expansions need to be inside "", the other text could
be inside '', but repeatedly flipping the type of quotes is error prone.
You do need to flip if you want a " though "xx"'"'"yy" gives xx"yy

Post by Joerg Sonnenberger
The dirname invocations are more tricky, since it could change
the behavior if a case of trailing slashes ever come through. I would
prefer to leave that one out.

Or use an extra shell assignment to strip trailing slashes.

If it is doing 'dirname' of a filename (rather than a directory name)
then there shouldn't be any problem.

Or some horrid hackey based on teh properties of IFS and $*.
If you know what the below prints:

(IFS=/; x="abc//def/geh/"; set -- $x; IFS=' '; x="$*"; set -- $x; IFS=/; x="$*"; IFS=' '; echo "$x"; )

David