Closed
Bug 206643
Opened 21 years ago
Closed 21 years ago
[PATCH] major performance improvement in cygwin-wrapper
Categories
(SeaMonkey :: Build Config, enhancement)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.5beta
People
(Reporter: paxunix, Assigned: mozbugs-build)
Details
Attachments
(2 files, 3 obsolete files)
1.50 KB,
patch
|
cls
:
review+
mkaply
:
approval1.5b+
|
Details | Diff | Splinter Review |
1.80 KB,
patch
|
paxunix
:
review+
cls
:
superreview+
mkaply
:
approval1.5b+
|
Details | Diff | Splinter Review |
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:
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•21 years ago
|
||
Marking NEW this it has a patch.
Assignee: mozbugs-build → seawood
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•21 years ago
|
||
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•21 years ago
|
||
I just got the build working yesterday: would using 'cygpath -m $DIR' help any in making the cygwin-wrapper faster?
Comment 5•21 years ago
|
||
Added bug #207521 to look at using cygpath.
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.
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
Comment 8•21 years ago
|
||
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).
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•21 years ago
|
||
This makes configure run mount -p to determine the cygwin mount point, then exports CYGDRIVE_MOUNT in the environment for cygwin-wrapper.
Comment 11•21 years ago
|
||
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+
Updated•21 years ago
|
Attachment #129523 -
Flags: review?(cls)
Reporter | ||
Comment 12•21 years ago
|
||
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•21 years ago
|
||
The same cygwin-wrapper also exists as nsprpub/build/cygwin-wrapper (and directory/c-sdk/config/cygwin-wrapper).
Reporter | ||
Comment 14•21 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•21 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•21 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•21 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.
Attachment #129523 -
Flags: approval1.5b?
Attachment #129532 -
Flags: approval1.5b?
Updated•21 years ago
|
Attachment #129523 -
Flags: approval1.5b? → approval1.5b+
Updated•21 years ago
|
Attachment #129532 -
Flags: approval1.5b? → approval1.5b+
Comment 18•21 years ago
|
||
The patches have been checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.5beta
Comment 19•20 years ago
|
||
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
Updated•20 years ago
|
Product: Browser → Seamonkey
Comment 20•19 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•19 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.
Description
•