packfile: drop release_pack_memory()

Long ago, in 97bfeb34df (Release pack windows before reporting out of
memory., 2006-12-24), we taught xmalloc() and friends to try unmapping
pack windows when malloc() failed. It's unlikely that his helps a lot in
practice, and it has some downsides. First, the downsides:

  1. It makes xmalloc() not thread-safe. We've worked around this in
     pack-objects.c, which installs its own locking version of the
     try_to_free_routine(). But other threaded code doesn't.

  2. It makes the system as a whole harder to reason about. Functions
     which allocate heap memory under the hood may have farther-reaching
     effects than expected.

That might be worth the tradeoff if there's a benefit. But in practice,
it seems unlikely. We're generally dealing with mmap'd files, so the OS
is going to do a much better job at responding to memory pressure by
dropping individual pages (the exception is systems with NO_MMAP, but
even there the OS can probably respond just as well with swapping).

So the only thing we're really freeing is address space. On 64-bit
systems, we have plenty of that to go around. On 32-bit systems, it
could possibly help. But around the same time we made two other changes:
77ccc5bbd1 (Introduce new config option for mmap limit., 2006-12-23) and
60bb8b1453 (Fully activate the sliding window pack access., 2006-12-23).
Together that means that a 32-bit system should have no more than 256MB
total of packed-git mmaps at one time, split between a few 32MB windows.
It's unlikely we have any address space problems since then, but we
don't have any data since the features were all added at the same time.

Likewise, xmmap() will try to free memory. At first glance, it seems
like we'd need this (when we try to mmap a new window, we might need to
close an old one to save address space on a 32-bit system). But we're
saved again by core.packedGitLimit: if we're going to exceed our 256MB
limit, we'll close an existing window before we even call mmap().

So it seems unlikely that this feature is actually doing anything
useful. And while we don't have reports of it harming anything (probably
because it rarely if ever kicks in), it would be nice to simplify the
system overall. This patch drops the whole try_to_free system from
xmalloc(), as well as the manual pack memory release in xmmap().

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 76ce9069467e06545652a70040b79316f0a87fb9..93e92876aa76378883471414c2a044edfa77eca4 100644 (file)
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2342,15 +2342,6 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
        free(array);
 }
 
-static void try_to_free_from_threads(size_t size)
-{
-       packing_data_lock(&to_pack);
-       release_pack_memory(size);
-       packing_data_unlock(&to_pack);
-}
-
-static try_to_free_t old_try_to_free_routine;
-
 /*
  * The main object list is split into smaller lists, each is handed to
  * one worker.
@@ -2391,12 +2382,10 @@ static void init_threaded_search(void)
        pthread_mutex_init(&cache_mutex, NULL);
        pthread_mutex_init(&progress_mutex, NULL);
        pthread_cond_init(&progress_cond, NULL);
-       old_try_to_free_routine = set_try_to_free_routine(try_to_free_from_threads);
 }
 
 static void cleanup_threaded_search(void)
 {
-       set_try_to_free_routine(old_try_to_free_routine);
        pthread_cond_destroy(&progress_cond);
        pthread_mutex_destroy(&cache_mutex);
        pthread_mutex_destroy(&progress_mutex);

diff --git a/git-compat-util.h b/git-compat-util.h
index 83be89de0aac7c96799635e3b2858a465f440acf..f0d13e4e28470a544516a52146aae80c079467de 100644 (file)
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -818,9 +818,6 @@ const char *inet_ntop(int af, const void *src, char *dst, size_t size);
 int git_atexit(void (*handler)(void));
 #endif
 
-typedef void (*try_to_free_t)(size_t);
-try_to_free_t set_try_to_free_routine(try_to_free_t);
-
 static inline size_t st_add(size_t a, size_t b)
 {
        if (unsigned_add_overflows(a, b))

diff --git a/packfile.c b/packfile.c
index fc43a6c52c75a32548c20bbc4a5aa7d0cc3ddd0d..d98ac2287683f971a448b931262ef3d459e145d4 100644 (file)
--- a/packfile.c
+++ b/packfile.c
@@ -287,13 +287,6 @@ static int unuse_one_window(struct packed_git *current)
        return 0;
 }
 
-void release_pack_memory(size_t need)
-{
-       size_t cur = pack_mapped;
-       while (need >= (cur - pack_mapped) && unuse_one_window(NULL))
-               ; /* nothing */
-}
-
 void close_pack_windows(struct packed_git *p)
 {
        while (p->windows) {
@@ -710,23 +703,12 @@ void unuse_pack(struct pack_window **w_cursor)
        }
 }
 
