You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "zwoop (via GitHub)" <gi...@apache.org> on 2023/03/01 17:19:11 UTC
[GitHub] [trafficserver] zwoop opened a new pull request, #9480: Slight performance improvements before calling APIHooks::clear
zwoop opened a new pull request, #9480:
URL: https://github.com/apache/trafficserver/pull/9480
This calls the clear() method 28 times otherwise, which may seem benign, but it actually shows up as a significant CPU user in perf top. This patch added an extra 50K RPS (give or take) on my benchmark road trip.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #9480: Slight performance improvements before calling APIHooks::clear
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #9480:
URL: https://github.com/apache/trafficserver/pull/9480#issuecomment-1457227116
Can you add a comment in the code about the 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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a diff in pull request #9480: Slight performance improvements before calling APIHooks::clear
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #9480:
URL: https://github.com/apache/trafficserver/pull/9480#discussion_r1122120879
##########
proxy/InkAPIInternal.h:
##########
@@ -207,10 +207,14 @@ template <typename ID, int N>
void
FeatureAPIHooks<ID, N>::clear()
{
- for (auto &h : m_hooks) {
- h.clear();
+ if (m_hooks_p) {
+ for (auto &h : m_hooks) {
+ if (!h.is_empty()) {
Review Comment:
Part of the "problem" is that APIHOoks as we did it can not be inlined, so it calls it 28 times for no good reason :/
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9480: Slight performance improvements before calling APIHooks::clear
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9480:
URL: https://github.com/apache/trafficserver/pull/9480#discussion_r1122099978
##########
proxy/InkAPIInternal.h:
##########
@@ -207,10 +207,14 @@ template <typename ID, int N>
void
FeatureAPIHooks<ID, N>::clear()
{
- for (auto &h : m_hooks) {
- h.clear();
+ if (m_hooks_p) {
+ for (auto &h : m_hooks) {
+ if (!h.is_empty()) {
Review Comment:
Why don't you check this in `APIHooks::clear()`? I guess `clean()` could just return without doing anything if it's empty.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #9480: Slight performance improvements before calling APIHooks::clear
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #9480:
URL: https://github.com/apache/trafficserver/pull/9480#issuecomment-1458740073
> Can you add a comment in the code about the change.
Added a comment.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #9480: Slight performance improvements before calling APIHooks::clear
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #9480:
URL: https://github.com/apache/trafficserver/pull/9480#issuecomment-1470891058
Cherry-picked to v9.2.x
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #9480: Slight performance improvements before calling APIHooks::clear
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #9480:
URL: https://github.com/apache/trafficserver/pull/9480#issuecomment-1458744264
This is actually worse than I first expected, there's about 230 calls to this function. However, upon inspecting this, the problem is that
```
class HttpAPIHooks : public FeatureAPIHooks<TSHttpHookID, TS_HTTP_LAST_HOOK>
```
Creates (give or take) 180 or so APIHook objects which can never be used. There's a big gap between the last HTTP Hook (TS_HTTP_REQUEST_CLIENT_HOOK) and the first SSL hook (TS_SSL_FIRST_HOOK=201). I'm thinking that we need to move the last HTTP hook to be something like
```
TS_HTTP_LAST_HOOK = TS_HTTP_REQUEST_CLIENT_HOOK,
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop merged pull request #9480: Slight performance improvements before calling APIHooks::clear
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop merged PR #9480:
URL: https://github.com/apache/trafficserver/pull/9480
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org