You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Rémy Maucherat <re...@apache.org> on 2019/11/22 15:25:36 UTC
Drop SocketWrapperBase.write(Non)BlockingDirect methods
Hi,
I'm looking at the endpoint code a lot in preparation for ports to 8.5.
I noticed the write(Non)BlockingDirect methods in particular as they had a
fixme which wondered about the buffering strategy. Given the use, the
thinking seems very legacy optimization oriented, because:
- If the socket buffer is configured as direct, then it is a good idea to
always use it
- The async API is a better API for "direct" writes (where the caller owns
the buffer that is ultimately written to the socket or the SSL engine) with
a clearer contract
- HTTP/2 and websocket don't use them anymore by default, and sendfile also
bypasses them
I have not made a performance study but it's fairly certain the benefit is
zero. As a result, I'm considering removing them (quick experiment: no
particular problem with the testsuite).
Rémy
Re: Drop SocketWrapperBase.write(Non)BlockingDirect methods
Posted by Mark Thomas <ma...@apache.org>.
On 27/11/2019 17:07, Rémy Maucherat wrote:
> On Wed, Nov 27, 2019 at 5:35 PM Mark Thomas <markt@apache.org
> My preference for writing earlier is only a slight one. I'm more
> concerned that different methods take a different approach. Taking a
> closer look at that is on my TODO list but it isn't a priority for me at
> the moment.
>
>
> I'll double check the inconsistency :)
>
> writeBlocking writes after.
> writeNonBlockingInternal has that "&&
> !socketBufferHandler.isWriteBufferWritable()" in the while loop that
> makes the thing inconsistent, it is a leftover from the old code.
> For now I'll fix that.
I was looking at writeBlocking(byte[]...) and writeBlocking(ByteBuffer).
I think writeBlocking(byte[]...) should use while (len > 0) for
consistency shouldn't it?
I think a similar inconsistency exists in the non-blocking write too.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Drop SocketWrapperBase.write(Non)BlockingDirect methods
Posted by Rémy Maucherat <re...@apache.org>.
On Wed, Nov 27, 2019 at 5:35 PM Mark Thomas <ma...@apache.org> wrote:
> On 27/11/2019 15:59, Rémy Maucherat wrote:
> > On Wed, Nov 27, 2019 at 4:51 PM Mark Thomas <markt@apache.org
> > <ma...@apache.org>> wrote:
> >
> > I've been looking at the 9.0.x changes.
> >
> > What is clearer now (I think the differences were always there - just
> > harder to see with the more complex code) is that there are some
> minor
> > differences in behaviour when all of the following are true:
> > - the source buffer is emptied when it is copied to the socket buffer
> > - the socket buffer is full after the source buffer has been copied
> >
> > In some cases there will be an immediate write to the network. In
> other
> > cases the network write will wait until the next write to the socket
> > buffer or the next explicit flush.
> >
> > I think I prefer an immediate write in this scenario. WDYT to
> updating
> > (very carefully) all of the write methods to behave that way?
> >
> >
> > Ah personally I did prefer not writing that full buffer (unless the
> > source still has data, of course) on purpose since I considered not
> > doing IO reduced the likelihood to block on IO (or write 0 for non
> > blocking and having to use the poller). That's possible though if you
> > like it better.
>
> My preference for writing earlier is only a slight one. I'm more
> concerned that different methods take a different approach. Taking a
> closer look at that is on my TODO list but it isn't a priority for me at
> the moment.
>
I'll double check the inconsistency :)
writeBlocking writes after.
writeNonBlockingInternal has that "&&
!socketBufferHandler.isWriteBufferWritable()" in the while loop that makes
the thing inconsistent, it is a leftover from the old code.
For now I'll fix that.
Rémy
Re: Drop SocketWrapperBase.write(Non)BlockingDirect methods
Posted by Mark Thomas <ma...@apache.org>.
On 27/11/2019 15:59, Rémy Maucherat wrote:
> On Wed, Nov 27, 2019 at 4:51 PM Mark Thomas <markt@apache.org
> <ma...@apache.org>> wrote:
>
> I've been looking at the 9.0.x changes.
>
> What is clearer now (I think the differences were always there - just
> harder to see with the more complex code) is that there are some minor
> differences in behaviour when all of the following are true:
> - the source buffer is emptied when it is copied to the socket buffer
> - the socket buffer is full after the source buffer has been copied
>
> In some cases there will be an immediate write to the network. In other
> cases the network write will wait until the next write to the socket
> buffer or the next explicit flush.
>
> I think I prefer an immediate write in this scenario. WDYT to updating
> (very carefully) all of the write methods to behave that way?
>
>
> Ah personally I did prefer not writing that full buffer (unless the
> source still has data, of course) on purpose since I considered not
> doing IO reduced the likelihood to block on IO (or write 0 for non
> blocking and having to use the poller). That's possible though if you
> like it better.
My preference for writing earlier is only a slight one. I'm more
concerned that different methods take a different approach. Taking a
closer look at that is on my TODO list but it isn't a priority for me at
the moment.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Drop SocketWrapperBase.write(Non)BlockingDirect methods
Posted by Rémy Maucherat <re...@apache.org>.
On Wed, Nov 27, 2019 at 4:51 PM Mark Thomas <ma...@apache.org> wrote:
> I've been looking at the 9.0.x changes.
>
> What is clearer now (I think the differences were always there - just
> harder to see with the more complex code) is that there are some minor
> differences in behaviour when all of the following are true:
> - the source buffer is emptied when it is copied to the socket buffer
> - the socket buffer is full after the source buffer has been copied
>
> In some cases there will be an immediate write to the network. In other
> cases the network write will wait until the next write to the socket
> buffer or the next explicit flush.
>
> I think I prefer an immediate write in this scenario. WDYT to updating
> (very carefully) all of the write methods to behave that way?
>
Ah personally I did prefer not writing that full buffer (unless the source
still has data, of course) on purpose since I considered not doing IO
reduced the likelihood to block on IO (or write 0 for non blocking and
having to use the poller). That's possible though if you like it better.
Rémy
Re: Drop SocketWrapperBase.write(Non)BlockingDirect methods
Posted by Mark Thomas <ma...@apache.org>.
On 27/11/2019 12:47, Rémy Maucherat wrote:
> On Tue, Nov 26, 2019 at 2:36 PM Rémy Maucherat <remm@apache.org
> <ma...@apache.org>> wrote:
>
> It looked ok and removed a big chunk of code so I went ahead with it.
>
>
> Status update:
>
>
> - This one, also the easiest one: drop
> SocketWrapperBase.write(Non)BlockingDirect methods
>
>
> Done.
I've been looking at the 9.0.x changes.
What is clearer now (I think the differences were always there - just
harder to see with the more complex code) is that there are some minor
differences in behaviour when all of the following are true:
- the source buffer is emptied when it is copied to the socket buffer
- the socket buffer is full after the source buffer has been copied
In some cases there will be an immediate write to the network. In other
cases the network write will wait until the next write to the socket
buffer or the next explicit flush.
I think I prefer an immediate write in this scenario. WDYT to updating
(very carefully) all of the write methods to behave that way?
Cheers,
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Drop SocketWrapperBase.write(Non)BlockingDirect methods
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, Nov 26, 2019 at 2:36 PM Rémy Maucherat <re...@apache.org> wrote:
> It looked ok and removed a big chunk of code so I went ahead with it.
>
Status update:
> - This one, also the easiest one: drop
> SocketWrapperBase.write(Non)BlockingDirect methods
>
Done.
Updated port list:
- Remove multipoller from the NIO connector.
- Endpoint generics for the connection type (not fully mandatory but
cleaner).
> - Move the processor tracking to the wrapper and the connection map to the
> endpoint; this one can improve performance
> - The pile ups of close refactoring; big cleanup and move close to the
> wrapper, also improves robustness, I would do it last
>
> Rémy
Re: Drop SocketWrapperBase.write(Non)BlockingDirect methods
Posted by Rémy Maucherat <re...@apache.org>.
On Sat, Nov 23, 2019 at 8:54 PM Mark Thomas <ma...@apache.org> wrote:
> On 22/11/2019 15:25, Rémy Maucherat wrote:
> > Hi,
> >
> > I'm looking at the endpoint code a lot in preparation for ports to 8.5.
> >
> > I noticed the write(Non)BlockingDirect methods in particular as they had
> > a fixme which wondered about the buffering strategy. Given the use, the
> > thinking seems very legacy optimization oriented, because:
> > - If the socket buffer is configured as direct, then it is a good idea
> > to always use it
> > - The async API is a better API for "direct" writes (where the caller
> > owns the buffer that is ultimately written to the socket or the SSL
> > engine) with a clearer contract
> > - HTTP/2 and websocket don't use them anymore by default, and sendfile
> > also bypasses them
> >
> > I have not made a performance study but it's fairly certain the benefit
> > is zero. As a result, I'm considering removing them (quick experiment:
> > no particular problem with the testsuite).
>
> My only concern would be stability. With careful review we should be
> able to handle that.
>
It looked ok and removed a big chunk of code so I went ahead with it.
As far as porting goes, I have:
- This one, also the easiest one: drop
SocketWrapperBase.write(Non)BlockingDirect methods
- Move the processor tracking to the wrapper and the connection map to the
endpoint; this one can improve performance
- The pile ups of close refactoring; big cleanup and move close to the
wrapper, also improves robustness, I would do it last
If everything is ported, the two branches should be similar and patch
cherry pick should work better for future fixes.
Rémy
Re: Drop SocketWrapperBase.write(Non)BlockingDirect methods
Posted by Mark Thomas <ma...@apache.org>.
On 22/11/2019 15:25, Rémy Maucherat wrote:
> Hi,
>
> I'm looking at the endpoint code a lot in preparation for ports to 8.5.
>
> I noticed the write(Non)BlockingDirect methods in particular as they had
> a fixme which wondered about the buffering strategy. Given the use, the
> thinking seems very legacy optimization oriented, because:
> - If the socket buffer is configured as direct, then it is a good idea
> to always use it
> - The async API is a better API for "direct" writes (where the caller
> owns the buffer that is ultimately written to the socket or the SSL
> engine) with a clearer contract
> - HTTP/2 and websocket don't use them anymore by default, and sendfile
> also bypasses them
>
> I have not made a performance study but it's fairly certain the benefit
> is zero. As a result, I'm considering removing them (quick experiment:
> no particular problem with the testsuite).
My only concern would be stability. With careful review we should be
able to handle that.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org