Re: [PATCH] macOS: queue for munmap operations

From: Torsten Bögershausen <tboegi@web.de>
Date: Oct 21, 2025, 06:26 UTC
Message-ID: <20251021062621.GA370@tb-raspi4>
In-Reply-To: <pull.1993.git.1760999702581.gitgitgadget@gmail.com>

Some comments inline, all up to improvements

On Mon, Oct 20, 2025 at 10:35:02PM +0000, Koji Nakamaru via GitGitGadget wrote:

Show 10 quoted lines

> From: Koji Nakamaru <koji.nakamaru@gree.net>
> 
> Executing many mmap/munmap calls alternately can cause a huge load on
> macOS. In order to reduce it, we should temporarily store munmap
> operations in a queue and process them all at once when the queue is
> filled. When the program terminates, we can discard any remaining munmap
> operations as corresponding mmaped regions are automatically reclaimed.
> 
> Add a queue for munmap operations to perform them all at once.
>

Suggestions for rewording: In order to reduce the peak load store all munmap operations in a queue. Process them all at once (and more efficient) when the queue is filled. The queue may be ignored when the git process terminates. The operating system will do all munmap() when the process exits.

Show 75 quoted lines

> Here are some example timings. On the Linux kernel repository that
> requires about 1700 mmap/munmap calls:
> 
>   time git ls-tree -r -l --full-tree 211ddde > /dev/null
> 
>   Before:
>         real    0m2.083s
>         user    0m0.201s
>         sys     0m1.873s
> 
>   After:
>         real    0m0.243s
>         user    0m0.179s
>         sys     0m0.052s
> 
> On a private repository that requires about 943000 mmap/munmap calls:
> 
>   time git ls-tree -r -l --full-tree xxxxxxx > /dev/null
> 
>   Before:
>         real    27m15.138s
>         user    0m5.084s
>         sys     27m9.636s
> 
>   After:
>         real    0m24.209s
>         user    0m3.055s
>         sys     0m21.123s
> 
> Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
> ---
>     macOS: queue for munmap operations
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1993%2FKojiNakamaru%2Ffeature%2Fosx-queued-munmap-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1993/KojiNakamaru/feature/osx-queued-munmap-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1993
> 
>  Makefile                            |  1 +
>  compat/osxmmap.c                    | 49 +++++++++++++++++++++++++++++
>  compat/posix.h                      |  7 +++++
>  contrib/buildsystems/CMakeLists.txt |  4 +++
>  meson.build                         |  2 ++
>  5 files changed, 63 insertions(+)
>  create mode 100644 compat/osxmmap.c
> 
> diff --git a/Makefile b/Makefile
> index f79c905bdc..058bc83753 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1654,6 +1654,7 @@ ifeq ($(uname_S),Darwin)
>  		COMPAT_CFLAGS += -DAPPLE_COMMON_CRYPTO
>          endif
>  	PTHREAD_LIBS =
> +	COMPAT_OBJS += compat/osxmmap.o
>  endif
>  
>  ifdef NO_LIBGEN_H
> diff --git a/compat/osxmmap.c b/compat/osxmmap.c
> new file mode 100644
> index 0000000000..5f9cf633ca
> --- /dev/null
> +++ b/compat/osxmmap.c
> @@ -0,0 +1,49 @@
> +#include <pthread.h>
> +#include "../git-compat-util.h"
> +/* We need original mmap/munmap here. */
> +#undef mmap
> +#undef munmap
> +
> +/*
> + * OSX doesn't have any specific setting like Linux's vm.max_map_count,
> + * so COUNT_MAX can be any large number. We here set it to the default
> + * value of Linux's vm.max_map_count.
> + */
> +#define COUNT_MAX (65530)

Why the parantheses ? And would a less generic name be better, like MAX_UNMAP_COUNT

Show 12 quoted lines

> +
> +struct munmap_queue {
> +	void *start;
> +	size_t length;
> +};
> +
> +void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset)
> +{
> +	/*
> +	 * We can simply discard munmap operations in the queue by
> +	 * restricting mmap arguments.
> +	 */

Should I read this as The munmap queue is only ment to defere read-only mappings. And that is what Git does at the moment.

Show 34 quoted lines

> +	if (start != NULL || flags != MAP_PRIVATE || prot != PROT_READ)
> +		die("invalid usage of mmap");
> +	return mmap(start, length, prot, flags, fd, offset);
> +}
> +
> +int git_munmap(void *start, size_t length)
> +{
> +	static pthread_mutex_t mutex;
> +	static struct munmap_queue *queue;
> +	static int count;
> +	int i;
> +
> +	pthread_mutex_lock(&mutex);
> +	if (!queue)
> +		queue = xmalloc(COUNT_MAX * sizeof(struct munmap_queue));
> +	queue[count].start = start;
> +	queue[count].length = length;
> +	if (++count == COUNT_MAX) {
> +		for (i = 0; i < COUNT_MAX; i++)
> +			munmap(queue[i].start, queue[i].length);
> +		count = 0;
> +	}
> +	pthread_mutex_unlock(&mutex);
> +	return 0;
> +}
> diff --git a/compat/posix.h b/compat/posix.h
> index 067a00f33b..3fa1218289 100644
> --- a/compat/posix.h
> +++ b/compat/posix.h
> @@ -278,6 +278,13 @@ int git_munmap(void *start, size_t length);
>  
>  #include <sys/mman.h>
>  
> +#if defined(__APPLE__)

I think it would be better to have a global Makefile knob here. Which a) allows to take out this patch once the MacOs kernel is improved b) allows to hook in this code for other OS Something like DEFER_MUNMAPS - better suggestions welcome

Show 42 quoted lines

> +#define mmap git_mmap
> +#define munmap git_munmap
> +void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
> +int git_munmap(void *start, size_t length);
> +#endif
> +
>  #endif /* NO_MMAP || USE_WIN32_MMAP */
>  
>  #ifndef MAP_FAILED
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index edb0fc04ad..5c08f2fe5c 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -271,6 +271,10 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
>  		compat/strdup.c)
>  	set(NO_UNIX_SOCKETS 1)
>  
> +elseif(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> +	list(APPEND compat_SOURCES
> +		compat/osxmmap.c)
> +
>  elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux")
>  	add_compile_definitions(PROCFS_EXECUTABLE_PATH="/proc/self/exe" HAVE_DEV_TTY )
>  	list(APPEND compat_SOURCES unix-socket.c unix-stream-server.c compat/linux/procinfo.c)
> diff --git a/meson.build b/meson.build
> index cee9424475..b9b6e731b1 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1275,6 +1275,8 @@ elif host_machine.system() == 'windows'
>    else
>      libgit_sources += 'compat/mingw.c'
>    endif
> +elif host_machine.system() == 'darwin'
> +  libgit_sources += 'compat/osxmmap.c'
>  endif
>  
>  if host_machine.system() == 'linux'
> 
> base-commit: 4253630c6f07a4bdcc9aa62a50e26a4d466219d1
> -- 
> gitgitgadget
>