From 70bcfb77a7f7aaac6c37f292f1a01ea272d25871 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Sun, 27 Jan 2008 02:21:53 +0100 Subject: [PATCH] [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. --- include/common/ebtree.h | 20 +++++++++++++++----- include/common/standard.h | 19 +++++++++++++++---- include/proto/task.h | 2 +- src/backend.c | 2 +- src/ev_epoll.c | 2 +- src/task.c | 2 +- 6 files changed, 34 insertions(+), 13 deletions(-) diff --git a/include/common/ebtree.h b/include/common/ebtree.h index 854666ab5..1a5ce8650 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 + * (C) 2002-2008 - Willy Tarreau * * 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 248bbe91b..162a9ebc0 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 chars from to . Last char is always diff --git a/include/proto/task.h b/include/proto/task.h index 6bee1b2f1..49e3a94b6 100644 --- 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 037885074..4c5eec3dd 100644 --- 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 a0c48b108..326d5a42e 100644 --- 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 7f6e0e7a2..c60961f3a 100644 --- 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; }