Closed
Bug 747448
Opened 12 years ago
Closed 12 years ago
makefile optimization: investigate replacing $(shell cd $(DEPTH); pwd) with builtin $(abspath)
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla15
People
(Reporter: joey, Assigned: jhford)
References
Details
Attachments
(5 files)
17.67 KB,
patch
|
Details | Diff | Splinter Review | |
858 bytes,
text/plain
|
Details | |
1.29 KB,
text/plain
|
Details | |
1.13 KB,
text/plain
|
Details | |
4.55 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → joey
Reporter | ||
Comment 1•12 years ago
|
||
Also check if the core_{abs,real}path functions can be replaced with the $(abspath ) native flavor. ted there's some goofy core_abspath function we have as well ted i can't recall why ted http://mxr.mozilla.org/mozilla-central/source/config/config.mk#84 ted i think perhaps it attempts to handle windows paths in msys gmake joey ok I'll setup unit tests for abspath to check permutations before poking at the core_{abs,real}path functions
Comment 2•12 years ago
|
||
bsmedberg, do you remember why we have core_abspath? Was it just because abspath didn't exist in old make versions, or did we need that special-casing of Windows paths for a particular reason?
Reporter | ||
Comment 3•12 years ago
|
||
also replace $(shell pwd) with $(CURDIR)
Comment 4•12 years ago
|
||
Some of these functions existed to deal with gmake prior to 3.81, where abspath was introduced (IIRC). I don't *think* we need them any more, but we should carefully check the pymake and msys builds to make sure that there isn't something goofy we still need them for.
Reporter | ||
Comment 5•12 years ago
|
||
core_abspath, core_relpath: http://mxr.mozilla.org/mozilla-central/source/config/config.mk#84 toolkit/locales/l10n.mk
Reporter | ||
Comment 6•12 years ago
|
||
Uploading unit test patch for safe keeping. paths generated by gmake, pymake and msys were identical for values tested so far. Main differences occur between abspath/realpath and the core_${abspath,realpath} equivalents. core_ paths retain dot and ../.. sequences when passed as arguments so testing cannot yet compare strings for equality. passing $(NULL) or $(this-is-a-typo-variable): core-abspath -> return pwd gnu-abspath -> return $(NULL) passing undef, $(NULL) or $(SPACE): core-abspath -> $(SPACE) x 4 gnu-abspath -> $(NULL)
Reporter | ||
Comment 7•12 years ago
|
||
(In reply to Joey Armstrong [:joey] from comment #6) > passing $(NULL) or $(this-is-a-typo-variable): > core-abspath -> return pwd > gnu-abspath -> return $(NULL) First batch of edits should probably modify core_{abs,rel}path and have the function call $(error ) when a path was not passed as an argument. If any calls rely on this behavior they can be changed to $(call core_abspath,.) to be explicit.
Reporter | ||
Comment 8•12 years ago
|
||
pass #1 - replace $(cd $(blah) && pwd) with $(CURDIR) pass #2 - replace core_{abs,real}path with $(abspath ) & $(realpath) if values are ok. Unit test can be run by: % gmake -f client.mk configure % cd ${obj}/config/makefiles/test % gmake check VERBOSE=1
Assignee | ||
Comment 9•12 years ago
|
||
Working on pass one, I was wondering what to do about the -W option for pwd. Msys's pwd has a -W flag that returns a native win32 path. My understanding of how ming passes these arguments [1] is that msys will look at the program being started and if it is not an msys program, it'll convert from the msys style paths to win32 style paths. With that in mind, I wonder if it's safe to wholesale replace pwd -W with $(CURDIR) and let msys do its magic. Also, a couple of places that use $(shell cd $(blah) && pwd) uses a value of blah that doesn't map to CURDIR. I've put in $(abspath blah) in those places. [1]http://www.mingw.org/wiki/Posix_path_conversion
Comment 10•12 years ago
|
||
It's very likely that if we were using pwd -W it was because the msys magic was insufficient: for example we were writing commands to a file, or the path was embedded like -Ic:/foo. But I believe that there were make functions which performed that translation for us, so it should be possible to apply that to $(CURDIR) just as well as the results of `pwd`.
Assignee | ||
Comment 11•12 years ago
|
||
do you know what those functions might be called or where they might be located?
Comment 12•12 years ago
|
||
core_winabspath
Assignee | ||
Comment 13•12 years ago
|
||
This is a makefile that runs each of the core_*path functions 20,000 times. On my mac I get $ time make -f makefile time-win > /dev/null real 0m1.058s user 0m0.986s sys 0m0.071s $ time make -f makefile time-abs > /dev/null real 0m0.645s user 0m0.516s sys 0m0.126s $ time make -f makefile time-real > /dev/null real 0m1.253s user 0m0.975s sys 0m0.271s
Comment 14•12 years ago
|
||
I think it's clear that this really doesn't matter on any *nix-style system where forking and execing processes is cheap. On Windows however creating a process is orders of magnitude slower, which is I believe what prompted this bug in the first place...
Assignee | ||
Comment 15•12 years ago
|
||
yep! boy is is slow on windows jhford@JHFORD-PC ~ $ time make -f makefile time-win > /dev/null real 0m5.826s user 0m2.136s sys 0m3.258s jhford@JHFORD-PC ~ $ time make -f makefile time-abs > /dev/null real 0m5.828s user 0m1.403s sys 0m3.305s jhford@JHFORD-PC ~ $ time make -f makefile time-real > /dev/null real 0m16.503s user 0m4.398s sys 0m11.542s
Assignee | ||
Comment 16•12 years ago
|
||
jhford@JHFORD-PC ~ $ time make -f makefile time-win > /dev/null real 0m5.375s user 0m1.871s sys 0m3.086s jhford@JHFORD-PC ~ $ time make -f makefile time-abs > /dev/null real 0m5.130s user 0m1.370s sys 0m3.337s jhford@JHFORD-PC ~ $ time make -f makefile time-real > /dev/null real 0m12.917s user 0m3.385s sys 0m9.061s
Assignee | ||
Comment 17•12 years ago
|
||
jhford@JHFORD-PC ~ $ time make -f makefile time-sh > /dev/null real 2m53.549s user 0m21.190s sys 1m41.245s jhford@JHFORD-PC ~ $ time make -f makefile time-abs > /dev/null real 0m1.570s user 0m1.044s sys 0m0.045s On my OS X machine, i changed from 'pwd -W' to 'pwd' and core_winabspath to core_abspth. I got the following results: ~ $ time make -f test.mk time-sh > /dev/null real 0m25.146s user 0m11.530s sys 0m11.410s ~ $ time make -f test.mk time-abs > /dev/null real 0m0.818s user 0m0.713s sys 0m0.094s Optimizing the core_*path functions is a very small win where removing the shell commands is a large win, even on OS X. I think the goal of this bug should be to remove the extra shell calls, leaving core_*path as they are now.
Reporter | ||
Updated•12 years ago
|
Assignee: joey → jhford
Assignee | ||
Comment 18•12 years ago
|
||
This patch replaces $(shell cd $(A_DIR) && pwd) and $(shell cd $(A_DIR) ; pwd) with the correct core_*path functions. I couldn't get the changes in mozilla-central/security/manager to work properly and I think that's because of the nss build system glue.
Attachment #621740 -
Flags: review?(khuey)
Attachment #621740 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 19•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/93e9ec07e3ed
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/93e9ec07e3ed
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•