Re: (tomcat) branch main updated: Fix disastrous cookie-logging patch.
Chuck, On 4/19/24 10:48, Chuck Caldarale wrote: On Apr 19, 2024, at 09:18, Christopher Schultz wrote: Hopefully this patch has the intended effect. ;) I’m not convinced this change will have any measurable performance improvement. The JVM C2 compiler is pretty good with escape analysis, so an unused StringBuilder object may not even get allocated. It should get allocated, since the constructor needs to be called. But it may be allocated in a cheap memory region and immediately become speedily-collected garbage. Also, there’s now an added comparison for each iteration of the cookies loop, plus the additional code for an object allocation. This enlarges the body of the loop, putting more pressure on the microcode cache in the CPU, possibly making each iteration take longer. That's a fair criticism. Are there any practical examples that show a performance benefit or GC reduction? None. I made this change merely based upon code inspection. Since this code executes for every single request, I guessed without evidence that reduction of memory-churn would be beneficial. -chris On 4/19/24 10:17, schu...@apache.org wrote: This is an automated email from the ASF dual-hosted git repository. schultz pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git The following commit(s) were added to refs/heads/main by this push: new cbefe8624e Fix disastrous cookie-logging patch. cbefe8624e is described below commit cbefe8624ee5d6255955134d08498f9926295126 Author: Christopher Schultz AuthorDate: Fri Apr 19 10:16:36 2024 -0400 Fix disastrous cookie-logging patch. --- java/org/apache/catalina/valves/AbstractAccessLogValve.java | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/java/org/apache/catalina/valves/AbstractAccessLogValve.java b/java/org/apache/catalina/valves/AbstractAccessLogValve.java index 0576b83442..dd29a5ec37 100644 --- a/java/org/apache/catalina/valves/AbstractAccessLogValve.java +++ b/java/org/apache/catalina/valves/AbstractAccessLogValve.java @@ -1513,17 +1513,19 @@ public abstract class AbstractAccessLogValve extends ValveBase implements Access if (cookies != null) { for (Cookie cookie : cookies) { if (cookieNameToLog.equals(cookie.getName())) { +if (value == null) { +value = new StringBuilder(); +} if (first) { first = false; } else { value.append(','); } -value = new StringBuilder(); value.append(cookie.getValue()); } } } -if (value.length() == 0) { +if (value == null) { buf.append('-'); } else { escapeAndAppend(value.toString(), buf); - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: (tomcat) branch main updated: Fix disastrous cookie-logging patch.
> On Apr 19, 2024, at 09:18, Christopher Schultz > wrote: > > Hopefully this patch has the intended effect. ;) I’m not convinced this change will have any measurable performance improvement. The JVM C2 compiler is pretty good with escape analysis, so an unused StringBuilder object may not even get allocated. Also, there’s now an added comparison for each iteration of the cookies loop, plus the additional code for an object allocation. This enlarges the body of the loop, putting more pressure on the microcode cache in the CPU, possibly making each iteration take longer. Are there any practical examples that show a performance benefit or GC reduction? - Chuck > -chris > > On 4/19/24 10:17, schu...@apache.org wrote: >> This is an automated email from the ASF dual-hosted git repository. >> schultz pushed a commit to branch main >> in repository https://gitbox.apache.org/repos/asf/tomcat.git >> The following commit(s) were added to refs/heads/main by this push: >> new cbefe8624e Fix disastrous cookie-logging patch. >> cbefe8624e is described below >> commit cbefe8624ee5d6255955134d08498f9926295126 >> Author: Christopher Schultz >> AuthorDate: Fri Apr 19 10:16:36 2024 -0400 >> Fix disastrous cookie-logging patch. >> --- >> java/org/apache/catalina/valves/AbstractAccessLogValve.java | 6 -- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> diff --git a/java/org/apache/catalina/valves/AbstractAccessLogValve.java >> b/java/org/apache/catalina/valves/AbstractAccessLogValve.java >> index 0576b83442..dd29a5ec37 100644 >> --- a/java/org/apache/catalina/valves/AbstractAccessLogValve.java >> +++ b/java/org/apache/catalina/valves/AbstractAccessLogValve.java >> @@ -1513,17 +1513,19 @@ public abstract class AbstractAccessLogValve extends >> ValveBase implements Access >> if (cookies != null) { >> for (Cookie cookie : cookies) { >> if (cookieNameToLog.equals(cookie.getName())) { >> +if (value == null) { >> +value = new StringBuilder(); >> +} >> if (first) { >> first = false; >> } else { >> value.append(','); >> } >> -value = new StringBuilder(); >> value.append(cookie.getValue()); >> } >> } >> } >> -if (value.length() == 0) { >> +if (value == null) { >> buf.append('-'); >> } else { >> escapeAndAppend(value.toString(), buf); >> - >> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org >> For additional commands, e-mail: dev-h...@tomcat.apache.org > > - > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: (tomcat) branch main updated: Fix disastrous cookie-logging patch.
All, Hopefully this patch has the intended effect. ;) -chris On 4/19/24 10:17, schu...@apache.org wrote: This is an automated email from the ASF dual-hosted git repository. schultz pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git The following commit(s) were added to refs/heads/main by this push: new cbefe8624e Fix disastrous cookie-logging patch. cbefe8624e is described below commit cbefe8624ee5d6255955134d08498f9926295126 Author: Christopher Schultz AuthorDate: Fri Apr 19 10:16:36 2024 -0400 Fix disastrous cookie-logging patch. --- java/org/apache/catalina/valves/AbstractAccessLogValve.java | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/java/org/apache/catalina/valves/AbstractAccessLogValve.java b/java/org/apache/catalina/valves/AbstractAccessLogValve.java index 0576b83442..dd29a5ec37 100644 --- a/java/org/apache/catalina/valves/AbstractAccessLogValve.java +++ b/java/org/apache/catalina/valves/AbstractAccessLogValve.java @@ -1513,17 +1513,19 @@ public abstract class AbstractAccessLogValve extends ValveBase implements Access if (cookies != null) { for (Cookie cookie : cookies) { if (cookieNameToLog.equals(cookie.getName())) { +if (value == null) { +value = new StringBuilder(); +} if (first) { first = false; } else { value.append(','); } -value = new StringBuilder(); value.append(cookie.getValue()); } } } -if (value.length() == 0) { +if (value == null) { buf.append('-'); } else { escapeAndAppend(value.toString(), buf); - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org