Page MenuHomeFreeBSD

Skip make includes for (most) directories without includes
Needs ReviewPublic

Authored by arichardson on Oct 14 2020, 10:55 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 1 2024, 11:53 AM
Unknown Object (File)
Dec 29 2023, 5:37 AM
Unknown Object (File)
Dec 29 2023, 4:10 AM
Unknown Object (File)
Dec 20 2023, 4:43 AM
Unknown Object (File)
Dec 4 2023, 8:11 AM
Unknown Object (File)
Oct 14 2023, 3:49 PM
Unknown Object (File)
Oct 3 2023, 5:28 PM
Unknown Object (File)
Aug 25 2023, 8:04 AM

Details

Summary

I got frustrated by the amount of time that buildworld spends during make
includes when doing -j1 builds to debug build failures. This change adds
a new SUBDIR_WITH_INCS= variable that can be set to only run make includes
on the subdirs listed in that variable or completely skip it if empty.
If it is not set, it will run on all subdirs except "test" and "tests".

-j1 includes step before: 70.02 real 47.59 user 16.20 sys
-j8 includes step before: 18.41 real 75.07 user 29.17 sys
-j36 includes step before: 2.58 real 61.16 user 7.03 sys
-j1 includes step after: 15.38 real 8.63 user 4.38 sys
-j8 includes step after: 5.68 real 11.41 user 7.18 sys
-j36 includes step after: 2.33 real 13.01 user 2.10 sys

Tracing the make includes step gives me 2198 execve calls with this
patch whereas before we were starting 5960 processes.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 34154
Build 31311: arc lint + arc unit

Event Timeline

arichardson created this revision.

This looks like a nice improvement and the error/warning bits resolve my initial concern about maintenance.

share/mk/bsd.subdir.mk
162

Is the == "${__dir}" needed?

This revision is now accepted and ready to land.Oct 14 2020, 5:08 PM
share/mk/bsd.subdir.mk
162

Good catch, :M can be used as a boolean expression.

Please don't. This is such a huge niche micro optimization. -j1 isn't normal and I don't see this being worth the maintenance cost.

Plus it breaks downstream builds and probably ports.

Stop hardcoding stuff that is harder to maintain!

This revision now requires changes to proceed.Oct 15 2020, 12:39 AM

Please don't. This is such a huge niche micro optimization. -j1 isn't normal and I don't see this being worth the maintenance cost.

Plus it breaks downstream builds and probably ports.

Stop hardcoding stuff that is harder to maintain!

An incremental build (without any changes!) takes 110 seconds at -j8. With this change it's 99 seconds. That's almost 10% of the total time time spent doing completely useless stuff like looking for includes in share/ or tests/.

But sure I'm happy to stop working on trying to improve this unnecessarily slow build system.

Sorry about that. I didn't communicate my concerns in a proper way. (As noted in private email).
Here I am worried about quietly not installing headers leading to silent bugs like a downstream compilation using a header it finds in another directory instead.
I missed the build time assertion in bsd.incs.mk. That helps a lot to mitigate my concerns as it yells at people to fix it. Having taken a second look I support this and think the implementation is good too.
We have a similar problem with CONFS that causes make installconfig to take forever for mergemaster and etcupdate. It would be nice if that was worked into this too.

Now some other thoughts:
It's been hard to reduce treewalks but the ones we do have to do we can reduce the overhead once there. Like not having WHATEVER!= executed everytime a Makefile is executed, or reading .depend files, but only when it is absolutely necessary. 'make print-dir' is an example that aims to reduce all overhead. That runs in 70 seconds with -j1 for me and 14 seconds with -j8. I think a lot of the 14 seconds is bmake overhead.

I've often thought about some kind of pre-buildworld script that does the treewalk and builds a database/Makefile/whatever that can avoid the later make recursion and handle this review automatically.
It would be hard to ensure it is always up-to-date and not redundantly ran. It could help with this review, CONFS, maybe reduce the hardcoding in Makefile.inc1, and maybe automate some of the SUBDIR_PARALLEL support.

share/mk/bsd.subdir.mk
156–159

