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