On 23/09/2012 12:04, Rainer Jung wrote:
> Hi Mark,
>
> On 23.09.2012 11:43, ma...@apache.org wrote:
>> Author: markt
>> Date: Sun Sep 23 09:43:43 2012
>> New Revision: 1388991
>>
>> URL: http://svn.apache.org/viewvc?rev=1388991&view=rev
>> Log:
>> Use CharBuffer rather than StringBuilder to build the access log
>> message to:
>> - save a char[] to String conversion and the associated garbage
>> - allow buffers to be recycled also saving garbage
>> Reduces allocations due to the AccessLog from 43% to 27% of the
>> overall allocations
>>
>> Modified:
>> tomcat/trunk/java/org/apache/catalina/valves/AccessLogValve.java
>>
>> tomcat/trunk/java/org/apache/catalina/valves/ExtendedAccessLogValve.java
>> tomcat/trunk/webapps/docs/config/valve.xml
>>
>> Modified:
>> tomcat/trunk/java/org/apache/catalina/valves/AccessLogValve.java
>> URL:
>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/valves/AccessLogValve.java?rev=1388991&r1=1388990&r2=1388991&view=diff
>>
>> ==============================================================================
>>
>> --- tomcat/trunk/java/org/apache/catalina/valves/AccessLogValve.java
>> (original)
>> +++ tomcat/trunk/java/org/apache/catalina/valves/AccessLogValve.java
>> Sun Sep 23 09:43:43 2012
>> @@ -570,13 +573,28 @@ public class AccessLogValve extends Valv
>> */
>> protected boolean requestAttributesEnabled = false;
>>
>> - // -------------------------------------------------------------
>> Properties
>> + /**
>> + * Buffer pool used for log message generation. Pool used to
>> reduce garbage
>> + * generation.
>> + */
>> + private Queue<CharBuffer> charBuffers = new
>> ConcurrentLinkedQueue<>();
>>
>> /**
>> - * @return Returns the enabled.
>> + * Log message buffers are usually recycled and re-used. To prevent
>> + * excessive memory usage, if a buffer grows beyond this size it
>> will be
>> + * discarded. The default is 256 characters. This should be set
>> to larger
>> + * than the typical access log message size.
>> */
>> - public boolean getEnabled() {
>> - return enabled;
>> + private int maxLogMessageBufferSize = 256;
>> +
>> + // -------------------------------------------------------------
>> Properties
>> +
>> + public int getMaxLogMessageBufferSize() {
>> + return maxLogMessageBufferSize;
>> + }
>> +
>> + public void setMaxLogMessageBufferSize(int
>> maxLogMessageBufferSize) {
>> + this.maxLogMessageBufferSize = maxLogMessageBufferSize;
>> }
>>
>> /**
>> @@ -596,6 +614,13 @@ public class AccessLogValve extends Valv
>> }
>>
>> /**
>> + * @return Returns the enabled.
>> + */
>> + public boolean getEnabled() {
>> + return enabled;
>> + }
>> +
>> + /**
>> * @param enabled
>> * The enabled to set.
>> */
>> @@ -933,13 +958,23 @@ public class AccessLogValve extends Valv
>> long start = request.getCoyoteRequest().getStartTime();
>> Date date = getDate(start + time);
>>
>> - StringBuilder result = new StringBuilder(128);
>> + CharBuffer result = charBuffers.poll();
>> + if (result == null) {
>
> I'm just curious here: do you already know whether using a direct buffer
> would be better or worse (e.g.
> ByteBuffer.allocateDirect(128).asCharBuffer())?
No, I haven't tested that.
>> + result = CharBuffer.allocate(128);
>> + }
>>
>> for (int i = 0; i < logElements.length; i++) {
>> logElements[i].addElement(result, date, request,
>> response, time);
>> }
>>
>> - log(result.toString());
>> + result.flip();
>> + log(result);
>> +
>> + // TODO - Make this configurable
>
> 256 -> maxLogMessageBufferSize ?
Yes. Thanks. I hard-coded it to test, added all the configuration and
forgot the important bit. Good catch.
> "<" -> "<=" ?
To align with the docs, yes. I'll fix that too.
>> + if (result.length() < 256) {
>> + result.clear();
>> + charBuffers.add(result);
>> + }
>> }
>>
>>
>
>> @@ -1676,19 +1713,19 @@ public class AccessLogValve extends Valv
>> }
>>
>> @Override
>> - public void addElement(StringBuilder buf, Date date, Request
>> request,
>> + public void addElement(CharBuffer buf, Date date, Request
>> request,
>> Response response, long time) {
>> if (millis) {
>> - buf.append(time);
>> + buf.append(Long.toString(time));
>> } else {
>> // second
>> - buf.append(time / 1000);
>> + buf.append(Long.toString(time / 1000));
>> buf.append('.');
>
> I think the following code originally was meant to improve performance
> by slicing the 3 digit number "remains" into a sequence of digits. I
> expect the below to be a bit more efficient and less complicated now
> that Long.toString() is used:
>
>> int remains = (int) (time % 1000);
>> - buf.append(remains / 100);
>> + buf.append(Long.toString(remains / 100));
>> remains = remains % 100;
>> - buf.append(remains / 10);
>> - buf.append(remains % 10);
>> + buf.append(Long.toString(remains / 10));
>> + buf.append(Long.toString(remains % 10));
>
> buf.append(Long.toString(time % 1000));
I'll switch that too.
Cheers,
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
