Bonjour Michaël,
TBH, my recommendation would be to drop *all* of these likely()
and unlikely() calls. What evidence have you got that those are
meaningfully improving the quality of the generated code? And if
they're buried inside macros, they certainly aren't doing anything
useful in terms of documenting the code.
Yes. I am wondering if we should not rework this part of the logging
with something like the attached. My 2c, thoughts welcome.
ISTM that the intent is to minimise the performance impact of ignored
pg_log calls, especially when under debug where it is most likely to be
the case AND that they may be in critical places.
Compared to dealing with the level inside the call, the use of the level
variable avoids a call-test-return cycle in this case, and the unlikely
should help the compiler reorder instructions so that no actual branch is
taken under the common case.
So I think that the current situation is a good thing at least for debug.
For other levels, they are on by default AND would not be placed at
critical performance points, so the whole effort of avoiding the call are
moot.
I agree with Tom that __pg_log_level variable name violates usages.
ISTM that switching the variable to explicitely global solves the issues,
and that possible the level test can be moved to inside the function for
all but the debug level. See attached which reprises some of your idea,
but keep the outside filtering for the debug level.
--
Fabien.
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 39c1a243d5..e7b33bb8e0 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3295,7 +3295,7 @@ executeMetaCommand(CState *st, instr_time *now)
argc = command->argc;
argv = command->argv;
- if (unlikely(__pg_log_level <= PG_LOG_DEBUG))
+ if (unlikely(pg_log_current_level <= PG_LOG_DEBUG))
{
PQExpBufferData buf;
diff --git a/src/common/logging.c b/src/common/logging.c
index c78ae793b8..ce12593e32 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -13,7 +13,7 @@
#include "common/logging.h"
-enum pg_log_level __pg_log_level;
+enum pg_log_level pg_log_current_level;
static const char *progname;
static int log_flags;
@@ -45,7 +45,7 @@ pg_logging_init(const char *argv0)
setvbuf(stderr, NULL, _IONBF, 0);
progname = get_progname(argv0);
- __pg_log_level = PG_LOG_INFO;
+ pg_log_current_level = PG_LOG_INFO;
if (pg_color_env)
{
@@ -107,7 +107,7 @@ pg_logging_config(int new_flags)
void
pg_logging_set_level(enum pg_log_level new_level)
{
- __pg_log_level = new_level;
+ pg_log_current_level = new_level;
}
void
@@ -142,6 +142,9 @@ pg_log_generic_v(enum pg_log_level level, const char *pg_restrict fmt, va_list a
size_t required_len;
char *buf;
+ if (unlikely(pg_log_current_level > level))
+ return;
+
Assert(progname);
Assert(level);
Assert(fmt);
diff --git a/src/include/common/logging.h b/src/include/common/logging.h
index 028149c7a1..c73c4ee76c 100644
--- a/src/include/common/logging.h
+++ b/src/include/common/logging.h
@@ -55,7 +55,7 @@ enum pg_log_level
PG_LOG_OFF,
};
-extern enum pg_log_level __pg_log_level;
+extern enum pg_log_level pg_log_current_level;
/*
* Kind of a hack to be able to produce the psql output exactly as required by
@@ -72,24 +72,16 @@ void pg_logging_set_locus_callback(void (*cb) (const char **filename, uint64 *l
void pg_log_generic(enum pg_log_level level, const char *pg_restrict fmt,...) pg_attribute_printf(2, 3);
void pg_log_generic_v(enum pg_log_level level, const char *pg_restrict fmt, va_list ap) pg_attribute_printf(2, 0);
-#define pg_log_fatal(...) do { \
- if (likely(__pg_log_level <= PG_LOG_FATAL)) pg_log_generic(PG_LOG_FATAL, __VA_ARGS__); \
- } while(0)
+#define pg_log_fatal(...) pg_log_generic(PG_LOG_FATAL, __VA_ARGS__)
-#define pg_log_error(...) do { \
- if (likely(__pg_log_level <= PG_LOG_ERROR)) pg_log_generic(PG_LOG_ERROR, __VA_ARGS__); \
- } while(0)
+#define pg_log_error(...) pg_log_generic(PG_LOG_ERROR, __VA_ARGS__)
-#define pg_log_warning(...) do { \
- if (likely(__pg_log_level <= PG_LOG_WARNING)) pg_log_generic(PG_LOG_WARNING, __VA_ARGS__); \
- } while(0)
+#define pg_log_warning(...) pg_log_generic(PG_LOG_WARNING, __VA_ARGS__)
-#define pg_log_info(...) do { \
- if (likely(__pg_log_level <= PG_LOG_INFO)) pg_log_generic(PG_LOG_INFO, __VA_ARGS__); \
- } while(0)
+#define pg_log_info(...) pg_log_generic(PG_LOG_INFO, __VA_ARGS__)
#define pg_log_debug(...) do { \
- if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) pg_log_generic(PG_LOG_DEBUG, __VA_ARGS__); \
+ if (unlikely(pg_log_current_level <= PG_LOG_DEBUG)) pg_log_generic(PG_LOG_DEBUG, __VA_ARGS__); \
} while(0)
#endif /* COMMON_LOGGING_H */
