On Mon, Jan 01, 2018 at 05:39:56PM -0800, Noah Misch wrote:
> On Sat, Dec 16, 2017 at 08:06:05PM -0800, Noah Misch wrote:
> > On Mon, Nov 06, 2017 at 12:07:52AM -0800, Noah Misch wrote:
>
> > I now see similar trouble from multiple
> > "make" processes running "make -C contrib/test_decoding install"
> > concurrently.
> > This is a risk for any directory named in an EXTRA_INSTALL variable of more
> > than one makefile. Under the right circumstances, this would affect
> > contrib/hstore and others in addition to contrib/test_decoding. That brings
> > me back to the locking idea:
> >
> > > The problem of multiple "make" processes in a directory (especially
> > > src/port)
> > > shows up elsewhere. In a cleaned tree, "make -j -C src/bin" or "make -j
> > > installcheck-world" will do it. For more-prominent use cases,
> > > src/Makefile
> > > prevents this with ".NOTPARALLEL:" and building first the directories
> > > that are
> > > frequent submake targets. Perhaps we could fix the general problem with
> > > directory locking; targets that call "$(MAKE) -C FOO" would first sleep
> > > until
> > > FOO's lock is available. That could be tricky to make robust.
> Performance has been the principal snare. I value "make -j" being fast when
> there's little to rebuild, but that shell script approach slowed an empty
> build by 340% (GNU/Linux w/ SSD) to 2300% (Cygwin). In a build having nothing
> to do, merely adding a no-op wrapper around "make -C" (does nothing but
> execvp() the real GNU make) slowed the build by over 10%[1]. To get what I
> considered decent performance took several design changes:
> 3. Lock only directories known to be entered more than once per top-level
> target. Preliminarily, this reduced the 147 lock acquisitions to 25.
>
> I regret that (3) entails ongoing maintenance of a whitelist of such
> directories; adding a "make -C" call can expand the list. However, I can
> automate verification of that whitelist.
As I developed this more, I found it dissatisfying. To verify the whitelist,
one empties the whitelist, rebuilds each makefile target, and reassembles the
whitelist from the list of directories visited more than once within a target.
That's too expensive to run all the time (e.g. with every "make check"). I
could make a buildfarm animal turn red when the whitelist is out of date, but
I wouldn't enjoy discovering or fixing whitelist drift that way. Hence, I'm
back to preferring fixes targeting the known problems:
- Fix conflicting writes in src/test/regress:
https://postgr.es/m/flat/20181224034411.GA3224776%40rfd.leadboat.com
- Fix conflicting EXTRA_INSTALL, detailed above. In the MAKELEVEL-0 part of
the temp-install Makefile target, serially process all applicable
EXTRA_INSTALL. See attached patch. On my usual development system, this
adds <2s to "make check-world" and <0.1s to "make check" or "make -C
contrib/earthdistance check".
To corroborate the sufficiency of those two patches, I ran 110 iterations of
"make -j40" and "make -j40 check-world" without encountering a failure
attributable to parallel make. I also ran those targets at excess parallelism
(-j120) and audited the list of commands appearing more than once:
make clean && make -j120 2>&1 | sort | uniq -c | sort -rn
make -j120 check-world 2>&1 | sort | uniq -c | sort -rn
Unlike the locking method, this doesn't fix clean-tree "make -j -C src/bin" or
clean-tree "make -j installcheck-world".
Thanks,
nm
--- a/GNUmakefile.in
+++ b/GNUmakefile.in
@@ -63,10 +63,12 @@ distclean maintainer-clean:
@rm -rf autom4te.cache/
rm -f config.cache config.log config.status GNUmakefile
+check check-tests installcheck installcheck-parallel installcheck-tests:
CHECKPREP_TOP=src/test/regress
check check-tests installcheck installcheck-parallel installcheck-tests:
submake-generated-headers
$(MAKE) -C src/test/regress $@
$(call recurse,check-world,src/test src/pl src/interfaces/ecpg contrib
src/bin,check)
+$(call recurse,checkprep, src/test src/pl src/interfaces/ecpg contrib src/bin)
$(call recurse,installcheck-world,src/test src/pl src/interfaces/ecpg contrib
src/bin,installcheck)
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -19,7 +19,7 @@
#
# Meta configuration
-standard_targets = all install installdirs uninstall distprep clean distclean
maintainer-clean coverage check installcheck init-po update-po
+standard_targets = all install installdirs uninstall distprep clean distclean
maintainer-clean coverage check checkprep installcheck init-po update-po
# these targets should recurse even into subdirectories not being built:
standard_always_targets = distprep clean distclean maintainer-clean
@@ -390,11 +390,17 @@ ifeq ($(MAKELEVEL),0)
rm -rf '$(abs_top_builddir)'/tmp_install
$(MKDIR_P) '$(abs_top_builddir)'/tmp_install/log
$(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install
install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
+ $(MAKE) -j1 $(if $(CHECKPREP_TOP),-C $(CHECKPREP_TOP),) checkprep
>>'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
endif
- $(if $(EXTRA_INSTALL),for extra in $(EXTRA_INSTALL); do $(MAKE) -C
'$(top_builddir)'/$$extra DESTDIR='$(abs_top_builddir)'/tmp_install install
>>'$(abs_top_builddir)'/tmp_install/log/install.log || exit; done)
endif
endif
+# Tasks to run serially at the end of temp-install. Some EXTRA_INSTALL
+# entries appear more than once in the tree, and parallel installs of the same
+# file can fail with EEXIST.
+checkprep:
+ $(if $(EXTRA_INSTALL),for extra in $(EXTRA_INSTALL); do $(MAKE) -C
'$(top_builddir)'/$$extra DESTDIR='$(abs_top_builddir)'/tmp_install install ||
exit; done)
+
PROVE = @PROVE@
# There are common routines in src/test/perl, and some test suites have
# extra perl modules in their own directory.
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -435,7 +435,7 @@ endif
endif # PGXS
ifndef NO_TEMP_INSTALL
-temp-install: EXTRA_INSTALL+=$(subdir)
+checkprep: EXTRA_INSTALL+=$(subdir)
endif
