[PATCH] major performance improvement in cygwin-wrapper

VERIFIED FIXED in mozilla1.5beta

Status

SeaMonkey
Build Config
--
enhancement
VERIFIED FIXED
15 years ago
12 years ago

People

(Reporter: paxunix, Assigned: (default assignee for unassigned bugs. does not receive bugmail))

Tracking

Trunk
mozilla1.5beta
x86
Windows 98

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

15 years ago
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:
(Reporter)

Comment 1

15 years ago
Created attachment 123904 [details] [diff] [review]
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

15 years ago
Marking NEW this it has a patch.
Assignee: mozbugs-build → seawood
Status: UNCONFIRMED → NEW
Ever confirmed: true
Just for clarification, the optionally defined env variable mentioned in comment
#0 is CYGDRIVE_MOUNT which is checked by the patched script.

Assignee: seawood → mozbugs-build

Comment 4

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

Comment 5

15 years ago
Added bug #207521 to look at using cygpath.
(Reporter)

Comment 6

15 years ago
Created attachment 124639 [details] [diff] [review]
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.
(Reporter)

Comment 7

15 years ago
Created attachment 125185 [details] [diff] [review]
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.
Attachment #124639 - Attachment is obsolete: true
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).
(Reporter)

Comment 9

15 years ago
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.
Created attachment 129523 [details] [diff] [review]
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 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.
Attachment #125185 - Flags: review+
Attachment #129523 - Flags: review?(cls)
(Reporter)

Comment 12

15 years ago
Created attachment 129532 [details] [diff] [review]
address whitespace issues

Changed all tabs to spaces per Moz Coding Style Guide (and old cygwin-wrapper
had no tabs too).
Attachment #123904 - Attachment is obsolete: true
Attachment #125185 - Attachment is obsolete: true
(Reporter)

Comment 13

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

Comment 14

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

Carrying bryner review from comment #11.  sr= volunteer?
Attachment #129532 - Flags: superreview?(cls)
Attachment #129532 - Flags: review+

Comment 15

15 years ago
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
Attachment #129523 - Flags: review?(cls) → review+

Comment 16

15 years ago
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
Attachment #129532 - Flags: superreview?(cls) → superreview+
(Reporter)

Comment 17

15 years ago
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.

Updated

15 years ago
Attachment #129523 - Flags: approval1.5b?

Updated

15 years ago
Attachment #129532 - Flags: approval1.5b?

Updated

15 years ago
Attachment #129523 - Flags: approval1.5b? → approval1.5b+

Updated

15 years ago
Attachment #129532 - Flags: approval1.5b? → approval1.5b+

Comment 18

15 years ago
The patches have been checked in.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.5beta
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?
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey

Comment 20

13 years ago
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

12 years ago
Wow - is it really necessary to jump through these flaming hoops of sh just to avoid calling cygpath?
You need to log in before you can comment on or make changes to this bug.