<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 9 Jun 2017, at 17:47, Frediano Ziglio <<a href="mailto:fziglio@redhat.com" class="">fziglio@redhat.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div style="" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class=""><br class="">On Fri, 2017-06-09 at 05:47 -0400, Frediano Ziglio wrote:<br class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><br class="">Calling spice_util_get_debug() from the SPICE_DEBUG() macro is<br class="">unnecessary since g_log() will already check whether the message<br class="">will<br class="">actually be printed. The only benefit to calling this function from<br class="">SPICE_DEBUG() is that it ensures that the SPICE_DEBUG environment<br class="">variable gets read the very first time we try to log something with<br class="">this macro. To solve this problem we instead use a constructor<br class="">function<br class="">to ensure that the env var is read at startup.<br class=""><br class="">Signed-off-by: Jonathon Jongsma <<a href="mailto:jjongsma@redhat.com" class="">jjongsma@redhat.com</a>><br class="">---<br class=""> src/spice-util.c | 17 ++++++++++-------<br class=""> src/spice-util.h | 7 ++-----<br class=""> 2 files changed, 12 insertions(+), 12 deletions(-)<br class=""><br class="">diff --git a/src/spice-util.c b/src/spice-util.c<br class="">index 86377b6..848f20a 100644<br class="">--- a/src/spice-util.c<br class="">+++ b/src/spice-util.c<br class="">@@ -18,6 +18,7 @@<br class=""> */<br class=""> #include "config.h"<br class=""> <br class="">+#include <common/macros.h><br class=""> #include <stdbool.h><br class=""> #include <stdlib.h><br class=""> #include <string.h><br class=""></blockquote><br class="">I would prefer the include after system includes, like spice-server<br class="">rules.<br class=""><br class=""><blockquote type="cite" class="">@@ -63,13 +64,6 @@ static void<br class="">spice_util_enable_debug_messages(void)<br class=""> <span class="Apple-converted-space"> </span>**/<br class=""> void spice_util_set_debug(gboolean enabled)<br class=""> {<br class="">- /* Make sure debug_once has been initialised<br class="">- * with the value of SPICE_DEBUG already, otherwise<br class="">- * spice_util_get_debug() may overwrite the value<br class="">- * that was just set using spice_util_set_debug()<br class="">- */<br class="">- spice_util_get_debug();<br class="">-<br class=""> if (enabled) {<br class=""> spice_util_enable_debug_messages();<br class=""> }<br class="">@@ -88,6 +82,15 @@ static gpointer getenv_debug(gpointer data)<br class=""> return GINT_TO_POINTER(debug);<br class=""> }<br class=""> <br class="">+/* Make sure debug_once has been initialised with the value of<br class="">SPICE_DEBUG<br class="">at<br class="">+ * startup, otherwise spice_util_get_debug() may overwrite the<br class="">value that is<br class="">+ * set using spice_util_set_debug() */<br class="">+SPICE_CONSTRUCTOR_FUNC(spice_log_init)<br class="">+{<br class="">+ /* ensure that we enable debugging if the SPICE_DEBUG variable<br class="">is set */<br class="">+ spice_util_get_debug();<br class="">+}<br class="">+<br class=""> gboolean spice_util_get_debug(void)<br class=""> {<br class=""> g_once(&debug_once, getenv_debug, NULL);<br class=""></blockquote><br class="">the g_once/GOnce is redundant with constructors. Constructors are<br class="">called in a single thread environment so don't need any serialization<br class="">(note: destructors don't follow this rule).<br class=""></blockquote><br class="">Right.<br class=""><br class=""><blockquote type="cite" class="">You could then use a simpler variable and write all SPICE_DEBUG check<br class="">in the constructor.<br class="">At this point you could use the new variable instead of<br class="">spice_util_get_debug (maybe using a static inline function to keep<br class="">API/flexibility).<br class=""><br class=""><blockquote type="cite" class="">diff --git a/src/spice-util.h b/src/spice-util.h<br class="">index a2a7683..7a95a9e 100644<br class="">--- a/src/spice-util.h<br class="">+++ b/src/spice-util.h<br class="">@@ -32,11 +32,8 @@ gulong spice_g_signal_connect_object(gpointer<br class="">instance,<br class=""> GConnectFlags connect_flags);<br class=""> gchar* spice_uuid_to_string(const guint8 uuid[16]);<br class=""> <br class="">-#define SPICE_DEBUG(fmt, ...) \<br class="">- do { \<br class="">- if (G_UNLIKELY(spice_util_get_debug())) \<br class="">- g_debug(G_STRLOC " " fmt, ## __VA_ARGS__); \<br class="">- } while (0)<br class="">+#define SPICE_DEBUG(fmt, ...) \<br class="">+ g_debug(G_STRLOC " " fmt, ## __VA_ARGS__)<br class=""> <br class=""> #define SPICE_RESERVED_PADDING (10 * sizeof(void*))<br class=""> <br class=""></blockquote><br class="">I think the unlikely here is an optimization we want avoiding the<br class="">expensive g_debug call. And would be even better with my variable<br class="">proposal.<br class=""></blockquote><br class="">Hmm. This seems like unnecessary optimization to me. The glib logging<br class="">functions already have a check to see whether the message should be<br class="">printed. So adding our own (possibly out-of-sync) check around the call<br class="">seems unnecessary. Essentially:<br class=""><br class="">if (spice_debug_enabled) {<br class=""> // function call to g_logv() which contains pseudo-code like:<br class=""> if (glib_debug_enabled) {<br class=""> // log the message<br class=""> }<br class="">}<br class=""><br class="">I think if the debug function call overhead really is expensive enough<br class="">that it starts affecting the behavior of the program, then enabling<br class="">logging we will *definitely* affect the behavior of the program. I'd<br class="">rather to see evidence that it's a problem before we optimize it.<br class=""><br class="">Jonathon<br class=""><br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Yes, but you are removing an optimization without evidence too.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">The change was introduced by f7daf5a4987af3fc8f8a15d1e1655995ff610009:</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">commit f7daf5a4987af3fc8f8a15d1e1655995ff610009</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Author: Marc-André Lureau <</span><a href="mailto:marcandre.lureau@redhat.com" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">marcandre.lureau@redhat.com</a><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">></span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Date: Wed Nov 17 22:14:15 2010 +0100</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class=""> gtk: add a flag to turn debug off, SPICE_DEBUG=1 to override</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">No rationale or explanation precisely if the G_UNLIKELY optimization</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">was intentional.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></div></blockquote><div><br class=""></div><div>About intentional or not, why this:</div><div><br class=""></div><div><div>#define spice_warn_if_fail(x) G_STMT_START { \</div><div> if G_LIKELY(x) { } else { \</div><div> spice_warning("condition `%s' failed", #x); \</div><div> } \</div><div>} G_STMT_END</div><div class=""><br class=""></div><div class="">instead of G_UNLIKELY(!(x)) ?</div><div class=""><br class=""></div><div class="">I see nothing “bad” in that code, it just looks weird.</div><div class=""><br class=""></div></div><blockquote type="cite" class=""><div class=""><div style="" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Frediano</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">_______________________________________________</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Spice-devel mailing list</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="mailto:Spice-devel@lists.freedesktop.org" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">Spice-devel@lists.freedesktop.org</a><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="https://lists.freedesktop.org/mailman/listinfo/spice-devel" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">https://lists.freedesktop.org/mailman/listinfo/spice-devel</a></div></div></blockquote></div><br class=""></body></html>