Remove deprecated debug output functions from 21 source files. #9683
Conversation
| ~NetAcceptAction() override { Debug("net_accept", "NetAcceptAction dying"); } | ||
| ~NetAcceptAction() override | ||
| { | ||
| static DbgCtl dbg_ctl{"net_accept"}; |
There was a problem hiding this comment.
Are there any concerns about late initialization for this? IIRC the compiler can arrange for dbg_ctl to be constructed on first invocation rather than process start. Is the constructor thread safe?
There was a problem hiding this comment.
I used the reference count work-around for the C++ initialization/destruction order fiasco. Construction of a DbgCtl increments the reference count to the tag container. The tag container is created when the reference count goes from 0 to 1:
trafficserver/include/tscore/DbgCtl.h
Line 38 in 2acf9a4
trafficserver/src/tscore/DbgCtl.cc
Line 123 in 2acf9a4
trafficserver/src/tscore/DbgCtl.cc
Line 72 in 2acf9a4
I probably would have gotten a promotion for figuring all of this out, if I were under 40 and it involved YAML.
There was a problem hiding this comment.
Also note that the static local DbgCtl is key to getting the old Debug() call to have the improved performance:
trafficserver/include/tscore/Diags.h
Line 218 in 9cd7e57
That code has gone through both years of review and running in production.
There was a problem hiding this comment.
If you want to see something really scary, pull over, here's a static DbgCtl that's local to a lambda:
trafficserver/include/tscore/Diags.h
Line 240 in 9cd7e57
There was a problem hiding this comment.
I was much more concerned about thread safety. Can the registry be updated from multiple threads concurrently? Consider line "DbgCtl.cc:158". Is that thread safe?
auto res = d.set.insert(ctl);
There was a problem hiding this comment.
Yes because the constructor of _RegistryAccessor takes the registry's mutex:
trafficserver/src/tscore/DbgCtl.cc
Line 139 in fc76b29
iocore/net/UnixNet.cc
Outdated
| DbgCtl dbg_ctl_v_net_queue{"v_net_queue"}; | ||
| DbgCtl dbg_ctl_iocore_net_main{"iocore_net_main"}; | ||
|
|
||
| #ifdef DEBUG |
There was a problem hiding this comment.
Is it worth #ifdef for these? It's not much of a footprint.
There was a problem hiding this comment.
I'm indifferent, what's you preference? One could make the argument that DDbg() itself is not worth it.
SolidWallOfCode
left a comment
SolidWallOfCode
left a comment
There was a problem hiding this comment.
My only real concern is with the controls that are function local and when, exactly, those are constructed.
|
[approve ci centos] |
ywkaras
force-pushed
the
NewDbg4
branch
3 times, most recently
from
a493f4a to
8fa41fb
Compare
ywkaras
force-pushed
the
NewDbg4
branch
3 times, most recently
from
697fa7f to
40bf579
Compare
ywkaras
changed the title
|
@SolidWallOfCode if this doesn' t get reviewed soon, @zwoop will throw me in the wood chipper in the backyard of the Denver Apple site. And you'll have to live with the guilt. |
ywkaras
force-pushed
the
NewDbg4
branch
2 times, most recently
from
845c5a6 to
3247c5e
Compare
zwoop
left a comment
zwoop
left a comment
There was a problem hiding this comment.
Not 100% sure I like the pattern with static "ctl"'s in functions / scopes, but I'll survive.
|
As we discussed, I investigated the overhead of local static that are dynamically initialized. I looked at the assembler generated by some example code. In this example, if the local static in f() is already initialized, the overhead will be executing instructions on lines 3 through 5 of the assembler code. The mutex locking only comes in if it's not initialized, to avoid a race condition if multiple threads see it as uninitialized, and try to initialize it. https://godbolt.org/z/EjM9jsPz3 |
* commit '236b749b2b3cc746829ad534a7034ab7799d1b71': Allow origins to do TLS renegotiation (apache#10385) Remove deprecated debug output functions from 21 source files. (apache#9683) Fixes some make test build problems (apache#10402) Removes unused Errata functions from WCCP (apache#10380) Move InkAPI.cc into src/api (apache#10315) cmake: Generate files in rc, install the trafficserver script (apache#10367) Add support for OCSP requests by GET method (apache#10306) Preserve unmapped url regardless of need for remapping (apache#10304) Add TSVConnFdGet api (apache#10324) include/ts: comma on all last enum elements (apache#10400) cmake: Add remaining plugins without external deps (apache#10395) CID-1508974 (apache#10397) CID-1508987 (apache#10398) Coverity 1518564: fix off by one (apache#10401)
4 participants
No description provided.