Review feedback

* Change the default of the file limit to 0 (unlimited).
* Changed the heuristic to close files to be the file that contains the
  least-recently-used window such that the window is the
  most-recently-used in the file, and the file does not have in-use
  windows.
* Parameterized the filelimit test to check for a limit of 1 and 100
  open windows.

include/git2/common.h

@@ -240,7 +240,7 @@ typedef enum {
  *	* opts(GIT_OPT_SET_MWINDOW_FILE_LIMIT, size_t):
  *
  *		> Set the maximum number of files that can be mapped at any time
- *		> by the library
+ *		> by the library. The default (0) is unlimited.
  *
  *	* opts(GIT_OPT_GET_SEARCH_PATH, int level, git_buf *buf)
  *

src/mwindow.c

@@ -22,15 +22,15 @@
 #define DEFAULT_MAPPED_LIMIT \
 	((1024 * 1024) * (sizeof(void*) >= 8 ? 8192ULL : 256UL))
 
-/* default ulimit -n on macOS is just 256 */
-#define DEFAULT_FILE_LIMIT 128
+/* default is unlimited */
+#define DEFAULT_FILE_LIMIT 0
 
 size_t git_mwindow__window_size = DEFAULT_WINDOW_SIZE;
 size_t git_mwindow__mapped_limit = DEFAULT_MAPPED_LIMIT;
 size_t git_mwindow__file_limit = DEFAULT_FILE_LIMIT;
 
 /* Whenever you want to read or modify this, grab git__mwindow_mutex */
-static git_mwindow_ctl mem_ctl;
+git_mwindow_ctl git_mwindow__mem_ctl;
 
 /* Global list of mwindow files, to open packs once across repos */
 git_strmap *git__pack_cache = NULL;
@@ -136,7 +136,7 @@ void git_mwindow_free_all(git_mwindow_file *mwf)
  */
 void git_mwindow_free_all_locked(git_mwindow_file *mwf)
 {
-	git_mwindow_ctl *ctl = &mem_ctl;
+	git_mwindow_ctl *ctl = &git_mwindow__mem_ctl;
 	size_t i;
 
 	/*
@@ -178,94 +178,131 @@ int git_mwindow_contains(git_mwindow *win, off64_t offset)
 		&& offset <= (off64_t)(win_off + win->window_map.len);
 }
 
+#define GIT_MWINDOW__LRU -1
+#define GIT_MWINDOW__MRU 1
+
 /*
- * Find the least-recently-used window in a file
+ * Find the least- or most-recently-used window in a file that is not currently
+ * being used. The 'only_unused' flag controls whether the caller requires the
+ * file to only have unused windows.
+ *
+ * Returns whether such a window was found in the file.
  */
-static void git_mwindow_scan_lru(
-	git_mwindow_file *mwf,
-	git_mwindow **lru_w,
-	git_mwindow **lru_l)
+static bool git_mwindow_scan_recently_used(
+		git_mwindow_file *mwf,
+		git_mwindow **out_window,
+		git_mwindow **out_last,
+		bool only_unused,
+		int comparison_sign)
 {
-	git_mwindow *w, *w_l;
-
-	for (w_l = NULL, w = mwf->windows; w; w = w->next) {
-		if (!w->inuse_cnt) {
-			/*
-			 * If the current one is more recent than the last one,
-			 * store it in the output parameter. If lru_w is NULL,
-			 * it's the first loop, so store it as well.
-			 */
-			if (!*lru_w || w->last_used < (*lru_w)->last_used) {
-				*lru_w = w;
-				*lru_l = w_l;
-			}
+	git_mwindow *w, *w_last;
+	git_mwindow *lru_window = NULL, *lru_last = NULL;
+
+	assert(mwf);
+	assert(out_window);
+
+	lru_window = *out_window;
+	if (out_last)
+		lru_last = *out_last;
+
+	for (w_last = NULL, w = mwf->windows; w; w_last = w, w = w->next) {
+		if (w->inuse_cnt) {
+			if (only_unused)
+				return false;
+			/* This window is currently being used. Skip it. */
+			continue;
+		}
+
+		/*
+		 * If the current one is more (or less) recent than the last one,
+		 * store it in the output parameter. If lru_window is NULL,
+		 * it's the first loop, so store it as well.
+		 */
+		if (!lru_window ||
+				(comparison_sign == GIT_MWINDOW__LRU && lru_window->last_used > w->last_used) ||
+				(comparison_sign == GIT_MWINDOW__MRU && lru_window->last_used < w->last_used)) {
+			lru_window = w;
+			lru_last = w_last;
 		}
-		w_l = w;
 	}
+
+	if (!lru_window && !lru_last)
+		return false;
+
+	*out_window = lru_window;
+	if (out_last)
+		*out_last = lru_last;
+	return true;
 }
 
 /*
- * Close the least recently used window. Called under lock from new_window.
+ * Close the least recently used window (that is currently not being used) out
+ * of all the files. Called under lock from new_window.
  */
 static int git_mwindow_close_lru_window(void)
 {
-	git_mwindow_ctl *ctl = &mem_ctl;
+	git_mwindow_ctl *ctl = &git_mwindow__mem_ctl;
 	git_mwindow_file *cur;
 	size_t i;
-	git_mwindow *lru_w = NULL, *lru_l = NULL, **list = NULL;
+	git_mwindow *lru_window = NULL, *lru_last = NULL, **list = NULL;
 
 	git_vector_foreach(&ctl->windowfiles, i, cur) {
-		git_mwindow *last = lru_w;
-		git_mwindow_scan_lru(cur, &lru_w, &lru_l);
-		if (lru_w != last)
+		if (git_mwindow_scan_recently_used(
+				cur, &lru_window, &lru_last, false, GIT_MWINDOW__LRU)) {
 			list = &cur->windows;
+		}
 	}
 
-	if (!lru_w) {
+	if (!lru_window) {
 		git_error_set(GIT_ERROR_OS, "failed to close memory window; couldn't find LRU");
 		return -1;
 	}
 
-	ctl->mapped -= lru_w->window_map.len;
-	git_futils_mmap_free(&lru_w->window_map);
+	ctl->mapped -= lru_window->window_map.len;
+	git_futils_mmap_free(&lru_window->window_map);
 
-	if (lru_l)
-		lru_l->next = lru_w->next;
+	if (lru_last)
+		lru_last->next = lru_window->next;
 	else
-		*list = lru_w->next;
+		*list = lru_window->next;
 
-	git__free(lru_w);
+	git__free(lru_window);
 	ctl->open_windows--;
 
 	return 0;
 }
 
 /*
- * Close the file that contains the least recently used window. Called under
- * lock from new_window.
+ * Close the file that does not have any open windows AND contains the
+ * least-recently-used most-recently-used window.
+ *
+ * Called under lock from new_window.
  */
 static int git_mwindow_close_lru_file(void)
 {
-	git_mwindow_ctl *ctl = &mem_ctl;
-	git_mwindow_file *lru_f = NULL, *cur;
+	git_mwindow_ctl *ctl = &git_mwindow__mem_ctl;
+	git_mwindow_file *lru_file = NULL, *current_file = NULL;
+	git_mwindow *lru_window = NULL;
 	size_t i;
-	git_mwindow *lru_w = NULL, *lru_l = NULL;
 
-	git_vector_foreach(&ctl->windowfiles, i, cur) {
-		git_mwindow *last = lru_w;
-		git_mwindow_scan_lru(cur, &lru_w, &lru_l);
-		if (lru_w != last)
-			lru_f = cur;
+	git_vector_foreach(&ctl->windowfiles, i, current_file) {
+		git_mwindow *mru_window = NULL;
+		if (!git_mwindow_scan_recently_used(
+				current_file, &mru_window, NULL, true, GIT_MWINDOW__MRU)) {
+			continue;
+		}
+		if (!lru_window || lru_window->last_used > mru_window->last_used)
+			lru_file = current_file;
 	}
 
-	if (!lru_f) {
+	if (!lru_file) {
 		git_error_set(GIT_ERROR_OS, "failed to close memory window file; couldn't find LRU");
 		return -1;
 	}
 
-	git_mwindow_free_all_locked(lru_f);
-	p_close(lru_f->fd);
-	lru_f->fd = -1;
+	git_mwindow_free_all_locked(lru_file);
+	p_close(lru_file->fd);
+	lru_file->fd = -1;
 
 	return 0;
 }
@@ -276,7 +313,7 @@ static git_mwindow *new_window(
 	off64_t size,
 	off64_t offset)
 {
-	git_mwindow_ctl *ctl = &mem_ctl;
+	git_mwindow_ctl *ctl = &git_mwindow__mem_ctl;
 	size_t walign = git_mwindow__window_size / 2;
 	off64_t len;
 	git_mwindow *w;
@@ -342,7 +379,7 @@ unsigned char *git_mwindow_open(
 	size_t extra,
 	unsigned int *left)
 {
-	git_mwindow_ctl *ctl = &mem_ctl;
+	git_mwindow_ctl *ctl = &git_mwindow__mem_ctl;
 	git_mwindow *w = *cursor;
 
 	if (git_mutex_lock(&git__mwindow_mutex)) {
@@ -394,7 +431,7 @@ unsigned char *git_mwindow_open(
 
 int git_mwindow_file_register(git_mwindow_file *mwf)
 {
-	git_mwindow_ctl *ctl = &mem_ctl;
+	git_mwindow_ctl *ctl = &git_mwindow__mem_ctl;
 	int ret;
 
 	if (git_mutex_lock(&git__mwindow_mutex)) {
@@ -408,8 +445,10 @@ int git_mwindow_file_register(git_mwindow_file *mwf)
 		return -1;
 	}
 
-	while (git_mwindow__file_limit <= ctl->windowfiles.length &&
-			git_mwindow_close_lru_file() == 0) /* nop */;
+	if (git_mwindow__file_limit) {
+		while (git_mwindow__file_limit <= ctl->windowfiles.length &&
+				git_mwindow_close_lru_file() == 0) /* nop */;
+	}
 
 	ret = git_vector_insert(&ctl->windowfiles, mwf);
 	git_mutex_unlock(&git__mwindow_mutex);
@@ -419,7 +458,7 @@ int git_mwindow_file_register(git_mwindow_file *mwf)
 
 void git_mwindow_file_deregister(git_mwindow_file *mwf)
 {
-	git_mwindow_ctl *ctl = &mem_ctl;
+	git_mwindow_ctl *ctl = &git_mwindow__mem_ctl;
 	git_mwindow_file *cur;
 	size_t i;
 

tests/pack/filelimit.c

 #include "clar_libgit2.h"
+#include "mwindow.h"
+#include "global.h"
 #include <git2.h>
 
+extern git_mwindow_ctl git_mwindow__mem_ctl;
+size_t mwindow_file_limit = 0;
+size_t original_mwindow_file_limit = 0;
+
+void test_pack_filelimit__initialize_tiny(void)
+{
+	mwindow_file_limit = 1;
+	cl_git_pass(git_libgit2_opts(GIT_OPT_GET_MWINDOW_FILE_LIMIT, &original_mwindow_file_limit));
+	cl_git_pass(git_libgit2_opts(GIT_OPT_SET_MWINDOW_FILE_LIMIT, mwindow_file_limit));
+}
+
+void test_pack_filelimit__initialize_medium(void)
+{
+	mwindow_file_limit = 100;
+	cl_git_pass(git_libgit2_opts(GIT_OPT_GET_MWINDOW_FILE_LIMIT, &original_mwindow_file_limit));
+	cl_git_pass(git_libgit2_opts(GIT_OPT_SET_MWINDOW_FILE_LIMIT, mwindow_file_limit));
+}
+
+void test_pack_filelimit__cleanup(void)
+{
+	cl_git_pass(git_libgit2_opts(GIT_OPT_SET_MWINDOW_FILE_LIMIT, original_mwindow_file_limit));
+}
+
 void test_pack_filelimit__open_repo_with_1025_packfiles(void)
 {
+	git_mwindow_ctl *ctl = &git_mwindow__mem_ctl;
 	git_repository *repo;
 	git_revwalk *walk;
 	git_oid id;
 	int i;
+	unsigned int open_windows;
 
 	/*
 	 * This repository contains 1025 packfiles, each with one commit, one tree,
@@ -30,7 +57,16 @@ void test_pack_filelimit__open_repo_with_1025_packfiles(void)
 		++i;
 	cl_assert_equal_i(1025, i);
 
+	cl_git_pass(git_mutex_lock(&git__mwindow_mutex));
+	/*
+	 * Adding an assert while holding a lock will cause the whole process to
+	 * deadlock. Copy the value and do the assert after releasing the lock.
+	 */
+	open_windows = ctl->open_windows;
+	cl_git_pass(git_mutex_unlock(&git__mwindow_mutex));
+
+	cl_assert_equal_i(mwindow_file_limit, open_windows);
+
 	git_revwalk_free(walk);
 	git_repository_free(repo);
 }
-