> On Apr 4, 2019, at 8:05 AM, Walt Karas <wka...@verizonmedia.com.INVALID>
> wrote:
>
> Because senility. probably "on" should be a non-pointer of type const
> volatile char, and InkAPI in the core can just keep a pointer to the whole
> structure for updating.
>
> But I'm leaning more to fixing up the capability that already kinda there.
> Which is optimizimg only for the case when all debug output is disabled.
> Especially since maybe zwoop would be on my side for once :o) .
My bad, whatever Walt said, I take the opposite stance.
— leif
P.s
J/K!
>
> On Thu, Apr 4, 2019 at 8:51 AM Alan Carroll
> <solidwallofc...@verizonmedia.com.invalid> wrote:
>
>> This could enable a significant speed up for debug tags. One point is that
>> when the debug tag string is set, at that point the debug objects could be
>> updated to the correct state according to the debug tag string, rather than
>> checking it every time a debug message is logged.
>>
>> I have to ask, why is there a pointer to the enable flag, instead of just
>> the enable flag? Your logic doesn't even check that flag, it checks whether
>> the pointer is null.
>>
>> On Wed, Apr 3, 2019 at 11:52 AM Leif Hedstrom <zw...@apache.org> wrote:
>>
>>>
>>>
>>>> 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.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>
