pack-objects: Add runtime detection of online CPU's

Packing objects can be done in parallell nowadays, but it's only done if the config option pack.threads is set to a value above 1. Because of that, the code-path used is often not the most optimal one.

This patch adds a routine to detect the number of online CPU's at runtime (online_cpus()). When pack.threads (or --threads=) is given a value of 0, the number of threads is set to the number of online CPU's. This feature is also documented.

As per Nicolas Pitre's recommendations, the default is still to run pack-objects single-threaded unless explicitly activated, either by configuration or by command line parameter.

The routine online_cpus() is a rework of "numcpus.c", written by one Philip Willoughby <[email protected]>. numcpus.c is in the public domain and can presently be downloaded from
http://csgsoft.doc.ic.ac.uk/numcpus/

---

This patch was built on todays master as of 5 minutes ago (40aab8119f38c622f58d8e612e7a632eb1f3ded2).
As far as I understood all of Nicolas' comments to my original patch and the one sent in by Mr Casey, this implements all the suggestions made to both sets.

Documentation/config.txt           |    2 +
Documentation/git-pack-objects.txt |    2 +
Makefile                           |    2 +-
builtin-pack-objects.c             |   13 ++++++-----
thread-utils.c                     |   39 ++++++++++++++++++++++++++++++++++++
thread-utils.h                     |   19 +++++++++++++++++
6 files changed, 70 insertions(+), 7 deletions(-)

create mode 100644 thread-utils.c
create mode 100644 thread-utils.h

index f9bdb16..e9f26ed 100644
@@ -756,6 +756,8 @@ pack.threads::

warning. This is meant to reduce packing time on multiprocessor machines. The required amount of memory for the delta search window is however multiplied by the number of threads.
+ Specifying 0 will cause git to auto-detect the number of CPU's
+ and set the number of threads accordingly.

pack.indexVersion::
Specify the default pack index version. Valid values are 1 for

index 8353be1..5c1bd3b 100644
@@ -177,6 +177,8 @@ base-name::

This is meant to reduce packing time on multiprocessor machines. The required amount of memory for the delta search window is however multiplied by the number of threads.
+ Specifying 0 will cause git to auto-detect the number of CPU's
+ and set the number of threads accordingly.

--index-version=<version>[,<offset>]::
This is intended to be used by the test suite only. It allows

index 92341c4..9ea378a 100644
@@ -306,7 +306,7 @@ DIFF_OBJS = \
 

LIB_OBJS = \
	blob.o commit.o connect.o csum-file.o cache-tree.o base85.o \
-	date.o diff-delta.o entry.o exec_cmd.o ident.o \
+	date.o diff-delta.o entry.o exec_cmd.o ident.o thread-utils.o \
	pretty.o interpolate.o hash.o \
	lockfile.o \
	patch-ids.o \

index acb0555..a7ffb53 100644
@@ -14,10 +14,7 @@

#include "revision.h"
#include "list-objects.h"
#include "progress.h"
-
-#ifdef THREADED_DELTA_SEARCH
-#include <pthread.h>
-#endif
+#include "thread-utils.h"

