tech-userlevel archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: make: should -j affect cwd?



In article <20120118134222.GA1222%panix.com@localhost>,
Thor Lancelot Simon  <tls%panix.com@localhost> wrote:
>On Wed, Jan 18, 2012 at 12:21:55AM -0500, James K. Lowden wrote:
>> 
>> People who should know say the code verges on unmaintainable.  
>
>Have a look for yourself -- I don't think the core of it is so bad.
>
>The Sprite make was basically written in a summer, with some care taken
>to use decent implementations of sane datastructures and algorithms.  And
>it worked pretty well, for building the kind of projects that existed
>then, with the kind of Makefiles that existed then.
>
>When it was picked up by CSRG and used to completely rototill the BSD
>build infrastructure, a lot of what had been minor convenience features
>got a really major workout and the program started to Grow.  As did the
>feature set, and not in a planned way, either.
>
>If you really want to do a bunch of work to improve make, I think the
>right place to start would be to look back at the CVS logs and then
>ask some of the developers who've done the most work on make to
>identify the parts that would be most improved by a rewrite.  With a
>lot less work than writing a whole new implementation, you could probably
>do quite a bit of good.

One of the problems with the current make is performance. Doing a simple
ktrace , then kdump | grep NAMI reveals that each path is stat'ed four times
one of them completely redundant, since it was just stat'ed before. There
has been a lot of sandbagging in the path resolution code, to fix bugs by
adding extra checks without understanding the original source of the problem.

The second main issue is variable evaluation, and the recent abuse of
${VAR:Uconstantstring}. Running a profiled make in libc reveals that 20%
of the run is spent in tolower() and ApplyModifiers(). 50% of the run
is spent doing stat().

The following patch ~1/2's the number of stat() calls, by:

- caching the stat in a place that it was not cached before.
- and not using the cache only in the recheck case (not invalidating
  after the first cache use which seems like a horrible hack!)

To fix the second issue, we should look at why ApplyModifiers is expensive
and why we call tolower() so much. Perhaps even look at the uses of :U in
the Makefiles and avoid them where possible.

As for a feature solely missed by our make are pattern rules. If those are
added most simple gmake Makefiles will work.

christos

Index: dir.c
===================================================================
RCS file: /cvsroot/src/usr.bin/make/dir.c,v
retrieving revision 1.63
diff -u -p -u -r1.63 dir.c
--- dir.c       5 Mar 2011 23:57:05 -0000       1.63
+++ dir.c       18 Jan 2012 17:05:46 -0000
@@ -1315,11 +1315,11 @@ Dir_FindFile(const char *name, Lst path)
        if (stb.st_mtime == 0)
                stb.st_mtime = 1;
        entry = Hash_CreateEntry(&mtimes, name, NULL);
+       Hash_SetTimeValue(entry, stb.st_mtime);
        if (DEBUG(DIR)) {
            fprintf(debug_file, "   Caching %s for %s\n", 
Targ_FmtTime(stb.st_mtime),
                    name);
        }
