Re: [DEMO][PATCH v2 6/5] compat: add a qsort_s() implementation based on GNU's qsort_r(1)
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 23, 2017, 19:07 UTC
- Message-ID
- <xmqq60l5sihz.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <9f8b564d-ec9f-abc9-77f6-aa84c6e78b7a@web.de>
René Scharfe <l.s.r@web.de> writes:
Show 13 quoted lines
> Implement qsort_s() as a wrapper to the GNU version of qsort_r(1) and > use it on Linux. Performance increases slightly: > > Test HEAD^ HEAD > -------------------------------------------------------------------- > 0071.2: sort(1) 0.10(0.20+0.02) 0.10(0.21+0.01) +0.0% > 0071.3: string_list_sort() 0.17(0.15+0.01) 0.16(0.15+0.01) -5.9% > > Additionally the unstripped size of compat/qsort_s.o falls from 24576 > to 16544 bytes in my build. > > IMHO these savings aren't worth the increased complexity of having to > support two implementations.
I do worry about having to support more implementations in the future that have different function signature for the comparison callbacks, which will make things ugly, but this addition alone doesn't look too bad to me.
Thanks.
Show 44 quoted lines
> diff --git a/compat/qsort_s.c b/compat/qsort_s.c
> index 52d1f0a73d..763ee1faae 100644
> --- a/compat/qsort_s.c
> +++ b/compat/qsort_s.c
> @@ -1,5 +1,21 @@
> #include "../git-compat-util.h"
>
> +#if defined HAVE_GNU_QSORT_R
> +
> +int git_qsort_s(void *b, size_t n, size_t s,
> + int (*cmp)(const void *, const void *, void *), void *ctx)
> +{
> + if (!n)
> + return 0;
> + if (!b || !cmp)
> + return -1;
> +
> + qsort_r(b, n, s, cmp, ctx);
> + return 0;
> +}
> +
> +#else
> +
> /*
> * A merge sort implementation, simplified from the qsort implementation
> * by Mike Haertel, which is a part of the GNU C Library.
> @@ -67,3 +83,5 @@ int git_qsort_s(void *b, size_t n, size_t s,
> }
> return 0;
> }
> +
> +#endif
> diff --git a/config.mak.uname b/config.mak.uname
> index 447f36ac2e..a1858f54ff 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -37,6 +37,7 @@ ifeq ($(uname_S),Linux)
> NEEDS_LIBRT = YesPlease
> HAVE_GETDELIM = YesPlease
> SANE_TEXT_GREP=-a
> + HAVE_GNU_QSORT_R = YesPlease
> endif
> ifeq ($(uname_S),GNU/kFreeBSD)
> HAVE_ALLOCA_H = YesPlease