Re: (tomcat) branch main updated: Fix disastrous cookie-logging patch.

2024-04-26 Thread Christopher Schultz 

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.

2024-04-19 Thread Chuck Caldarale 


> 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.

2024-04-19 Thread Christopher Schultz 

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