static const char pack_usage[] = "\
git-pack-objects [{ -q | --progress | --all-progress }] \n\
@@ -1861,7 +1858,7 @@ static int git_pack_config(const char *k, const char *v)

	}
	if (!strcmp(k, "pack.threads")) {
		delta_search_threads = git_config_int(k, v);
-		if (delta_search_threads < 1)
+		if (delta_search_threads < 0)
			die("invalid number of threads specified (%d)",
			    delta_search_threads);
#ifndef THREADED_DELTA_SEARCH
@@ -2076,6 +2073,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
	if (!pack_compression_seen && core_compression_seen)
		pack_compression_level = core_compression_level;

+ if (!delta_search_threads) /* --threads=0 means autodetect */
+ delta_search_threads = online_cpus();
+

	progress = isatty(2);
	for (i = 1; i < argc; i++) {
		const char *arg = argv[i];
@@ -2130,7 +2130,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
		if (!prefixcmp(arg, "--threads=")) {
			char *end;
			delta_search_threads = strtoul(arg+10, &end, 0);
-			if (!arg[10] || *end || delta_search_threads < 1)
+
+			if (!arg[10] || *end || delta_search_threads < 0)
				usage(pack_usage);

#ifndef THREADED_DELTA_SEARCH
if (delta_search_threads > 1)

new file mode 100644
index 0000000..b19243c
@@ -0,0 +1,39 @@
+#include "thread-utils.h"
+
+/*
+ * By doing this in two steps we can at least get
+ * get the function to be somewhat coherent, even
+ * with this disgusting nest of #ifdefs.
+ */
+#ifndef _SC_NPROCESSORS_ONLN
+# ifdef _SC_NPROC_ONLN
+#  define _SC_NPROCESSORS_ONLN _SC_NPROC_ONLN
+# elif defined _SC_CRAY_NCPU
+#  define _SC_NPROCESSORS_ONLN _SC_CRAY_NCPU
+# endif
+#endif
+int online_cpus(void)
+{
+#ifdef THREADED_DELTA_SEARCH
+# ifdef _SC_NPROCESSORS_ONLN
+	long ncpus;
+
+	if ((ncpus = (long)sysconf(_SC_NPROCESSORS_ONLN)) > 0)
+		return (int)ncpus;
+# else
+#  ifdef _WIN32
+	SYSTEM_INFO info;
+	GetSystemInfo(&info);
+
+	return (int)info.dwNumberOfProcessors;
+#  endif /* _WIN32 */
+#  if defined(hpux) || defined(__hpux) || defined(_hpux)
+	struct pst_dynamic psd;
+
+	if (!pstat_getdynamic(&psd, sizeof(psd), (size_t)1, 0))
+		return (int)psd.psd_proc_cnt;
+#  endif /* hpux */
+# endif /* _SC_NPROCESSORS_ONLN */
+#endif /* THREADED_DELTA_SEARCH */
+	return 1;
+}

new file mode 100644
index 0000000..53754b3
@@ -0,0 +1,19 @@
+#ifndef THREAD_COMPAT_H
+#define THREAD_COMPAT_H
+
+#include "cache.h"
+
+#ifdef THREADED_DELTA_SEARCH
+#include <pthread.h>
+# ifdef _WIN32
+#  define WIN32_LEAN_AND_MEAN
+# include <windows.h>
+# endif
+# if defined(hpux) || defined(__hpux) || defined(_hpux)
+#  include <sys/pstat.h>
+# endif
+#endif
+
+extern int online_cpus(void);
+
+#endif /* THREAD_COMPAT_H */

1.5.4.rc5.11.g0eab8


-- 
Andreas Ericsson                   [email protected]
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

"we can at least get get the ..."

An extra "get" snuck in there.

Michael

Hi,

$ taskset 0x3 git gc ...

Since you can do roughly the same by saying "git pack-objects --threads=2", I'd rather not add a GNU/Linux specific hack for this.

Andreas Ericsson                   [email protected]
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

	if (delta_search_threads != 1)
		warning("no threads support, ignoring %s", k);

I changed this to '!= 1' since that is the only time the user gets what they asked for when THREADED_DELTA_SEARCH is not enabled. If the user requested nthreads == ncpus by setting delta_search_threads = 0, I think we should let the user know that thread support is not enabled, and we are ignoring their request.

Same comment as above about warning when delta_search_threads != 1.

-brandon

---
 builtin-pack-objects.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

index 586ae11..5a22f49 100644
@@ -2239,6 +2239,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 #ifdef THREADED_DELTA_SEARCH
 	if (!delta_search_threads)	/* --threads=0 means autodetect */
 		delta_search_threads = online_cpus();
+	if (progress)
+		fprintf(stderr, "Using %d pack threads.\n",
+			delta_search_threads);
 #endif
 
 	prepare_packed_git();

1.5.4.2.199.g0941

Please send in a fix-up patch to remove it. I noticed it while reviewing the patch, and even commented on it, but I somehow forgot that this leftover debugging message disqualified the series from 'next' when I was merging topics to 'next'.

Are you sure you are thinking of the same message? This one was submitted in a patch by itself, and I didn't see any followup discussion. It's in next as:

6c723f5 pack-objects: Print a message describing the number of
threads for packing

-Peff

Ah, no.

But now you mention it, I tend to agree with you. This is primarily of interest for git developers and I do not think the end users would care. Maybe under --verbose or --debug option (but I do not think we have --debug option anywhere).

I wrote up a --verbose patch, but it just seemed silly. Who would actually turn it on?

How about this instead?

-- >8 --
pack-objects: show "using N threads" only when autodetected

Every other case is uninteresting, since either:
- it is the default of 1, in which case we are always just printing "using 1 thread"
- it is whatever the user set it to, in which case they already know

But with --threads=0, they might want to be informed of the number of CPUs detected.
---
If we ever change the default to autodetect, this logic might change, but we can deal with that then.

BTW, I seem to remember some work recently on coalescing hunks in merge conflicts separated by a small number of lines. It seems to me that the diff below would be easier to read with a similar tactic.

 builtin-pack-objects.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

index b70b2e5..516eb24 100644
@@ -2236,11 +2236,12 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		die("--thin cannot be used to build an indexable pack.");
 
 #ifdef THREADED_DELTA_SEARCH
-	if (!delta_search_threads)	/* --threads=0 means autodetect */
+	if (!delta_search_threads) {	/* --threads=0 means autodetect */
 		delta_search_threads = online_cpus();
-	if (progress)
-		fprintf(stderr, "Using %d pack threads.\n",
-			delta_search_threads);
+		if (progress)
+			fprintf(stderr, "Using %d pack threads.\n",
+					delta_search_threads);
+	}
 #endif
 
 	prepare_packed_git();

1.5.4.3.340.gda2e.dirty

FWIW, it wasn't in the original patch I sent in, but only in the one sent by Brandon Casey. I believe that may have added to the confusion.

I like Jeff's suggestion of only showing it when we autodetect though, but I won't have time to send a patch until this weekend at the earliest.

Andreas Ericsson                   [email protected]
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

I like the message and thought it was useful especially for non-developers.

Even if the number of threads was not auto-detected, it is a confirmation that the number of threads used is the number of threads configured.

For example, it seems easy to do this:

git config pack.thread 4
git repack

The user would immediately know something was wrong when they saw the message "Using 1 pack threads" instead of the "4" they thought they configured. Also, since it's only printed in the THREADED_DELTA_SEARCH case, it's also a confirmation that this option was indeed used for a particular build of git.

Mainly, I thought it was a harmless message that other users would "enjoy" seeing, but if others disagree, I won't argue. Notice I quoted "enjoy" to emphasize it.

I'd also say that if the message is too noisy in the "user explicitly assigned number of threads" case, then it's just as noisy in the "auto assign" case, so just remove the message completely.

We're saying:

If I set pack.threads to 4, I know git is using 4 threads to repack since I told it to use 4 threads. I don't need to see a noisy message telling me so.

If I set pack.threads to 0, I know git is using 4 threads to repack since I have 4 cpus. I don't need to see a noisy message telling me so.

-brandon

There are hundreds of ways the user can fail to configure git correctly; I don't think it's worth printing output so verbose that the user can manually check that every config option was respected.

At any rate, I think your reasoning is not a good guideline for user output. You are making output to notice a mistake that happens one time (the time of config), but you are showing the output to the reader many times (every time they repack from here to eternity). But there are also mistakes that could be made in the "many times" case, and you are taking their attention away from that.

In the case of repack, it is probably not a big deal. But in the case of 'push', for example, I think we want as little output as possible taking attention away from the useful information: which refs were pushed, which were rejected, and so forth. That's why Nicolas made the pack-objects output considerably more terse last November.

Same reasoning as above. You configure THREADED_DELTA_SEARCH once; you don't need to check that it was enabled every time you repack.

I am not opposed to that; the "auto assign" case is nice to see the first time you repack ("did it find all of my CPUs?"), but yes, it probably will be the same every time after.

-Peff

It's hard coming up with examples that someone cannot take to the N'th degree and make look ridiculous. Maybe impossible.

My reasoning was that I liked it. I thought it was interesting and useful to show the number of threads that pack-objects was using. The example I gave was to show how it could be useful to non-developers since Junio suggested that the information was primarily of interest for git developers and he didn't think that end users would care.

In any case, I'm not attached to the patch. I am thankful for Nicolas's patch though, since it forced me to learn the relationship between progress and pack_to_stdout.

-brandon

I know. I didn't mean to say "this message is ridiculous." I meant to say "we should strive for consistency in interface, and I don't see anything that makes this config option any different than, say, core.followSymlinks."

-Peff

Maybe a message for any unrecognized config option should be displayed instead.

This is enjoyable maybe the first time, but that might get useless/annoying after a while. I think that displaying it in the autodetection case is a good compromize, and then simply specifying the number of threads explicitly will silence it.

Also, I think that such message should absolutely not be sent over in the context of a fetch/clone. This is a local matter only, and should be displayed only when those progress messages are meant for the local user.

Therefore I propose this patch instead:

index b70b2e5..6dcb4e2 100644
@@ -2236,11 +2236,12 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		die("--thin cannot be used to build an indexable pack.");
 
 #ifdef THREADED_DELTA_SEARCH
-	if (!delta_search_threads)	/* --threads=0 means autodetect */
+	if (!delta_search_threads) {	/* --threads=0 means autodetect */
 		delta_search_threads = online_cpus();
-	if (progress)
-		fprintf(stderr, "Using %d pack threads.\n",
-			delta_search_threads);
+		if (progress > pack_to_stdout)
+			fprintf(stderr, "Using %d pack threads.\n",
+				delta_search_threads);
+	}
 #endif
 
 	prepare_packed_git();

I think that is generally useful, though it is somewhat hard with our config parsing mechanism. You could handle specific cases like "I'm in git_pack_config, this is a pack.* variable, and I don't understand it". But it would be nice to have a general "no callback claimed to understand this variable" which I think is impossible (since we do things like parsing the config just to grab a small part of it).

I think that is a nice addition to my patch, but I am also fine with simply reverting 6c723f5e entirely, as Brandon suggested.

-Peff

From: Andreas Ericsson <[email protected]>

Packing objects can be done in parallell nowadays, but it's only done if the config option pack.threads is set to a value above 1. Because of that, the code-path used is often not the most optimal one.

This patch adds a routine to detect the number of online CPU's at runtime (online_cpus()). When pack.threads (or --threads=) is given a value of 0, the number of threads is set to the number of online CPU's. This feature is also documented.

As per Nicolas Pitre's recommendations, the default is still to run pack-objects single-threaded unless explicitly activated, either by configuration or by command line parameter.

The routine online_cpus() is a rework of "numcpus.c", written by one Philip Willoughby <[email protected]>. numcpus.c is in the public domain and can presently be downloaded from
http://csgsoft.doc.ic.ac.uk/numcpus/

---

I reworked this patch from Andreas for detecting the number of online CPU's. I kept the commit message and the Signed-off-by and added my own. I'm not sure what the procedure is here.

-brandon

 Documentation/config.txt           |    2 +
 Documentation/git-pack-objects.txt |    2 +
 Makefile                           |    1 +
 builtin-pack-objects.c             |   14 +++++++---
 thread-utils.c                     |   48 ++++++++++++++++++++++++++++++++++++
 thread-utils.h                     |    6 ++++
 6 files changed, 69 insertions(+), 4 deletions(-)
 create mode 100644 thread-utils.c
 create mode 100644 thread-utils.h

index 7b67671..62b697c 100644
@@ -808,6 +808,8 @@ pack.threads::
 	warning. This is meant to reduce packing time on multiprocessor
 	machines. The required amount of memory for the delta search window
 	is however multiplied by the number of threads.
+	Specifying 0 will cause git to auto-detect the number of CPU's
+	and set the number of threads accordingly.
 
 pack.indexVersion::
 	Specify the default pack index version.  Valid values are 1 for

index 8353be1..5c1bd3b 100644
@@ -177,6 +177,8 @@ base-name::
 	This is meant to reduce packing time on multiprocessor machines.
 	The required amount of memory for the delta search window is
 	however multiplied by the number of threads.
+	Specifying 0 will cause git to auto-detect the number of CPU's
+	and set the number of threads accordingly.
 
 --index-version=<version>[,<offset>]::
 	This is intended to be used by the test suite only. It allows

index d33a556..2dc8247 100644
@@ -741,6 +741,7 @@ endif
 ifdef THREADED_DELTA_SEARCH
 	BASIC_CFLAGS += -DTHREADED_DELTA_SEARCH
 	EXTLIBS += -lpthread
+	LIB_OBJS += thread-utils.o
 endif
 
 ifeq ($(TCLTK_PATH),)

index d2bb12e..586ae11 100644
@@ -16,6 +16,7 @@
 #include "progress.h"
 
 #ifdef THREADED_DELTA_SEARCH
+#include "thread-utils.h"
 #include <pthread.h>
 #endif
 

@@ -1852,11 +1853,11 @@ static int git_pack_config(const char *k, const char *v)
 	}
 	if (!strcmp(k, "pack.threads")) {
 		delta_search_threads = git_config_int(k, v);
-		if (delta_search_threads < 1)
+		if (delta_search_threads < 0)
 			die("invalid number of threads specified (%d)",
 			    delta_search_threads);
 #ifndef THREADED_DELTA_SEARCH
-		if (delta_search_threads > 1)
+		if (delta_search_threads != 1)
 			warning("no threads support, ignoring %s", k);
 #endif
 		return 0;

@@ -2122,10 +2123,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		if (!prefixcmp(arg, "--threads=")) {
 			char *end;
 			delta_search_threads = strtoul(arg+10, &end, 0);
-			if (!arg[10] || *end || delta_search_threads < 1)
+			if (!arg[10] || *end || delta_search_threads < 0)
 				usage(pack_usage);
 #ifndef THREADED_DELTA_SEARCH
-			if (delta_search_threads > 1)
+			if (delta_search_threads != 1)
 				warning("no threads support, "
 					"ignoring %s", arg);
 #endif

@@ -2235,6 +2236,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	if (!pack_to_stdout && thin)
 		die("--thin cannot be used to build an indexable pack.");
 
+#ifdef THREADED_DELTA_SEARCH
+	if (!delta_search_threads)	/* --threads=0 means autodetect */
+		delta_search_threads = online_cpus();
+#endif
+
 	prepare_packed_git();
 
 	if (progress)

new file mode 100644
index 0000000..cce4b77
@@ -0,0 +1,6 @@
+#ifndef THREAD_COMPAT_H
+#define THREAD_COMPAT_H
+
+extern int online_cpus(void);
+
+#endif /* THREAD_COMPAT_H */

1.5.4.2.199.g0941

The changes are small enough that maintaining original authorship is probably the right thing to do. For anything larger it would probably have made sense to send something on top of it. For a rewrite or when implementing a feature that was thought up by someone else, mentioning in the message who the original idea was from is enough.

Andreas Ericsson                   [email protected]
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231