206643 - [PATCH] major performance improvement in cygwin-wrapper

Status: VERIFIED FIXED

(SeaMonkey :: Build Config, enhancement)

Description

[paxunix]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4b) Gecko/20030521 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4b) Gecko/20030521 Mozilla Firebird/0.6

The latest batch of changes from bug #158920 now cause cygwin-wrapper to fork
several processes in order to accomplish string manipulation that can be done
using regular variable expansion in shell script.

I have a patch that I've been building with nightly for a week now that uses
variable expansion and an optionally defined environment var to determine the
cygdrive mount.  On my P2-266, this patch makes cygwin-wrapper run on average
twice as fast as the existing wrapper.

The patched wrapper output matches that from the existing wrapper for all my
test cases (I used mount point strings from the comments in bug 158920).

Reproducible: Always

Steps to Reproduce:

Comment 1

[paxunix]

Attachment: Double the performance of mozilla/build/cygwin-wrapper

No need to execute awk and sed just to do what we can do with shell variable
expansions.

If CYGDRIVE_MOUNT is correctly defined, you can avoid the mount call also.

If we were using a shell with more minerals (like zsh or ksh93), I could do
even better and get rid of the final sed as well.

Comment 2

[Olivier Cahagne]

Marking NEW this it has a patch.

Comment 3

[hacker formerly known as [e-mail]]

Just for clarification, the optionally defined env variable mentioned in comment
#0 is CYGDRIVE_MOUNT which is checked by the patched script.

Comment 4

[Will Sargent]

I just got the build working yesterday: would using 'cygpath -m $DIR' help any
in making the cygwin-wrapper faster?

Comment 5

[Will Sargent]

Added bug #207521 to look at using cygpath.

Comment 6

[paxunix]

Attachment: Get rid of final sed

This patch gets rid of the final sed call (without ksh93 or zsh, though it
would have made things shorter and cleaner).  Some metrics:

Cygwin mountpoint:  /cygdrive
Testcases:  1574 invocations of cygwin-wrapper with varying command lines (as
taken from a build log); machine (PII/266, 256MB) lightly loaded

Using Mozilla's current wrapper:  1:01:42.98 hours
Wrapper from patch 1 (CYGDRIVE_MOUNT="/cygdrive"):  22:19.07 minutes
Wrapper from this patch (CYGDRIVE_MOUNT="/cygdrive"):  3:17.61 minutes

I've verified as best I could that differing cygwin mountpoints and differing
drive mounts give results as under the original wrapper, but there's nothing
like real-world testing.

The most important difference between this wrapper and the original is that if
the user exports CYGDRIVE_MOUNT before building, the wrapper uses no pipes, so
no new processes are created during its run.

Comment 7

[paxunix]

Attachment: Correctly handle non-"up" case where argument has a prefix

This fixes a problem with the previous patch that amazingly didn't come out
when building firebird, but did when building thunderbird.  This patch
correctly handles the case where the cygwin mountpoint occurs inside an
argument, rather than only at the beginning.

Comment 8

[Brian Ryner (not reading)]

So, it seems like it would make senes to have configure run "mount -p" to get
the mount point, stash the result in autoconf.mk, and then have that value
available for cygwin-wrapper to use (by exporting the environment variable).

Comment 9

[paxunix]

I agree that saving the prefix and exporting it would be sensible.  I know
nothing of the autoconf foo to pull that off, sadly, so that work would have to
be borne by someone else.

Comment 10

[Brian Ryner (not reading)]

Attachment: hook up CYGDRIVE_MOUNT to configure

This makes configure run mount -p to determine the cygwin mount point, then
exports CYGDRIVE_MOUNT in the environment for cygwin-wrapper.

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 125185 [details] [diff] [review]
Correctly handle non-"up" case where argument has a prefix

>+	else
>+        eval 'leader=${i%%'${mountpoint}'/[a-zA-Z]/*}'
>+		if ! test "${leader}" = "${i}"; then
>+            eval 'pathname=${i#'${leader}${mountpoint}'/[a-zA-Z]/}'
>+			eval 'no_mountpoint=${i#'${leader}${mountpoint}'/}'

Just one minor nit, the 'eval' lines are misindented because they're using
spaces instead of tabs.  r=bryner with this change.

Comment 12

[paxunix]

Attachment: address whitespace issues

Changed all tabs to spaces per Moz Coding Style Guide (and old cygwin-wrapper
had no tabs too).

Comment 13

[paxunix]

The same cygwin-wrapper also exists as nsprpub/build/cygwin-wrapper (and
directory/c-sdk/config/cygwin-wrapper).

Comment 14

[paxunix]

Comment on attachment 129532 [details] [diff] [review]
address whitespace issues

Carrying bryner review from comment #11.  sr= volunteer?

Comment 15

[cls]

Comment on attachment 129523 [details] [diff] [review]
hook up CYGDRIVE_MOUNT to configure

Move the CYGDRIVE_MOUNT assignment & AC_SUBST outside of that _CYGWIN_PERL if
test && r=cls

Comment 16

[cls]

Comment on attachment 129532 [details] [diff] [review]
address whitespace issues

After applying the patches, my clobber builds went from 2:48 to 2:23.  I guess
my system just sucks. :-/ sr=cls

Comment 17

[paxunix]

In that case, I would say that your machine doesn't suck.  On my machine it can
take around 1 second for cygwin to spawn a new cygwin-based process (for
example, running configure absolutely crawls) if there is any sort of
significant load (like there is during a moz build)--that's suckage.

Feel free to check this in whenever.

Comment 18

[cls]

The patches have been checked in.

Comment 19

[Brian 'netdragon' Bober]

Verified Fixed.

Btw... Why do we have 3 Cygwin wrappers now, two of them being identical, and
one being faster than the other 2? 

Issue in bug 245918:

Can we combine them (or have cygwin_wrapper always be
$_topsrcdir/build/cygwin-wrapper?

Comment 20

[cls]

The 3 copies of the cygwin-wrapper script are in 3 independent projects. 
Combining them is not an option.  They should be in sync.  File a bug if they
are not.

Comment 21

[Rich Megginson]

Wow - is it really necessary to jump through these flaming hoops of sh just to avoid calling cygpath?