Details
-
Type:
Improvement
-
Status:
Closed
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: 7.0.0.pre5, 6.1.14
-
Fix Version/s: 6.1.15.rc3
-
Component/s: None
-
Labels:None
-
Number of attachments :
Description
From Joshua Ball:
Tell me if there is another place where I should send patches to. But
this one probably needs to be discussed anyway before it is committed.
When profiling a server under heavy traffic load, I noticed that a lot
of lock contention was occurring on the ArrayList class. I traced it
down to locks being acquired in the AbstractBuffers class.
For some reason ArrayList is being used as a stack. I think a queue
makes much more sense, since then items can be put onto the queue and
removed simultaneously.
The following patch replaces the use of ArrayList with a
ConcurrentLinkedQueue. Profiling the server with the patch applied
makes the contention go away.
Josh "Ua" Ball
diff -ur jetty-6.1.14_theirs/modules/jetty/src/main/java/org/mortbay/jetty/AbstractBuffers.java
jetty-6.1.14/modules/jetty/src/main/java/org/mortbay/jetty/AbstractBuffers.java
— jetty-6.1.14_theirs/modules/jetty/src/main/java/org/mortbay/jetty/AbstractBuffers.java 2008-11-13
08:19:44.000000000 -0800
+++ jetty-6.1.14/modules/jetty/src/main/java/org/mortbay/jetty/AbstractBuffers.java 2009-01-15
18:36:33.234375000 -0800
@@ -19,6 +19,7 @@
import org.mortbay.component.AbstractLifeCycle;
import org.mortbay.io.Buffer;
import org.mortbay.io.Buffers;
+import java.util.concurrent.ConcurrentLinkedQueue;
/* ------------------------------------------------------------ */
/** Abstract Buffer pool.
@@ -32,11 +33,15 @@
private int _requestBufferSize=8*1024;
private int _responseBufferSize=24*1024;
+ // extending ConcurrentLinkedQueue makes it easier to spot the
monitors in a profiling tool
+
+ class AbstractBuffersQueue extends ConcurrentLinkedQueue<Buffer> { }
+
// Use and array of buffers to avoid contention
- private transient ArrayList _headerBuffers=new ArrayList();
+ private transient AbstractBuffersQueue _headerBuffers=new
AbstractBuffersQueue();
protected transient int _loss; - private transient ArrayList _requestBuffers;
- private transient ArrayList _responseBuffers;
+ private transient AbstractBuffersQueue _requestBuffers;
+ private transient AbstractBuffersQueue _responseBuffers;
/* ------------------------------------------------------------ */
/**
@@ -101,31 +106,19 @@
public Buffer getBuffer(int size)
{
if (size==_headerBufferSize)
- {
- synchronized(_headerBuffers)
- { - if (_headerBuffers.size()>0) - return (Buffer) _headerBuffers.remove(_headerBuffers.size()-1); - }
- return newBuffer(size);
+ { + Buffer ret = _headerBuffers.poll(); + return ret == null ? newBuffer(size) : ret; }else if (size==_responseBufferSize)
{ - synchronized(_responseBuffers)
- {
- if (_responseBuffers.size()==0)
- return newBuffer(size);
- return (Buffer)
_responseBuffers.remove(_responseBuffers.size()-1);
- }
+ Buffer ret = _responseBuffers.poll();
+ return ret == null ? newBuffer(size) : ret;
}
else if (size==_requestBufferSize)
{ - synchronized(_requestBuffers)
- {
- if (_requestBuffers.size()==0)
- return newBuffer(size);
- return (Buffer)
_requestBuffers.remove(_requestBuffers.size()-1);
- }
+ Buffer ret = _requestBuffers.poll();
+ return ret == null ? newBuffer(size) : ret;
}
return newBuffer(size);
@@ -141,24 +134,15 @@
int c=buffer.capacity();
if (c==_headerBufferSize)
{
- synchronized(_headerBuffers)
- {
- _headerBuffers.add(buffer);
- }
+ _headerBuffers.offer(buffer);
}
else if (c==_responseBufferSize)
{ - synchronized(_responseBuffers)
- {
- _responseBuffers.add(buffer);
- }
+ _responseBuffers.offer(buffer);
}
else if (c==_requestBufferSize)
{ - synchronized(_requestBuffers)
- {
- _requestBuffers.add(buffer);
- }
+ _requestBuffers.offer(buffer);
{ super.doStart(); - if (_headerBuffers!=null) - _headerBuffers.clear(); - else - _headerBuffers=new ArrayList(); - - if (_requestBuffers!=null) - _requestBuffers.clear(); - else - _requestBuffers=new ArrayList(); + _headerBuffers = new AbstractBuffersQueue(); + + _requestBuffers=new AbstractBuffersQueue(); - if (_responseBuffers!=null) - _responseBuffers.clear(); - else - _responseBuffers=new ArrayList(); + _responseBuffers=new AbstractBuffersQueue(); }
}
}
}
@@ -168,20 +152,11 @@
Activity
Greg,
sorry, I just realized I never committed my updated test case 
I did knock out the timing and syncing for incrementing the time counters. The only sync that was still in place was for incrementing the number of buffers, which may fall foul of what you were saying regardless.
I have some concerns (other than java 1.5 dependency for jetty 6), with
the object creation that concurrent link queue does.
I may investigate applying it's non blocking algorithms to our ArrayQueue
class.