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.
> >>
> >>
> >>
> >>
> >>
> >>
>
>
