[OPTIM] GCC4's builtin_expect() is suboptimal

GCC4 is stupid (unbelievable news!).

When some code uses __builtin_expect(x != 0, 1), it really performs
the check of x != 0 then tests that the result is not zero! This is
a double check when only one was expected. Some performance drops
of 10% in the HTTP parser code have been observed due to this bug.

GCC 3.4 is fine though.

A solution consists in expecting that the tested value is 1. In
this case, it emits the correct code, but it's still not optimal
it seems. Finally the best solution is to ignore likely() and to
pray for the compiler to emit correct code. However, we still have
to fix unlikely() to remove the test there too, and to fix all
code which passed pointers overthere to pass integers instead.

diff --git a/include/common/ebtree.h b/include/common/ebtree.h
index 854666ab590490743a3a15e00e6ffc5970c7fba4..1a5ce86501ca18fc3398fefa29dc120f343f3200 100644 (file)
--- a/include/common/ebtree.h
+++ b/include/common/ebtree.h
@@ -1,6 +1,6 @@
 /*
  * Elastic Binary Trees - generic macros and structures.
- * (C) 2002-2007 - Willy Tarreau <w@1wt.eu>
+ * (C) 2002-2008 - Willy Tarreau <w@1wt.eu>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -307,13 +307,23 @@ static inline int fls64(unsigned long long x)
  * this in inline functions, because the code reordering it causes very often
  * has a negative impact on the calling functions.
  */
-#if __GNUC__ < 3 && !defined(__builtin_expect)
+#if !defined(likely)
+#if __GNUC__ < 3
 #define __builtin_expect(x,y) (x)
-#endif
-
-#ifndef likely
+#define likely(x) (x)
+#define unlikely(x) (x)
+#elif __GNUC__ < 4
+/* gcc 3.x does the best job at this */
 #define likely(x) (__builtin_expect((x) != 0, 1))
 #define unlikely(x) (__builtin_expect((x) != 0, 0))
+#else
+/* GCC 4.x is stupid, it performs the comparison then compares it to 1,
+ * so we cheat in a dirty way to prevent it from doing this. This will
+ * only work with ints and booleans though.
+ */
+#define likely(x) (x)
+#define unlikely(x) (__builtin_expect((x), 0))
+#endif
 #endif
 
 /* Support passing function parameters in registers. For this, the

diff --git a/include/common/standard.h b/include/common/standard.h
index 248bbe91b99c17e4c72717ae2bdfaf2671301a1e..162a9ebc09b2494bba5516cc89b6fab9d4b5e0cb 100644 (file)
--- a/include/common/standard.h
+++ b/include/common/standard.h
@@ -2,7 +2,7 @@
   include/common/standard.h
   This files contains some general purpose functions and macros.
 
-  Copyright (C) 2000-2007 Willy Tarreau - w@1wt.eu
+  Copyright (C) 2000-2008 Willy Tarreau - w@1wt.eu
   
   This library is free software; you can redistribute it and/or
   modify it under the terms of the GNU Lesser General Public
@@ -46,13 +46,24 @@
  * helps the compiler produce the most compact critical paths, which is
  * generally better for the cache and to reduce the number of jumps.
  */
+#if !defined(likely)
 #if __GNUC__ < 3
 #define __builtin_expect(x,y) (x)
-#endif
-
+#define likely(x) (x)
+#define unlikely(x) (x)
+#elif __GNUC__ < 4
+/* gcc 3.x does the best job at this */
 #define likely(x) (__builtin_expect((x) != 0, 1))
 #define unlikely(x) (__builtin_expect((x) != 0, 0))
-
+#else
+/* GCC 4.x is stupid, it performs the comparison then compares it to 1,
+ * so we cheat in a dirty way to prevent it from doing this. This will
+ * only work with ints and booleans though.
+ */
+#define likely(x) (x)
+#define unlikely(x) (__builtin_expect((x), 0))
+#endif
+#endif
 
 /*
  * copies at most <size-1> chars from <src> to <dst>. Last char is always

diff --git a/include/proto/task.h b/include/proto/task.h
index 6bee1b2f19cb37a7cb45a9e5629ae82206f7a5fc..49e3a94b6b35121fa78938f62dbf81c20f4e66fe 100644 (file)
--- a/include/proto/task.h
+++ b/include/proto/task.h
@@ -55,7 +55,7 @@ static inline struct task *__task_wakeup(struct task *t)
        DLIST_ADD(run_queue, &t->qlist);
        t->state = TASK_RUNNING;
 
-       if (likely(t->wq)) {
+       if (likely(t->wq != NULL)) {
                tree_delete(t->wq);
                t->wq = NULL;
        }

diff --git a/src/backend.c b/src/backend.c
index 037885074deb8adb3802b959c0d0786b62bf7ba4..4c5eec3dd95eac6097cecc8a0107c5c35a77f259 100644 (file)
--- a/src/backend.c
+++ b/src/backend.c
@@ -786,7 +786,7 @@ static struct server *fwrr_get_next_server(struct proxy *p)
        fwrr_queue_srv(srv);
 
  requeue_servers:
-       if (unlikely(full)) {
+       if (unlikely(full != NULL)) {
                if (switched) {
                        /* the tree has switched, requeue all extracted servers
                         * into "init", because their place was lost, and only

diff --git a/src/ev_epoll.c b/src/ev_epoll.c
index a0c48b108118a8def0440f277aa08a761b5c69fc..326d5a42ed0f2691ec93134b032bdd7e2926b4fc 100644 (file)
--- a/src/ev_epoll.c
+++ b/src/ev_epoll.c
@@ -142,7 +142,7 @@ REGPRM2 static void alloc_chg_list(const int fd, int old_evt)
 {
        struct fd_chg *ptr;
 
-       if (unlikely(chg_ptr[fd]))
+       if (unlikely(chg_ptr[fd] != NULL))
                return;
 
 #if LIMIT_NUMBER_OF_CHANGES

diff --git a/src/task.c b/src/task.c
index 7f6e0e7a2359c4b26821c530e4e22fab5b35fe30..c60961f3ac26fb28d1315d6720d86f121cfa239b 100644 (file)
--- a/src/task.c
+++ b/src/task.c
@@ -68,7 +68,7 @@ struct task *task_queue(struct task *task)
                task->qlist.p = NULL;
        }
 
-       if (unlikely(task->wq)) {
+       if (unlikely(task->wq != NULL)) {
                tree_delete(task->wq);
                task->wq = NULL;
        }