Can we make it optional and default on? I don't think we need a MK_FOO option just a .if !defined(SUBDIR_INCS_ALL_DIRS) or something wrapping the logic here. Then at least it will be easier to flip later if we find a problem or complaints.

usr.sbin/Makefile
225

Missing bsnmpd:

--- all_subdir_usr.sbin/bsnmpd ---
--- bridge_snmp.pico ---
/bolt/src/github.com/DankBSD/base/usr.sbin/bsnmpd/modules/snmp_bridge/bridge_snmp.c:50:10: fatal error: 'bsnmp/snmpmod.h' file not found
#include <bsnmp/snmpmod.h>
         ^~~~~~~~~~~~~~~~~
usr.sbin/Makefile
225

Full working diff

--- i/usr.sbin/Makefile
+++ w/usr.sbin/Makefile
@@ -215,6 +214,6 @@ SUBDIR.${MK_TESTS}+=	tests
 
 SUBDIR_PARALLEL=
 # Skip make includes for this part of the tree
-SUBDIR_WITH_INCS=
+SUBDIR_WITH_INCS=	${SUBDIR:Mbsnmpd}
 
 .include <bsd.subdir.mk>
diff --git i/usr.sbin/bsnmpd/Makefile w/usr.sbin/bsnmpd/Makefile
index 632753ddc9e..487f690eee5 100644
--- i/usr.sbin/bsnmpd/Makefile
+++ w/usr.sbin/bsnmpd/Makefile
@@ -5,4 +5,6 @@ SUBDIR=	gensnmptree	\
 	modules		\
 	tools
 
+SUBDIR_WITH_INCS=	bsnmpd modules
+
 .include <bsd.subdir.mk>
diff --git i/usr.sbin/bsnmpd/modules/Makefile w/usr.sbin/bsnmpd/modules/Makefile
index 6640bb62d11..772c1b213b4 100644
--- i/usr.sbin/bsnmpd/modules/Makefile
+++ w/usr.sbin/bsnmpd/modules/Makefile
@@ -34,4 +34,6 @@ INCSDIR= ${INCLUDEDIR}/bsnmp
 
 SUBDIR_TARGETS+=	smilint
 
+SUBDIR_WITH_INCS=	snmp_bridge snmp_mibII ${SUBDIR:Msnmp_netgraph}
+
 .include <bsd.prog.mk>
+SUBDIR_WITH_INCS=	${SUBDIR:Mbsnmpd}

should be

+SUBDIR_WITH_INCS=	bsndmpd
arichardson marked 4 inline comments as done.
  • Fix check for missing includes for directories more than one level below (caused me to miss libpam and bsnmpd)
  • Add a variable to run includes for all dirs

Sorry about that. I didn't communicate my concerns in a proper way. (As noted in private email).
Here I am worried about quietly not installing headers leading to silent bugs like a downstream compilation using a header it finds in another directory instead.
I missed the build time assertion in bsd.incs.mk. That helps a lot to mitigate my concerns as it yells at people to fix it. Having taken a second look I support this and think the implementation is good too.
We have a similar problem with CONFS that causes make installconfig to take forever for mergemaster and etcupdate. It would be nice if that was worked into this too.

There don't seem to be too many directories that have confs so I can try to also add a SUBDIR_WITH_CONFS variant of this patch.

Now some other thoughts:
It's been hard to reduce treewalks but the ones we do have to do we can reduce the overhead once there. Like not having WHATEVER!= executed everytime a Makefile is executed, or reading .depend files, but only when it is absolutely necessary. 'make print-dir' is an example that aims to reduce all overhead. That runs in 70 seconds with -j1 for me and 14 seconds with -j8. I think a lot of the 14 seconds is bmake overhead.

I've often thought about some kind of pre-buildworld script that does the treewalk and builds a database/Makefile/whatever that can avoid the later make recursion and handle this review automatically.
It would be hard to ensure it is always up-to-date and not redundantly ran. It could help with this review, CONFS, maybe reduce the hardcoding in Makefile.inc1, and maybe automate some of the SUBDIR_PARALLEL support.

I guess another option would be to always do one full tree walk to generate a file with the list of directories that uses CONFS/INCS/etc., and then read that in the later build phases, but I feel like that would be quite difficult to get right.