What do you mean by "exposing diags"?
On Wed, Jun 12, 2019 at 11:50 AM Bryan Call <bc...@apache.org> wrote:
> I would rather see TSDebug be optimized and exposing diags instead of
> creating a new type of debug.
>
> -Bryan
>
>
> > On Jun 12, 2019, at 9:44 AM, Walt Karas <wka...@verizonmedia.com.INVALID>
> wrote:
> >
> > I don't believe so. The issue I'm trying to address is that there is
> still
> > significant execution time overhead for debug output calls when
> > proxy.config.diags.debug.enabled is 0. This change would make the
> > execution time overhead negligible (if TSFastDbg is used or TSDebug is
> > changed to be like the proposed TSFastDbg). Another way of putting it
> is,
> > I want to clean up the existing TSDEBUG alternative to TSDebug.
> >
> > On Wed, Jun 12, 2019 at 11:31 AM Bryan Call <bc...@apache.org> wrote:
> >
> >> How would this be controlled on the command line when running
> >> trafic_server -T?
> >>
> >> It looks like it is a departure from how debug tags and controlled now.
> >>
> >> -Bryan
> >>
> >>
> >>> On Jun 11, 2019, at 1:21 PM, Walt Karas <wka...@verizonmedia.com
> .INVALID>
> >> wrote:
> >>>
> >>> Any +1's for simply replacing TSDebug with the proposed TSFastDbg ?
> >>>
> >>> On Mon, Apr 8, 2019 at 6:20 PM Leif Hedstrom <zw...@apache.org> wrote:
> >>>
> >>>>
> >>>>
> >>>> 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.
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>
> >>
>
>
