> On Apr 3, 2019, at 10:47 AM, Walt Karas <wka...@verizonmedia.com.INVALID>
> wrote:
>
> It looks like the TS API already has less granular version of this:
>
> extern int diags_on_for_plugins;
>
> #define TSDEBUG \
>
> if (diags_on_for_plugins) \
>
> TSDebug
>
>
> A problem with this is if you write:
>
>
> if (flag)
>
> TSDEBUG(yadayada);
>
> else
>
> do_something();
>
>
> then the else will be associated with the if hidden in the macro. I'd
> prefer to change to something like:
>
>
> extern const volatile char TS_detail_dbg_output_enabled;
>
>
> #define TSFastDbg(_TAG, ...) \
> do { \
> if (TS_detail_dbg_output_enabled) \
> TSDebug(_TAG, __VA_ARGS__); \
>
> } while(0)
Yeh, that kinda sounds like a bug, doesn’t it?
>
> The negative is that we'd only see the benefit when all debug output was
> disabled. But, in any case, that is the main benefit, to avoid impacts
> during normal operation.
Right, we should always optimize for the common cases, obviously. When there’s
a decision to make, where you can either make it “equally” expensive always, or
more expensive in a rare case, but cheaper in the common case, we should likely
chose the latter.
>
> Looking in src/tscore/Diags.cc, it looks like the debug enable flag is only
> set at startup time. So we wouldn't be able to enable TSFastDbg using
> traffic_cntl without adding complexity.
>
> On Tue, Apr 2, 2019 at 5:48 PM Walt Karas <wka...@verizonmedia.com> wrote:
>
>> Add this new structure to apidefs.h:
>>
>> typedef struct TSFastDbgCntl_
>> {
>> const char * const tag; // nul-terminated string
>> const volatile char * const on; // pointer to 1-byte flag
>> }
>> TSFastDbgCntl;
>>
>> Add this new API function, that returns a pointer to an instance of the
>> above structure:
>>
>> TSFastDbgCntl * TSCreateFastDbgCntl(const char *tag);
>>
>> Add this macro, which would be used in place of direct use of TSDebug().
>>
>> #define TSFastDbg(_FD_CNTL, ...) \
>> do { \
>> if ((_FD_CNTL)->on) \
>> TSDebug((_FD_CNTL)->tag, __VA_ARGS__); \
>> } while(0)
>>
>> The first parameter to TSFastDbg() is a pointer that was returned by
>> TSCreateFastDbgCntl(). The remaining parameters are printf parameters
>> (format string and values matching format specifiers). The core would be
>> responsible for changing the value of the 'on' fields in the TSFastDbgCntl
>> instances. For direct calls to TSDebug(), even if the tag is disabled, all
>> the parameters have to be prepared and pushed onto the stack. As was
>> discussed in IRC, you have to feel guilty about doing converting a
>> string_view to a string and outputting it with c_str(). Because the code
>> to do this will be executed even if the debug tag is disabled.
>> Furthermore, for each call to TSDebug, it's necessary to do an associative
>> lookup on the tag string to determine that output for the tag is disabled.
>>
>> We would want to put equivalent capabilities in Diags.h in the core for
>> debug output. The implementation is non-trivial, but I think it would take
>> at most a week including the Au test additions.
>>
>> I looked at making TSFastDbg() an inline function rather than a macro:
>> https://godbolt.org/z/IfVbBk
>> The opitimization works well if you compile the code with the inline
>> function as C, but the optimization is poor if you compile it as C++. This
>> difference exists for both gcc and clang. It's weird.
>>
>>
>>
>>
>>
>>
