Also, I’d like to reiterate my call to arms on v9.0.0:
Please, everyone lets focus on the things that are critical to ATS v9.0.0
readiness!
This includes:
1) Fixing all the critical bugs (and there’s a lot of them)
2) Testing “master"
3) Doing the incompatible changes that needs to be done before we can
release
4) Removing features that we do not want to support for the next 3+
years
5) Make sure that v9.0.0 has the functionality that you *absolutely*
need to be able
to use it
I kinda feel that several of these recent proposal do not fit into this, and in
fact, are a fair amount of distraction. I urge everyone to not go down this
route until we’ve got v9.0.0 stable and functional, and instead focus on the
v9.0.0 tasks at hand. I don’t think this one for example fits into any of
those 5 points above.
My $.01.
— Leif
P.s
https://github.com/apache/trafficserver/issues/5457
> On Jun 12, 2019, at 11:30 AM, Walt Karas <wka...@verizonmedia.com.INVALID>
> wrote:
>
> I would guess the performance increase would be well under 1%. But it's a
> fairly simple change. The biggest performance increase I think would come
> from avoiding a PCRE match, to see if the specific debug tag was enabled.
> For some TSDebug calls, there can be many arguments to push, and perhaps a
> fair amount of calculation to generate the correct arguments.
>
> On Wed, Jun 12, 2019 at 12:21 PM Bryan Call <bc...@apache.org> wrote:
>
>> I am surprised you are seeing this as a performance issue. Looking at the
>> code it looks like 1 function call and a couple pointer dereferences.
>>
>> If you wanted to speed up TSDebug and make the code harder to maintain you
>> could create a structure that looks like like DiagsConfig in ts.h and then
>> set a pointer used in the API to look at the gobal DiagsConfig. Then you
>> could have a macro that would do basically what on() does. This would
>> bypass a function call and would have 1 more pointer dereference than your
>> example.
>>
>> -Bryan
>>
>>
>>> On Jun 12, 2019, at 10:00 AM, Walt Karas <wka...@verizonmedia.com.INVALID>
>> wrote:
>>>
>>> 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.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
