Ben Reser wrote on Fri, Jul 26, 2013 at 09:56:26 -0700:
> On Fri, Jul 26, 2013 at 3:00 AM, Ivan Zhakov <i...@visualsvn.com> wrote:
> > I really like idea go back to simple svn_hash_gets/svn_hash_sets()
> > implementation and remove these premature optimizations.
>
> I've left the improved svn_hash_sets() implementation in place. I'm
> not seeing any further harm from the current solution but if we find
> something else I'd support getting rid of it entirely.
There was some discussion on IRC today which resulted in the below.
[[[
Follow-up to r1507366: svn_hash_gets: compute the length of string literal
keys (common case) at compile-time, without multiply-evaluating dynamically-
computed keys.
* build/ac-macros/svn-macros.m4
(SVN_CHECK_FOR_DUNDER_BUILTINS): New macro.
* configure.ac:
Call SVN_CHECK_FOR_DUNDER_BUILTINS() and set -D SVN_HAS_DUNDER_BUILTINS.
* subversion/include/svn_hash.h
(svn_hash_gets): Use __builtin_choose_expr() and __builtin_constant_p(),
when available.
]]]
Index: build/ac-macros/svn-macros.m4
===================================================================
--- build/ac-macros/svn-macros.m4 (revision 1508134)
+++ build/ac-macros/svn-macros.m4 (working copy)
@@ -163,3 +163,17 @@ AC_DEFUN([SVN_CHECK_FOR_ATOMIC_BUILTINS],
return 0;
}], [svn_cv_atomic_builtins=yes], [svn_cv_atomic_builtins=no],
[svn_cv_atomic_builtins=no])])
])
+
+AC_DEFUN([SVN_CHECK_FOR_DUNDER_BUILTINS],
+[
+ AC_CACHE_CHECK([whether the compiler provides dunder builtins],
[svn_cv_dunder_builtins],
+ [
+ AC_COMPILE_IFELSE([AC_LANG_SOURCE([[
+ int main(int argc)
+ {
+ return (!__builtin_choose_expr(__builtin_constant_p(argc), 1, 0)
+ && __builtin_choose_expr(__builtin_constant_p("foobar"), 1, 0))
+ ? 0 /* EXIT_SUCCESS */ : 1 /* EXIT_FAILURE */;
+ }]])], svn_cv_dunder_builtins="yes", svn_cv_dunder_builtins="no")
+ ])
+])
Index: configure.ac
===================================================================
--- configure.ac (revision 1508134)
+++ configure.ac (working copy)
@@ -167,11 +167,15 @@ if test -n "$sqlite_compat_ver" && test "$sqlite_c
fi
SVN_CHECK_FOR_ATOMIC_BUILTINS
-
if test "$svn_cv_atomic_builtins" = "yes"; then
AC_DEFINE(SVN_HAS_ATOMIC_BUILTINS, 1, [Define if compiler provides atomic
builtins])
fi
+SVN_CHECK_FOR_DUNDER_BUILTINS
+if test "$svn_cv_dunder_builtins" = "yes"; then
+ AC_DEFINE(SVN_HAS_DUNDER_BUILTINS, 1, [Define if compiler provides
__builtin_constant_p and __builtin_choose_expr])
+fi
+
dnl Set up a number of directories ---------------------
dnl Create SVN_BINDIR for proper substitution
Index: subversion/include/svn_hash.h
===================================================================
--- subversion/include/svn_hash.h (revision 1508134)
+++ subversion/include/svn_hash.h (working copy)
@@ -244,8 +244,26 @@ svn_hash_from_cstring_keys(apr_hash_t **hash,
*
* @since New in 1.8.
*/
-#define svn_hash_gets(ht, key) \
+#if SVN_HAS_DUNDER_BUILTINS
+/* We have two use-cases:
+ 1. (common) KEY is a string literal.
+ 2. (rare) KEY is the result of an expensive function call.
+ For the former, we want to evaluate the string length at compile time. (We
+ use sizeof(), but strlen() would do.) For the latter, however, we want to
+ avoid having the macro multiply-evaluate KEY. So, if our compiler is smart
+ enough (that includes at least gcc and clang), we use __builtin_* to have
+ our cake and eat it too: */
+# define svn_hash_gets(ht, key) \
+ apr_hash_get(ht, key, \
+ __builtin_choose_expr(__builtin_constant_p(key), \
+ sizeof(key)-1, \
+ APR_HASH_KEY_STRING))
+#else
+/* ... and when our compiler is anything else, we take the hit and compute the
+ length of string literals at run-time. */
+# define svn_hash_gets(ht, key) \
apr_hash_get(ht, key, APR_HASH_KEY_STRING)
+#endif
/** Shortcut for apr_hash_set() with a const char * key.
*
