> On 17 Jan 2019, at 2:59 am, Ken Gaillot <kgail...@redhat.com> wrote:
>
> 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.
I believe my interest was the ability to turn them on dynamically in a running
program (yes, i used it plenty back in the day) and have the overhead be
minimal for the normal case when they weren't in use.
>
> 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
_______________________________________________
Developers mailing list
Developers@clusterlabs.org
https://lists.clusterlabs.org/mailman/listinfo/developers