-static void try_to_free_pack_memory(size_t size)
-{
-       release_pack_memory(size);
-}
-
 struct packed_git *add_packed_git(const char *path, size_t path_len, int local)
 {
-       static int have_set_try_to_free_routine;
        struct stat st;
        size_t alloc;
        struct packed_git *p;
 
-       if (!have_set_try_to_free_routine) {
-               have_set_try_to_free_routine = 1;
-               set_try_to_free_routine(try_to_free_pack_memory);
-       }
-
        /*
         * Make sure a corresponding .pack file exists and that
         * the index looks sane.

diff --git a/sha1-file.c b/sha1-file.c
index 487ea35d2d398434d2a907188d8f7ede17721fe3..4895408e1ed309241a044c019e11f3addeb722a6 100644 (file)
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -952,12 +952,8 @@ void *xmmap_gently(void *start, size_t length,
 
        mmap_limit_check(length);
        ret = mmap(start, length, prot, flags, fd, offset);
-       if (ret == MAP_FAILED) {
-               if (!length)
-                       return NULL;
-               release_pack_memory(length);
-               ret = mmap(start, length, prot, flags, fd, offset);
-       }
+       if (ret == MAP_FAILED && !length)
+               ret = NULL;
        return ret;
 }
 

diff --git a/trace.c b/trace.c
index fa4a2e7120e405f3cf2d12422802b785f9e37fad..b3ef0e627f8cec18433c17ffb080575894882758 100644 (file)
--- a/trace.c
+++ b/trace.c
@@ -88,8 +88,6 @@ static int prepare_trace_line(const char *file, int line,
        if (!trace_want(key))
                return 0;
 
-       set_try_to_free_routine(NULL);  /* is never reset */
-
        /* unit tests may want to disable additional trace output */
        if (trace_want(&trace_bare))
                return 1;

diff --git a/wrapper.c b/wrapper.c
index 1e45ab7b92749b653484f522701f5d86521b03c7..c55d7722d7be5a5933f2b662f685e6536357380e 100644 (file)
--- a/wrapper.c
+++ b/wrapper.c
@@ -4,12 +4,6 @@
 #include "cache.h"
 #include "config.h"
 
-static void do_nothing(size_t size)
-{
-}
-
-static void (*try_to_free_routine)(size_t size) = do_nothing;
-
 static int memory_limit_check(size_t size, int gentle)
 {
        static size_t limit = 0;
@@ -30,24 +24,11 @@ static int memory_limit_check(size_t size, int gentle)
        return 0;
 }
 
-try_to_free_t set_try_to_free_routine(try_to_free_t routine)
-{
-       try_to_free_t old = try_to_free_routine;
-       if (!routine)
-               routine = do_nothing;
-       try_to_free_routine = routine;
-       return old;
-}
-
 char *xstrdup(const char *str)
 {
        char *ret = strdup(str);
-       if (!ret) {
-               try_to_free_routine(strlen(str) + 1);
-               ret = strdup(str);
-               if (!ret)
-                       die("Out of memory, strdup failed");
-       }
+       if (!ret)
+               die("Out of memory, strdup failed");
        return ret;
 }
 
@@ -61,19 +42,13 @@ static void *do_xmalloc(size_t size, int gentle)
        if (!ret && !size)
                ret = malloc(1);
        if (!ret) {
-               try_to_free_routine(size);
-               ret = malloc(size);
-               if (!ret && !size)
-                       ret = malloc(1);
-               if (!ret) {
-                       if (!gentle)
-                               die("Out of memory, malloc failed (tried to allocate %lu bytes)",
-                                   (unsigned long)size);
-                       else {
-                               error("Out of memory, malloc failed (tried to allocate %lu bytes)",
-                                     (unsigned long)size);
-                               return NULL;
-                       }
+               if (!gentle)
+                       die("Out of memory, malloc failed (tried to allocate %lu bytes)",
+                           (unsigned long)size);
+               else {
+                       error("Out of memory, malloc failed (tried to allocate %lu bytes)",
+                             (unsigned long)size);
+                       return NULL;
                }
        }
 #ifdef XMALLOC_POISON
@@ -138,14 +113,8 @@ void *xrealloc(void *ptr, size_t size)
        ret = realloc(ptr, size);
        if (!ret && !size)
                ret = realloc(ptr, 1);
-       if (!ret) {
-               try_to_free_routine(size);
-               ret = realloc(ptr, size);
-               if (!ret && !size)
-                       ret = realloc(ptr, 1);
-               if (!ret)
-                       die("Out of memory, realloc failed");
-       }
+       if (!ret)
+               die("Out of memory, realloc failed");
        return ret;
 }
 
@@ -160,14 +129,8 @@ void *xcalloc(size_t nmemb, size_t size)
        ret = calloc(nmemb, size);
        if (!ret && (!nmemb || !size))
                ret = calloc(1, 1);
-       if (!ret) {
-               try_to_free_routine(nmemb * size);
-               ret = calloc(nmemb, size);
-               if (!ret && (!nmemb || !size))
-                       ret = calloc(1, 1);
-               if (!ret)
-                       die("Out of memory, calloc failed");
-       }
+       if (!ret)
+               die("Out of memory, calloc failed");
        return ret;
 }