Last Comment Bug 747448 - makefile optimization: investigate replacing $(shell cd $(DEPTH); pwd) with builtin $(abspath)
: makefile optimization: investigate replacing $(shell cd $(DEPTH); pwd) with b...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: John Ford [:jhford]
:
Mentors:
Depends on:
Blocks: 748452
  Show dependency treegraph
 
Reported: 2012-04-20 10:14 PDT by Joey Armstrong [:joey]
Modified: 2012-05-10 20:45 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
unit test for abspath/realpath (17.67 KB, patch)
2012-04-26 14:55 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
makefile with current core_* functions (858 bytes, text/plain)
2012-05-02 09:45 PDT, John Ford [:jhford]
no flags Details
pass one: leave core_*path in place, use abspath/realpath instead of core_*path in the functions (1.29 KB, text/plain)
2012-05-02 10:09 PDT, John Ford [:jhford]
no flags Details
comparing $(shell cd ?? && pwd) to core_winabspath (1.13 KB, text/plain)
2012-05-02 11:27 PDT, John Ford [:jhford]
no flags Details
convert pwd calls to core_*path functions (4.55 KB, patch)
2012-05-07 14:30 PDT, John Ford [:jhford]
khuey: review+
Details | Diff | Splinter Review

Description Joey Armstrong [:joey] 2012-04-20 10:14:09 PDT

    
Comment 1 Joey Armstrong [:joey] 2012-04-20 10:26:56 PDT
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 Ted Mielczarek [:ted.mielczarek] 2012-04-20 12:52:26 PDT
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?
Comment 3 Joey Armstrong [:joey] 2012-04-20 13:50:13 PDT
also replace $(shell pwd) with $(CURDIR)
Comment 4 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-04-23 07:43:17 PDT
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.
Comment 5 Joey Armstrong [:joey] 2012-04-24 12:35:10 PDT
core_abspath, core_relpath:
http://mxr.mozilla.org/mozilla-central/source/config/config.mk#84
toolkit/locales/l10n.mk
Comment 6 Joey Armstrong [:joey] 2012-04-26 14:55:46 PDT
Created attachment 618814 [details] [diff] [review]
unit test for abspath/realpath

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)
Comment 7 Joey Armstrong [:joey] 2012-04-26 14:59:46 PDT
(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.
Comment 8 Joey Armstrong [:joey] 2012-04-26 15:07:52 PDT
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
Comment 9 John Ford [:jhford] 2012-04-30 15:44:34 PDT
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 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-04-30 17:39:46 PDT
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`.
Comment 11 John Ford [:jhford] 2012-05-01 12:24:48 PDT
do you know what those functions might be called or where they might be located?
Comment 12 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-05-01 12:30:51 PDT
core_winabspath
Comment 13 John Ford [:jhford] 2012-05-02 09:45:12 PDT
Created attachment 620354 [details]
makefile with current core_* functions

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 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-05-02 09:48:59 PDT
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...
Comment 15 John Ford [:jhford] 2012-05-02 10:05:41 PDT
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
Comment 16 John Ford [:jhford] 2012-05-02 10:09:47 PDT
Created attachment 620364 [details]
pass one: leave core_*path in place, use abspath/realpath instead of core_*path in the functions

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
Comment 17 John Ford [:jhford] 2012-05-02 11:27:51 PDT
Created attachment 620392 [details]
comparing $(shell cd ?? && pwd) to core_winabspath

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.
Comment 18 John Ford [:jhford] 2012-05-07 14:30:43 PDT
Created attachment 621740 [details] [diff] [review]
convert pwd calls to core_*path functions

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.
Comment 20 Ed Morley [:emorley] 2012-05-08 03:19:17 PDT
https://hg.mozilla.org/mozilla-central/rev/93e9ec07e3ed

Note You need to log in before you can comment on or make changes to this bug.