-       Hash_SetTimeValue(entry, stb.st_mtime);
        return (bmake_strdup(name));
     } else {
        if (DEBUG(DIR)) {
@@ -1428,7 +1428,7 @@ Dir_FindHereOrAbove(char *here, char *se
  *-----------------------------------------------------------------------
  */
 int
-Dir_MTime(GNode *gn)
+Dir_MTime(GNode *gn, Boolean recheck)
 {
     char          *fullName;  /* the full pathname of name */
     struct stat          stb;        /* buffer for finding the mod time */
@@ -1481,19 +1481,17 @@ Dir_MTime(GNode *gn)
        fullName = bmake_strdup(gn->name);
     }
 
-    entry = Hash_FindEntry(&mtimes, fullName);
+    if (!recheck)
+       entry = Hash_FindEntry(&mtimes, fullName);
+    else {
+       entry = NULL;
+    }
     if (entry != NULL) {
-       /*
-        * Only do this once -- the second time folks are checking to
-        * see if the file was actually updated, so we need to actually go
-        * to the file system.
-        */
        if (DEBUG(DIR)) {
            fprintf(debug_file, "Using cached time %s for %s\n",
                    Targ_FmtTime(Hash_GetTimeValue(entry)), fullName);
        }
        stb.st_mtime = Hash_GetTimeValue(entry);
-       Hash_DeleteEntry(&mtimes, entry);
     } else if (stat(fullName, &stb) < 0) {
        if (gn->type & OP_MEMBER) {
            if (fullName != gn->path)
@@ -1502,12 +1500,16 @@ Dir_MTime(GNode *gn)
        } else {
            stb.st_mtime = 0;
        }
-    } else if (stb.st_mtime == 0) {
-       /*
-        * 0 handled specially by the code, if the time is really 0, return
-        * something else instead
-        */
-       stb.st_mtime = 1;
+    } else {
+       if (stb.st_mtime == 0) {
+               /*
+                * 0 handled specially by the code, if the time is really 0,
+                * return something else instead
+                */
+               stb.st_mtime = 1;
+       }
+       entry = Hash_CreateEntry(&mtimes, fullName, NULL);
+       Hash_SetTimeValue(entry, stb.st_mtime);
     }
        
     if (fullName && gn->path == NULL) {
Index: dir.h
===================================================================
RCS file: /cvsroot/src/usr.bin/make/dir.h,v
retrieving revision 1.14
diff -u -p -u -r1.14 dir.h
--- dir.h       23 Jan 2009 21:26:30 -0000      1.14
+++ dir.h       18 Jan 2012 17:05:46 -0000
@@ -95,7 +95,7 @@ Boolean Dir_HasWildcards(char *);
 void Dir_Expand(const char *, Lst, Lst);
 char *Dir_FindFile(const char *, Lst);
 int Dir_FindHereOrAbove(char *, char *, char *, int);
-int Dir_MTime(GNode *);
+int Dir_MTime(GNode *, Boolean);
 Path *Dir_AddDir(Lst, const char *);
 char *Dir_MakeFlags(const char *, Lst);
 void Dir_ClearPath(Lst);
Index: job.c
===================================================================
RCS file: /cvsroot/src/usr.bin/make/job.c,v
retrieving revision 1.160
diff -u -p -u -r1.160 job.c
--- job.c       16 Sep 2011 15:38:03 -0000      1.160
+++ job.c       18 Jan 2012 17:05:47 -0000
@@ -1226,7 +1226,7 @@ Job_CheckCommands(GNode *gn, void (*abor
            Var_Set(IMPSRC, Var_Value(TARGET, gn, &p1), gn, 0);
            if (p1)
                free(p1);
-       } else if (Dir_MTime(gn) == 0 && (gn->type & OP_SPECIAL) == 0) {
+       } else if (Dir_MTime(gn, 0) == 0 && (gn->type & OP_SPECIAL) == 0) {
            /*
             * The node wasn't the target of an operator we have no .DEFAULT
             * rule to go on and the target doesn't already exist. There's
Index: make.c
===================================================================
RCS file: /cvsroot/src/usr.bin/make/make.c,v
retrieving revision 1.84
diff -u -p -u -r1.84 make.c
--- make.c      16 Sep 2011 15:38:04 -0000      1.84
+++ make.c      18 Jan 2012 17:05:48 -0000
@@ -221,7 +221,7 @@ Make_OODate(GNode *gn)
      * doesn't depend on their modification time...
      */
     if ((gn->type & (OP_JOIN|OP_USE|OP_USEBEFORE|OP_EXEC)) == 0) {
-       (void)Dir_MTime(gn);
+       (void)Dir_MTime(gn, 0);
        if (DEBUG(MAKE)) {
            if (gn->mtime != 0) {
                fprintf(debug_file, "modified %s...", Targ_FmtTime(gn->mtime));
@@ -406,7 +406,7 @@ MakeFindChild(void *gnp, void *pgnp)
     GNode          *gn = (GNode *)gnp;
     GNode          *pgn = (GNode *)pgnp;
 
-    (void)Dir_MTime(gn);
+    (void)Dir_MTime(gn, 0);
     Make_TimeStamp(pgn, gn);
     pgn->unmade--;
 
@@ -574,7 +574,7 @@ MakeHandleUse(void *cgnp, void *pgnp)
 time_t
 Make_Recheck(GNode *gn)
 {
-    time_t mtime = Dir_MTime(gn);
+    time_t mtime = Dir_MTime(gn, 1);
 
 #ifndef RECHECK
     /*
@@ -1336,7 +1336,7 @@ Make_ExpandUse(Lst targs)
            *eon = ')';
        }
 
-       (void)Dir_MTime(gn);
+       (void)Dir_MTime(gn, 0);
        Var_Set(TARGET, gn->path ? gn->path : gn->name, gn, 0);
        Lst_ForEach(gn->children, MakeUnmark, gn);
        Lst_ForEach(gn->children, MakeHandleUse, gn);



Home | Main Index | Thread Index | Old Index