Re: cvs commit: jakarta-tomcat/src/share/org/apache/tomcat/modules/server Ajp13.java

[Dan Milstein <da...@shore.net>]

Costin,

It's great to see you cleaning up Ajp13, performance-wise (and I'm glad the
comments/code cleanup have helped).

I do see something which seems like a major gotcha, though -- you remove a
lot of String creation by creating MessageBytes objects which point directly
into the underlying input buffer.  *However*, if there is any data posted as
part of the request, that will immediately overwrite the input buffer (l.
376), and any further data which is input will also overwrite the input
buffer (refillReadBuffer()).

Unless I'm missing something, this is going to result in the delayed string
creation creating the wrong string if the post data overwrites the
underlying input buffer.

You could solve this by devoting a buffer to just that initial header info
-- inHeadBuff, and then using a separate buffer for reading input.  Of
course, then you're adding an 8K buffer per Ajp13 thread.  I had thought at
one point that you could use a single buffer for input and output, but
that's not the case -- the servlet can be in the middle of reading its way
through the input stream when it starts to send output back to the browser.

Is there a reason you don't want to use your new hBuf Ajp13Packet for
general output as well as headers?  It kind of looks like you could merge it
with outBuf (since, even when it's built around an OutputBuffer, it still
supports appendBytes(), which is all that doWrite() needs).  If you did
this, you could get rid of one of those 8K buffers per thread.

-Dan

costin@apache.org wrote:
> 
> costin      01/02/26 19:02:47
> 
>   Modified:    src/share/org/apache/tomcat/modules/server Ajp13.java
>   Log:
>   Apply the same recycling as in Http connector.
>   - no Strings ( use MessageBytes )
>   - use OutputBuffer instead of getBytes ( getBytes creates a stream and
>   few buffers )
> 
>   Nothing else is changed - the only "trick" is appendString(), where
>   the buffer is shared between the OutputBuffer and Ajp13Packet
>   ( each adding bytes ). Probably having Ajp13Packet extend OutputBuffer
>   would be cleaner - or even better, turning Ajp13Packet in a better
>   marshaling tool.
> 
>   Revision  Changes    Path
>   1.15      +100 -33   jakarta-tomcat/src/share/org/apache/tomcat/modules/server/Ajp13.java
> 
>   Index: Ajp13.java

-- 

Dan Milstein // danmil@shore.net

Re: cvs commit: jakarta-tomcat/src/share/org/apache/tomcat/modules/server Ajp13.java

[cm...@yahoo.com]

Hi Dan,

> I do see something which seems like a major gotcha, though -- you remove a
> lot of String creation by creating MessageBytes objects which point directly
> into the underlying input buffer.  *However*, if there is any data posted as
> part of the request, that will immediately overwrite the input buffer (l.
> 376), and any further data which is input will also overwrite the input
> buffer (refillReadBuffer()).

That's why it's great to have the patches reviewed :-)

Yes, you are right.

I don't think it's a problem to add a 8K buffer per Ajp13 thread - as long
as the memory is constant and doesn't result in garbage ( the
getBytes() method is creating a buffer that is garbage-collected )

I'll make sure a different buffer is used for the body.

> course, then you're adding an 8K buffer per Ajp13 thread.  I had thought at
> one point that you could use a single buffer for input and output, but
> that's not the case -- the servlet can be in the middle of reading its way
> through the input stream when it starts to send output back to the browser.

True - using the same buffer for in/out was an idea ( to reduce memory
usage - the fact that GC is taking a lot of time was clear for a long
time), but it turned that the trick is to keep the memory constant and to
not generate garbage.


> Is there a reason you don't want to use your new hBuf Ajp13Packet for
> general output as well as headers?  It kind of looks like you could merge it
> with outBuf (since, even when it's built around an OutputBuffer, it still
> supports appendBytes(), which is all that doWrite() needs).  If you did
> this, you could get rid of one of those 8K buffers per thread.

I guess I do it the wrong way - instead of using 2 buffers for input,
where I should have, I got 2 buffers for output - where it wasn't needed. 
Well, I knew I need 2 buffers :-)

Thank you very much, Dan !

Costin