From e976b56dda6ae3d7d81bd114b61750e97cc918d3 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 15 Apr 2013 14:27:53 -0700 Subject: [PATCH] Add git__compare_and_swap and use it This removes the lock from the repository object and changes the internals to use the new atomic git__compare_and_swap to update the _odb, _config, _index, and _refdb variables in a threadsafe manner. --- src/global.h | 8 --- src/repository.c | 132 +++++++++++++++++++-------------------------- src/repository.h | 1 - src/thread-utils.h | 41 ++++++++++++++ src/util.h | 19 ------- 5 files changed, 95 insertions(+), 106 deletions(-) diff --git a/src/global.h b/src/global.h index f0ad1df29..badbc0883 100644 --- a/src/global.h +++ b/src/global.h @@ -10,14 +10,6 @@ #include "mwindow.h" #include "hash.h" -#if defined(GIT_THREADS) && defined(_MSC_VER) -# define GIT_MEMORY_BARRIER MemoryBarrier() -#elif defined(GIT_THREADS) -# define GIT_MEMORY_BARRIER __sync_synchronize() -#else -# define GIT_MEMORY_BARRIER /* noop */ -#endif - typedef struct { git_error *last_error; git_error error_t; diff --git a/src/repository.c b/src/repository.c index 8744b91e6..59479dc92 100644 --- a/src/repository.c +++ b/src/repository.c @@ -32,43 +32,57 @@ #define GIT_TEMPLATE_DIR "/usr/share/git-core/templates" -#define repo_swap_ptr(repo,item_ptr,new_value) \ - git__swap(&repo->lock, (void **)item_ptr, new_value) - -static void drop_odb(git_repository *repo) +static void set_odb(git_repository *repo, git_odb *odb) { - git_odb *dropme = repo_swap_ptr(repo, &repo->_odb, NULL); - if (dropme != NULL) { - GIT_REFCOUNT_OWN(dropme, NULL); - git_odb_free(dropme); + if (odb) { + GIT_REFCOUNT_OWN(odb, repo); + GIT_REFCOUNT_INC(odb); + } + + if ((odb = git__swap(repo->_odb, odb)) != NULL) { + GIT_REFCOUNT_OWN(odb, NULL); + git_odb_free(odb); } } -static void drop_refdb(git_repository *repo) +static void set_refdb(git_repository *repo, git_refdb *refdb) { - git_refdb *dropme = repo_swap_ptr(repo, &repo->_refdb, NULL); - if (dropme != NULL) { - GIT_REFCOUNT_OWN(dropme, NULL); - git_refdb_free(dropme); + if (refdb) { + GIT_REFCOUNT_OWN(refdb, repo); + GIT_REFCOUNT_INC(refdb); + } + + if ((refdb = git__swap(repo->_refdb, refdb)) != NULL) { + GIT_REFCOUNT_OWN(refdb, NULL); + git_refdb_free(refdb); } } -static void drop_config(git_repository *repo) +static void set_config(git_repository *repo, git_config *config) { - git_config *dropme = repo_swap_ptr(repo, &repo->_config, NULL); - if (dropme != NULL) { - GIT_REFCOUNT_OWN(dropme, NULL); - git_config_free(dropme); - git_repository__cvar_cache_clear(repo); + if (config) { + GIT_REFCOUNT_OWN(config, repo); + GIT_REFCOUNT_INC(config); } + + if ((config = git__swap(repo->_config, config)) != NULL) { + GIT_REFCOUNT_OWN(config, NULL); + git_config_free(config); + } + + git_repository__cvar_cache_clear(repo); } -static void drop_index(git_repository *repo) +static void set_index(git_repository *repo, git_index *index) { - git_index *dropme = repo_swap_ptr(repo, &repo->_index, NULL); - if (dropme != NULL) { - GIT_REFCOUNT_OWN(dropme, NULL); - git_index_free(dropme); + if (index) { + GIT_REFCOUNT_OWN(index, repo); + GIT_REFCOUNT_INC(index); + } + + if ((index = git__swap(repo->_index, index)) != NULL) { + GIT_REFCOUNT_OWN(index, NULL); + git_index_free(index); } } @@ -81,15 +95,14 @@ void git_repository_free(git_repository *repo) git_attr_cache_flush(repo); git_submodule_config_free(repo); - drop_config(repo); - drop_index(repo); - drop_odb(repo); - drop_refdb(repo); + set_config(repo, NULL); + set_index(repo, NULL); + set_odb(repo, NULL); + set_refdb(repo, NULL); git__free(repo->path_repository); git__free(repo->workdir); - git_mutex_free(&repo->lock); git__free(repo); } @@ -122,8 +135,6 @@ static git_repository *repository_alloc(void) memset(repo, 0x0, sizeof(git_repository)); - git_mutex_init(&repo->lock); - if (git_cache_init(&repo->objects) < 0) { git__free(repo); return NULL; @@ -581,7 +592,7 @@ int git_repository_config__weakptr(git_config **out, git_repository *repo) if (!error) { GIT_REFCOUNT_OWN(config, repo); - config = repo_swap_ptr(repo, &repo->_config, config); + config = git__compare_and_swap(&repo->_config, NULL, config); if (config != NULL) { GIT_REFCOUNT_OWN(config, NULL); git_config_free(config); @@ -609,17 +620,7 @@ int git_repository_config(git_config **out, git_repository *repo) void git_repository_set_config(git_repository *repo, git_config *config) { assert(repo && config); - - GIT_REFCOUNT_OWN(config, repo); - GIT_REFCOUNT_INC(config); - - config = repo_swap_ptr(repo, &repo->_config, config); - if (config != NULL) { - GIT_REFCOUNT_OWN(config, NULL); - git_config_free(config); - } - - git_repository__cvar_cache_clear(repo); + set_config(repo, config); } int git_repository_odb__weakptr(git_odb **out, git_repository *repo) @@ -638,7 +639,7 @@ int git_repository_odb__weakptr(git_odb **out, git_repository *repo) if (!error) { GIT_REFCOUNT_OWN(odb, repo); - odb = repo_swap_ptr(repo, &repo->_odb, odb); + odb = git__compare_and_swap(&repo->_odb, NULL, odb); if (odb != NULL) { GIT_REFCOUNT_OWN(odb, NULL); git_odb_free(odb); @@ -664,15 +665,7 @@ int git_repository_odb(git_odb **out, git_repository *repo) void git_repository_set_odb(git_repository *repo, git_odb *odb) { assert(repo && odb); - - GIT_REFCOUNT_OWN(odb, repo); - GIT_REFCOUNT_INC(odb); - - odb = repo_swap_ptr(repo, &repo->_odb, odb); - if (odb != NULL) { - GIT_REFCOUNT_OWN(odb, NULL); - git_odb_free(odb); - } + set_odb(repo, odb); } int git_repository_refdb__weakptr(git_refdb **out, git_repository *repo) @@ -688,7 +681,7 @@ int git_repository_refdb__weakptr(git_refdb **out, git_repository *repo) if (!error) { GIT_REFCOUNT_OWN(refdb, repo); - refdb = repo_swap_ptr(repo, &repo->_refdb, refdb); + refdb = git__compare_and_swap(&repo->_refdb, NULL, refdb); if (refdb != NULL) { GIT_REFCOUNT_OWN(refdb, NULL); git_refdb_free(refdb); @@ -712,15 +705,7 @@ int git_repository_refdb(git_refdb **out, git_repository *repo) void git_repository_set_refdb(git_repository *repo, git_refdb *refdb) { assert(repo && refdb); - - GIT_REFCOUNT_OWN(refdb, repo); - GIT_REFCOUNT_INC(refdb); - - refdb = repo_swap_ptr(repo, &repo->_refdb, refdb); - if (refdb != NULL) { - GIT_REFCOUNT_OWN(refdb, NULL); - git_refdb_free(refdb); - } + set_refdb(repo, refdb); } int git_repository_index__weakptr(git_index **out, git_repository *repo) @@ -739,7 +724,7 @@ int git_repository_index__weakptr(git_index **out, git_repository *repo) if (!error) { GIT_REFCOUNT_OWN(index, repo); - index = repo_swap_ptr(repo, &repo->_index, index); + index = git__compare_and_swap(&repo->_index, NULL, index); if (index != NULL) { GIT_REFCOUNT_OWN(index, NULL); git_index_free(index); @@ -767,15 +752,7 @@ int git_repository_index(git_index **out, git_repository *repo) void git_repository_set_index(git_repository *repo, git_index *index) { assert(repo && index); - - GIT_REFCOUNT_OWN(index, repo); - GIT_REFCOUNT_INC(index); - - index = repo_swap_ptr(repo, &repo->_index, index); - if (index != NULL) { - GIT_REFCOUNT_OWN(index, NULL); - git_index_free(index); - } + set_index(repo, index); } static int check_repositoryformatversion(git_config *config) @@ -1465,14 +1442,13 @@ static int at_least_one_cb(const char *refname, void *payload) static int repo_contains_no_reference(git_repository *repo) { - int error; - - error = git_reference_foreach(repo, GIT_REF_LISTALL, at_least_one_cb, NULL); + int error = git_reference_foreach(repo, GIT_REF_LISTALL, at_least_one_cb, NULL); if (error == GIT_EUSER) return 0; - - return error == 0 ? 1 : error; + if (!error) + return 1; + return error; } int git_repository_is_empty(git_repository *repo) diff --git a/src/repository.h b/src/repository.h index 873498de0..cc2f8c2b8 100644 --- a/src/repository.h +++ b/src/repository.h @@ -83,7 +83,6 @@ struct git_repository { git_refdb *_refdb; git_config *_config; git_index *_index; - git_mutex lock; git_cache objects; git_attr_cache attrcache; diff --git a/src/thread-utils.h b/src/thread-utils.h index 2ca290adf..7b663182d 100644 --- a/src/thread-utils.h +++ b/src/thread-utils.h @@ -68,6 +68,21 @@ GIT_INLINE(int) git_atomic_dec(git_atomic *a) #endif } +GIT_INLINE(void *) git___compare_and_swap( + volatile void **ptr, void *oldval, void *newval) +{ + bool swapped; +#if defined(GIT_WIN32) + swapped = ((LONGLONG)oldval == InterlockedCompareExchange64( + (LONGLONG volatile *)ptr, (LONGLONG)newval, (LONGLONG)oldval)); +#elif defined(__GNUC__) + swapped = (__sync_val_compare_and_swap(ptr, oldval, newval) == oldval); +#else +# error "Unsupported architecture for atomic operations" +#endif + return swapped ? oldval : newval; +} + #else #define git_thread unsigned int @@ -101,8 +116,34 @@ GIT_INLINE(int) git_atomic_dec(git_atomic *a) return --a->val; } +GIT_INLINE(void *) git___compare_and_swap( + volatile void **ptr, void *oldval, void *newval) +{ + if (*ptr == oldval) + *ptr = newval; + else + oldval = newval; + return oldval; +} + #endif +/* Atomically replace oldval with newval + * @return oldval if it was replaced or newval if it was not + */ +#define git__compare_and_swap(P,O,N) \ + git___compare_and_swap((volatile void **)P, O, N) + +#define git__swap(ptr, val) git__compare_and_swap(&ptr, ptr, val) + extern int git_online_cpus(void); +#if defined(GIT_THREADS) && defined(GIT_WIN32) +# define GIT_MEMORY_BARRIER MemoryBarrier() +#elif defined(GIT_THREADS) +# define GIT_MEMORY_BARRIER __sync_synchronize() +#else +# define GIT_MEMORY_BARRIER /* noop */ +#endif + #endif /* INCLUDE_thread_utils_h__ */ diff --git a/src/util.h b/src/util.h index a2233a7e8..82435aee8 100644 --- a/src/util.h +++ b/src/util.h @@ -313,23 +313,4 @@ int git__date_parse(git_time_t *out, const char *date); */ extern size_t git__unescape(char *str); -/* - * Swap a pointer with thread safety, returning old value. - */ -GIT_INLINE(void *) git__swap(git_mutex *lock, void **ptr_ptr, void *new_ptr) -{ - void *old_ptr; - - if (*ptr_ptr == new_ptr) - return NULL; - if (git_mutex_lock(lock) < 0) - return new_ptr; - - old_ptr = *ptr_ptr; - *ptr_ptr = new_ptr; - - git_mutex_unlock(lock); - return old_ptr; -} - #endif /* INCLUDE_util_h__ */