When I've done debug tags like this in the past, it ended up like
a simpler version of records.config. IE
bool your_tag = false;
register_tag("your_tag_name", &your_tag); //store a map<string,pointer> can
be referenced by name.
if (your_tag) printf(...); // direct access, no derefences or indirection
with additiona API
void set_tag_by_name(string const &name, bool value);
bool get_tag_by_name(string const &name);
^ it can be templated to handle more than bools if you like
I'm not sure if this better performance than Walt+Alan's suggestion, but
just another style.
On Thu, Apr 4, 2019 at 9:18 AM 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.
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>
>
>
