I'm not familiar with the reasoning for the current setup, but
pacemaker's crm_crit(), crm_error(), etc. use qb_logt(), while
crm_debug() and crm_trace() (which won't be used in ordinary runs) do
something similar to what you propose.
Pacemaker has about 1,700 logging calls that would be affected (not
counting another 2,000 debug/trace). Presumably that means Pacemaker
currently has about +16KB of memory overhead and binary size for
debug/trace logging static pointers, and that would almost double using
them for all logs. Not a big deal today? Or meaningful in an embedded
context?
Not sure if that overhead vs runtime trade-off is the original
motivation or not, but that's the first thing that comes to mind.
On Wed, 2019-01-16 at 16:20 +0100, Lars Ellenberg wrote:
> On Wed, Jan 16, 2019 at 03:44:22PM +0100, Lars Ellenberg wrote:
> > Back then when this "dynamic" logging was introduced,
> > I thought the whole point was that "quiet" callsites
> > are "cheap".
> >
> > So I think you want to
> > qb_log_callsite_get() only *once* per callsite,
> > assign that to a static pointer (as you say in the commit message).
> > And do the actual qb_log_real_() function call
> > only conditionally based on if (cs->targets).
> >
> > That way, a disabled trace logging boils down to a
> > if (cs && cs->targets)
> > ;
> > Much cheaper than what you have now.
> >
> > Or was always calling into both qb_log_callsite_get() and
> > qb_log_real_()
> > intentional for some obscure (to me) reason,
> > even for disabled call sites?
> >
> > Below I also added a test for (cs->tags & QB_LOG_TAG_LIBQB_MSG),
> > in case someone used qb_util_set_log_function().
> > If that special tag flag could be folded into cs->targets (e.g. bit
> > 0),
> > I'd like it even better.
> >
> > Cheers,
> > Lars
> >
> >
> > diff --git a/include/qb/qblog.h b/include/qb/qblog.h
> > index 1943b94..f63f4ad 100644
>
> Oops, that patch was against *before* the 633f262 commit :-)
> and that's why I did not notice the clash in macro argument "tags"
> and struct member "tags" when compile testing...
> I forgot I jumped between checkouts :-(
>
> anyways, the (now even make check testet)
> patch for *after* 633f262 commit:
>
> diff --git a/include/qb/qblog.h b/include/qb/qblog.h
> index 31981b8..ae1d25c 100644
> --- a/include/qb/qblog.h
> +++ b/include/qb/qblog.h
> @@ -340,11 +340,17 @@ void qb_log_from_external_source_va(const char
> *function,
> * @param fmt usual printf style format specifiers
> * @param args usual printf style args
> */
> -#define qb_logt(priority, tags, fmt, args...) do { \
> - struct qb_log_callsite* descriptor_pt = \
> - qb_log_callsite_get(__func__, __FILE__, fmt, \
> - priority, __LINE__, tags); \
> - qb_log_real_(descriptor_pt, ##args); \
> +#define qb_logt(priority, tags_, fmt, args...) do { \
> + static struct qb_log_callsite* descriptor_pt; \
> + if (!descriptor_pt) { \
> + descriptor_pt = \
> + qb_log_callsite_get(__func__, __FILE__, fmt, \
> + priority, __LINE__, tags_); \
> + } \
> + if (descriptor_pt && (descriptor_pt->targets || \
> + qb_bit_set(descriptor_pt->tags, \
> + QB_LOG_TAG_LIBQB_MSG_BIT))) \
> + qb_log_real_(descriptor_pt, ##args); \
> } while(0)
>
> _______________________________________________
> Developers mailing list
> Developers@clusterlabs.org
> https://lists.clusterlabs.org/mailman/listinfo/developers
--
Ken Gaillot <kgail...@redhat.com>
_______________________________________________
Developers mailing list
Developers@clusterlabs.org
https://lists.clusterlabs.org/mailman/listinfo/developers
