[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 854666a..1a5ce86 100644
--- 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 @@
* 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 248bbe9..162a9eb 100644
--- 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 6bee1b2..49e3a94 100644
--- a/include/proto/task.h
+++ b/include/proto/task.h
@@ -55,7 +55,7 @@
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 0378850..4c5eec3 100644
--- a/src/backend.c
+++ b/src/backend.c
@@ -786,7 +786,7 @@
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 a0c48b1..326d5a4 100644
--- a/src/ev_epoll.c
+++ b/src/ev_epoll.c
@@ -142,7 +142,7 @@
{
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 7f6e0e7..c60961f 100644
--- a/src/task.c
+++ b/src/task.c
@@ -68,7 +68,7 @@
task->qlist.p = NULL;
}
- if (unlikely(task->wq)) {
+ if (unlikely(task->wq != NULL)) {
tree_delete(task->wq);
task->wq = NULL;
}