You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/03/16 16:45:06 UTC
[GitHub] [trafficserver] ywkaras opened a new pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
ywkaras opened a new pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452
Also adds OneWriterMultiReader.h, higher-performance alternative to std::shared_mutex (also with logic to prevent writer starvation).
Setting proxy.config.diags.debug.enabled to 1 or 3 enables output for Debug macros with DbgCtl as first parameter or TSFDbg if the tag for the DbgClt/TSFDbCtl matches debug.tags regular expression. (A value of 1 still only enables Debug/TSDebug macros with a C-string tag as the first parameter.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#discussion_r574175850
##########
File path: include/tscore/OneWriterMultiReader.h
##########
@@ -0,0 +1,225 @@
+/** @file
+
Review comment:
One thing I couldn't understand in my performance testing is, no matter how I tweaked the parameters of h2load, I couldn't get under 65% idle on atslab00. All the requests were the same, the (cleartext) GET of a single object in cache, so maybe ATS was disk I/O bound?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a change in pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#discussion_r574159704
##########
File path: include/tscore/OneWriterMultiReader.h
##########
@@ -0,0 +1,225 @@
+/** @file
+
Review comment:
Is there a reason this PR rolls its own reader/writer lock rather than using one of the standard implementations?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #7452: Add FDbg/TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#issuecomment-767973856
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#discussion_r574210143
##########
File path: include/tscore/OneWriterMultiReader.h
##########
@@ -0,0 +1,225 @@
+/** @file
+
Review comment:
An important point to keep in mind is that, in my testing, I used a tag regex that didn't match any tag. So the 10% drop in throughput did not include the overhead of any actual output. Running in production with a regex like "http|dns" would result in a much bigger drop in throughput than 10%. Debugging could only be turned on in production with a regex that enabled a small amount of debug output.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#issuecomment-775459027
I did some performance testing, here are the details: https://gist.github.com/ywkaras/4b0386341a4a6ce0afacaf46dcce79d6 .
In my test scenario, enabling debug output (with a regex that matches no tag) reduces requests per second by about 10% with this change, versus 95% without this change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras removed a comment on pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
ywkaras removed a comment on pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#issuecomment-772106409
[approve ci Clang-Analyzer]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#issuecomment-772106409
[approve ci Clang-Analyzer]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#issuecomment-775469389
The Clang-Agonizer error is in DNS.cc, which this PR does not change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#issuecomment-770151389
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#issuecomment-772106409
[approve ci Clang-Analyzer]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras removed a comment on pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
ywkaras removed a comment on pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#issuecomment-772106409
[approve ci Clang-Analyzer]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#issuecomment-770146577
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras removed a comment on pull request #7452: Add FDbg/TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
ywkaras removed a comment on pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#issuecomment-767967027
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #7452: Add FDbg/TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#issuecomment-767967027
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras removed a comment on pull request #7452: Add FDbg/TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
ywkaras removed a comment on pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#issuecomment-767973856
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras closed pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
ywkaras closed pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#issuecomment-776269314
Merging this with https://github.com/apache/trafficserver/pull/7279 will be a bit nasty.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a change in pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#discussion_r574162919
##########
File path: include/tscore/OneWriterMultiReader.h
##########
@@ -0,0 +1,225 @@
+/** @file
+
Review comment:
Ah, I didn't carefully read your initial comment. What is the performance hit of using the std implementation of a reader_writer lock?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#discussion_r574172417
##########
File path: include/tscore/OneWriterMultiReader.h
##########
@@ -0,0 +1,225 @@
+/** @file
+
Review comment:
In the scenario I tested, I saw more than double the requests per second using my shared mutex implementation.
But, in truth, if we go with the second option (where each Debug() call has an instance of DbgCtl, even if it has to create it itself), it will be moot, the lock will almost never need to be locked.
Aside from better performance, my shared mutex has logic to prevent writer starvation. But it's not recursive.
It was actually a mistake that I implemented my own shared mutex, I didn't realize pthreads (and C++17) had one. But looking like a lucky accident.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras closed pull request #7452: Reduce overhead incurred by enabling debug output.
Posted by GitBox <gi...@apache.org>.
ywkaras closed pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7452: Change Debug, add TSFDbg macros for debug output, faster checking to see if enabled.
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7452:
URL: https://github.com/apache/trafficserver/pull/7452#issuecomment-777114506
Looks like a good approach. The performance numbers are encouraging.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org