Re: HttpServer performance issue / improvement

[Robert Engels]

The OCA tag has been removed. So it’s ok to review now. 

> On Apr 8, 2024, at 9:10 AM, Robert Engels  wrote:
> 
> And thanks for the note about HttpClient - ill leave that as is then.
> 
>> On Apr 8, 2024, at 9:06 AM, Robert Engels  wrote:
>> 
>> It’s fairly isolated but I haven’t checked to see how the JDK differs in 
>> jdk8 - but I’m guessing not very much so it should be a trivial port.
>> 
 On Apr 8, 2024, at 9:03 AM, Daniel Fuchs  wrote:
>>> 
>>> Hi Robert,
>>> 
>>> I am waiting for the OCA to be processed before looking
>>> at your PR.
>>> 
>>> FWIW some of the recent tests we have added for the HttpServer
>>> already use the HttpClient - so unless your test/fix does
>>> require HttpURLConnection it is fine to use HttpClient.
>>> 
>>> Were you planning to later propose to backport this change
>>> back to JDK 8?
>>> 
>>> best regards,
>>> 
>>> -- daniel
>>> 
> On 08/04/2024 14:23, Robert Engels wrote:
 I’ll update my PR to remove the nodelay setting from those tests. I am 
 going to change the PR to use UrlConnection rather than HttpClient as well.
>>> 
>>> 

Re: HttpServer performance issue / improvement

[Robert Engels]

And thanks for the note about HttpClient - ill leave that as is then. 

> On Apr 8, 2024, at 9:06 AM, Robert Engels  wrote:
> 
> It’s fairly isolated but I haven’t checked to see how the JDK differs in 
> jdk8 - but I’m guessing not very much so it should be a trivial port.
> 
>> On Apr 8, 2024, at 9:03 AM, Daniel Fuchs  wrote:
>> 
>> Hi Robert,
>> 
>> I am waiting for the OCA to be processed before looking
>> at your PR.
>> 
>> FWIW some of the recent tests we have added for the HttpServer
>> already use the HttpClient - so unless your test/fix does
>> require HttpURLConnection it is fine to use HttpClient.
>> 
>> Were you planning to later propose to backport this change
>> back to JDK 8?
>> 
>> best regards,
>> 
>> -- daniel
>> 
 On 08/04/2024 14:23, Robert Engels wrote:
>>> I’ll update my PR to remove the nodelay setting from those tests. I am 
>>> going to change the PR to use UrlConnection rather than HttpClient as well.
>> 
>> 

Re: HttpServer performance issue / improvement

[Robert Engels]

It’s fairly isolated but I haven’t checked to see how the JDK differs in jdk8 - 
but I’m guessing not very much so it should be a trivial port. 

> On Apr 8, 2024, at 9:03 AM, Daniel Fuchs  wrote:
> 
> Hi Robert,
> 
> I am waiting for the OCA to be processed before looking
> at your PR.
> 
> FWIW some of the recent tests we have added for the HttpServer
> already use the HttpClient - so unless your test/fix does
> require HttpURLConnection it is fine to use HttpClient.
> 
> Were you planning to later propose to backport this change
> back to JDK 8?
> 
> best regards,
> 
> -- daniel
> 
>> On 08/04/2024 14:23, Robert Engels wrote:
>> I’ll update my PR to remove the nodelay setting from those tests. I am going 
>> to change the PR to use UrlConnection rather than HttpClient as well.
> 
> 

Re: HttpServer performance issue / improvement

[Daniel Fuchs]

Hi Robert,

I am waiting for the OCA to be processed before looking
at your PR.

FWIW some of the recent tests we have added for the HttpServer
already use the HttpClient - so unless your test/fix does
require HttpURLConnection it is fine to use HttpClient.

Were you planning to later propose to backport this change
back to JDK 8?

best regards,

-- daniel

On 08/04/2024 14:23, Robert Engels wrote:

I’ll update my PR to remove the nodelay setting from those tests. I am going to 
change the PR to use UrlConnection rather than HttpClient as well.

Re: HttpServer performance issue / improvement

[Robert Engels]

I’ll update my PR to remove the nodelay setting from those tests. I am going to 
change the PR to use UrlConnection rather than HttpClient as well.  

> On Apr 8, 2024, at 7:10 AM, Robert Engels  wrote:
> 
> I added a new test specifically to address this issue. Ran fine on osx 
> without changes - long duration (failure) on Linux without the fix.
> 
>> On Apr 8, 2024, at 4:01 AM, Jaikiran Pai  wrote:
>> 
>> Recently on AIX the same change was needed to address the test time outs in 
>> the test/jdk/com/sun/net/httpserver/simpleserver/StressDirListings.java test 
>> - https://github.com/openjdk/jdk/pull/17363
>> 
>> -Jaikiran
>> 
 On 08/04/24 1:50 pm, Daniel Jeliński wrote:
>>> Hi Robert,
>>> If you are on Linux, see test/jdk/java/net/vthread/HttpALot.java. That
>>> test was recently modified to add -Dsun.net.httpserver.nodelay=true
>>> [1], because it took forever to complete on LInux machines. On other
>>> OSes the problem is not visible in existing tests.
>>> Cheers,
>>> Daniel
>>> 
>>> [1] https://github.com/openjdk/jdk/pull/10504
>>> 
>>> sob., 6 kwi 2024 o 00:37 robert engels  napisał(a):
 Hi Daniel,
 
 I think I have a solution that would work. I will try to get a PR 
 together. Do you know if there is an existing test case the demonstrates 
 the issue? - if not, I will start with that.
 
 Robert
 
> On Apr 4, 2024, at 9:44 AM, Daniel Jeliński  wrote:
 
 Hi Robert,
 Thanks for bringing this up! We are aware of the issue, it's tracked under 
 https://bugs.openjdk.org/browse/JDK-6968351.
 
 If you have an idea for a proper fix that doesn't add too much complexity, 
 please open a PR, and we'll be happy to help.
 Cheers,
 Daniel
 
 czw., 4 kwi 2024, 14:55 użytkownik Robert Engels  
 napisał:
> 
> When doing some testing on github.com/robaho/httpserver - which is a 
> fork of the jdk http server, I discovered a significant performance issue.
> 
> When an http connection is in ‘keep-alive’ - the default for http 1.1 - 
> the headers are “flushed” here 
> https://github.com/openjdk/jdk21/blob/890adb6410dab4606a4f26a942aed02fb2f55387/src/jdk.httpserver/share/classes/sun/net/httpserver/ExchangeImpl.java#L281
> 
> This means that after the handler runs and it sends data - e.g. /hello 
> sends “hello” on the connection, the connection will stall due to the 
> Nagel algorithm - usually incurring a 50 ms delay. The stall occurs since 
> the client will not see the expected data until after the delay, so it is 
> unable to send the next (when reusing the same connection/HttpClient).
> 
> You can set the TCP_NODELAY on the server to work-around this, but a 
> better solution would be to override the flush() on the 
> BufferedOutputStream to not flush() the underlying connection - i.e. only 
> write the buffered bytes, or rework it a bit to only flush when there is 
> no content to send.
> 

Re: HttpServer performance issue / improvement

[Robert Engels]

I added a new test specifically to address this issue. Ran fine on osx without 
changes - long duration (failure) on Linux without the fix. 

> On Apr 8, 2024, at 4:01 AM, Jaikiran Pai  wrote:
> 
> Recently on AIX the same change was needed to address the test time outs in 
> the test/jdk/com/sun/net/httpserver/simpleserver/StressDirListings.java test 
> - https://github.com/openjdk/jdk/pull/17363
> 
> -Jaikiran
> 
>> On 08/04/24 1:50 pm, Daniel Jeliński wrote:
>> Hi Robert,
>> If you are on Linux, see test/jdk/java/net/vthread/HttpALot.java. That
>> test was recently modified to add -Dsun.net.httpserver.nodelay=true
>> [1], because it took forever to complete on LInux machines. On other
>> OSes the problem is not visible in existing tests.
>> Cheers,
>> Daniel
>> 
>> [1] https://github.com/openjdk/jdk/pull/10504
>> 
>> sob., 6 kwi 2024 o 00:37 robert engels  napisał(a):
>>> Hi Daniel,
>>> 
>>> I think I have a solution that would work. I will try to get a PR together. 
>>> Do you know if there is an existing test case the demonstrates the issue? - 
>>> if not, I will start with that.
>>> 
>>> Robert
>>> 
 On Apr 4, 2024, at 9:44 AM, Daniel Jeliński  wrote:
>>> 
>>> Hi Robert,
>>> Thanks for bringing this up! We are aware of the issue, it's tracked under 
>>> https://bugs.openjdk.org/browse/JDK-6968351.
>>> 
>>> If you have an idea for a proper fix that doesn't add too much complexity, 
>>> please open a PR, and we'll be happy to help.
>>> Cheers,
>>> Daniel
>>> 
>>> czw., 4 kwi 2024, 14:55 użytkownik Robert Engels  
>>> napisał:
 
 When doing some testing on github.com/robaho/httpserver - which is a fork 
 of the jdk http server, I discovered a significant performance issue.
 
 When an http connection is in ‘keep-alive’ - the default for http 1.1 - 
 the headers are “flushed” here 
 https://github.com/openjdk/jdk21/blob/890adb6410dab4606a4f26a942aed02fb2f55387/src/jdk.httpserver/share/classes/sun/net/httpserver/ExchangeImpl.java#L281
 
 This means that after the handler runs and it sends data - e.g. /hello 
 sends “hello” on the connection, the connection will stall due to the 
 Nagel algorithm - usually incurring a 50 ms delay. The stall occurs since 
 the client will not see the expected data until after the delay, so it is 
 unable to send the next (when reusing the same connection/HttpClient).
 
 You can set the TCP_NODELAY on the server to work-around this, but a 
 better solution would be to override the flush() on the 
 BufferedOutputStream to not flush() the underlying connection - i.e. only 
 write the buffered bytes, or rework it a bit to only flush when there is 
 no content to send.
 

Re: HttpServer performance issue / improvement

[Jaikiran Pai]

Recently on AIX the same change was needed to address the test time outs 
in the 
test/jdk/com/sun/net/httpserver/simpleserver/StressDirListings.java test 
- https://github.com/openjdk/jdk/pull/17363


-Jaikiran

On 08/04/24 1:50 pm, Daniel Jeliński wrote:

Hi Robert,
If you are on Linux, see test/jdk/java/net/vthread/HttpALot.java. That
test was recently modified to add -Dsun.net.httpserver.nodelay=true
[1], because it took forever to complete on LInux machines. On other
OSes the problem is not visible in existing tests.
Cheers,
Daniel

[1] https://github.com/openjdk/jdk/pull/10504

sob., 6 kwi 2024 o 00:37 robert engels  napisał(a):

Hi Daniel,

I think I have a solution that would work. I will try to get a PR together. Do 
you know if there is an existing test case the demonstrates the issue? - if 
not, I will start with that.

Robert

On Apr 4, 2024, at 9:44 AM, Daniel Jeliński  wrote:

Hi Robert,
Thanks for bringing this up! We are aware of the issue, it's tracked under 
https://bugs.openjdk.org/browse/JDK-6968351.

If you have an idea for a proper fix that doesn't add too much complexity, 
please open a PR, and we'll be happy to help.
Cheers,
Daniel

czw., 4 kwi 2024, 14:55 użytkownik Robert Engels  
napisał:


When doing some testing on github.com/robaho/httpserver - which is a fork of 
the jdk http server, I discovered a significant performance issue.

When an http connection is in ‘keep-alive’ - the default for http 1.1 - the 
headers are “flushed” here 
https://github.com/openjdk/jdk21/blob/890adb6410dab4606a4f26a942aed02fb2f55387/src/jdk.httpserver/share/classes/sun/net/httpserver/ExchangeImpl.java#L281

This means that after the handler runs and it sends data - e.g. /hello sends 
“hello” on the connection, the connection will stall due to the Nagel algorithm 
- usually incurring a 50 ms delay. The stall occurs since the client will not 
see the expected data until after the delay, so it is unable to send the next 
(when reusing the same connection/HttpClient).

You can set the TCP_NODELAY on the server to work-around this, but a better 
solution would be to override the flush() on the BufferedOutputStream to not 
flush() the underlying connection - i.e. only write the buffered bytes, or 
rework it a bit to only flush when there is no content to send.

Re: HttpServer performance issue / improvement

[Daniel Jeliński]

Hi Robert,
If you are on Linux, see test/jdk/java/net/vthread/HttpALot.java. That
test was recently modified to add -Dsun.net.httpserver.nodelay=true
[1], because it took forever to complete on LInux machines. On other
OSes the problem is not visible in existing tests.
Cheers,
Daniel

[1] https://github.com/openjdk/jdk/pull/10504

sob., 6 kwi 2024 o 00:37 robert engels  napisał(a):
>
> Hi Daniel,
>
> I think I have a solution that would work. I will try to get a PR together. 
> Do you know if there is an existing test case the demonstrates the issue? - 
> if not, I will start with that.
>
> Robert
>
> On Apr 4, 2024, at 9:44 AM, Daniel Jeliński  wrote:
>
> Hi Robert,
> Thanks for bringing this up! We are aware of the issue, it's tracked under 
> https://bugs.openjdk.org/browse/JDK-6968351.
>
> If you have an idea for a proper fix that doesn't add too much complexity, 
> please open a PR, and we'll be happy to help.
> Cheers,
> Daniel
>
> czw., 4 kwi 2024, 14:55 użytkownik Robert Engels  
> napisał:
>>
>> 
>> When doing some testing on github.com/robaho/httpserver - which is a fork 
>> of the jdk http server, I discovered a significant performance issue.
>>
>> When an http connection is in ‘keep-alive’ - the default for http 1.1 - the 
>> headers are “flushed” here 
>> https://github.com/openjdk/jdk21/blob/890adb6410dab4606a4f26a942aed02fb2f55387/src/jdk.httpserver/share/classes/sun/net/httpserver/ExchangeImpl.java#L281
>>
>> This means that after the handler runs and it sends data - e.g. /hello sends 
>> “hello” on the connection, the connection will stall due to the Nagel 
>> algorithm - usually incurring a 50 ms delay. The stall occurs since the 
>> client will not see the expected data until after the delay, so it is unable 
>> to send the next (when reusing the same connection/HttpClient).
>>
>> You can set the TCP_NODELAY on the server to work-around this, but a better 
>> solution would be to override the flush() on the BufferedOutputStream to not 
>> flush() the underlying connection - i.e. only write the buffered bytes, or 
>> rework it a bit to only flush when there is no content to send.
>>
>

Re: HttpServer performance issue / improvement

[robert engels]

Sorry, “it’s” proceeding now. Says could be 1-2 weeks but usually 1-2 business 
days.

> On Apr 7, 2024, at 9:40 AM, robert engels  wrote:
> 
> I proceeding now. I guess when/if the OCA is approved someone will check it 
> out.
> 
>> On Apr 7, 2024, at 5:45 AM, Robert Engels  wrote:
>> 
>> I filled out the OCA. It may be in a pending review status?
>> 
>> 
>>> On Apr 7, 2024, at 1:38 AM, Alan Bateman  wrote:
>>> 
>>> 
>>> 
 On 07/04/2024 00:37, robert engels wrote:
 Hi Daniel,
 
 Here is a PR which fixes it.
>>> 
>>> I don't think anyone will be able to look at this PR until the "oca" label 
>>> is removed. Look for the comment that the bot added about signing the OCA 
>>> for instructions.
>>> 
>>> -Alan
> 

Re: HttpServer performance issue / improvement

[robert engels]

I proceeding now. I guess when/if the OCA is approved someone will check it out.

> On Apr 7, 2024, at 5:45 AM, Robert Engels  wrote:
> 
> I filled out the OCA. It may be in a pending review status?
> 
> 
>> On Apr 7, 2024, at 1:38 AM, Alan Bateman  wrote:
>> 
>> 
>> 
>>> On 07/04/2024 00:37, robert engels wrote:
>>> Hi Daniel,
>>> 
>>> Here is a PR which fixes it.
>> 
>> I don't think anyone will be able to look at this PR until the "oca" label 
>> is removed. Look for the comment that the bot added about signing the OCA 
>> for instructions.
>> 
>> -Alan

Re: HttpServer performance issue / improvement

[Robert Engels]

I filled out the OCA. It may be in a pending review status?


> On Apr 7, 2024, at 1:38 AM, Alan Bateman  wrote:
> 
> 
> 
>> On 07/04/2024 00:37, robert engels wrote:
>> Hi Daniel,
>> 
>> Here is a PR which fixes it.
> 
> I don't think anyone will be able to look at this PR until the "oca" label is 
> removed. Look for the comment that the bot added about signing the OCA for 
> instructions.
> 
> -Alan

Re: HttpServer performance issue / improvement

[Alan Bateman]

On 07/04/2024 00:37, robert engels wrote:

Hi Daniel,

Here is a PR which fixes it.


I don't think anyone will be able to look at this PR until the "oca" 
label is removed. Look for the comment that the bot added about signing 
the OCA for instructions.


-Alan

Re: HttpServer performance issue / improvement

[robert engels]

Hi Daniel,

I think I have a solution that would work. I will try to get a PR together. Do 
you know if there is an existing test case the demonstrates the issue? - if 
not, I will start with that.

Robert

> On Apr 4, 2024, at 9:44 AM, Daniel Jeliński  wrote:
> 
> Hi Robert,
> Thanks for bringing this up! We are aware of the issue, it's tracked under 
> https://bugs.openjdk.org/browse/JDK-6968351.
> 
> If you have an idea for a proper fix that doesn't add too much complexity, 
> please open a PR, and we'll be happy to help.
> Cheers,
> Daniel
> 
> czw., 4 kwi 2024, 14:55 użytkownik Robert Engels  > napisał:
>> 
>> When doing some testing on github.com/robaho/httpserver 
>>  - which is a fork of the jdk http 
>> server, I discovered a significant performance issue.
>> 
>> When an http connection is in ‘keep-alive’ - the default for http 1.1 - the 
>> headers are “flushed” here 
>> https://github.com/openjdk/jdk21/blob/890adb6410dab4606a4f26a942aed02fb2f55387/src/jdk.httpserver/share/classes/sun/net/httpserver/ExchangeImpl.java#L281
>> 
>> This means that after the handler runs and it sends data - e.g. /hello sends 
>> “hello” on the connection, the connection will stall due to the Nagel 
>> algorithm - usually incurring a 50 ms delay. The stall occurs since the 
>> client will not see the expected data until after the delay, so it is unable 
>> to send the next (when reusing the same connection/HttpClient).
>> 
>> You can set the TCP_NODELAY on the server to work-around this, but a better 
>> solution would be to override the flush() on the BufferedOutputStream to not 
>> flush() the underlying connection - i.e. only write the buffered bytes, or 
>> rework it a bit to only flush when there is no content to send.
>> 

Re: HttpServer performance issue / improvement

[Daniel Jeliński]

Hi Robert,
Thanks for bringing this up! We are aware of the issue, it's tracked under
https://bugs.openjdk.org/browse/JDK-6968351.

If you have an idea for a proper fix that doesn't add too much complexity,
please open a PR, and we'll be happy to help.
Cheers,
Daniel

czw., 4 kwi 2024, 14:55 użytkownik Robert Engels 
napisał:

> 
> When doing some testing on github.com/robaho/httpserver - which is a
> fork of the jdk http server, I discovered a significant performance issue.
>
> When an http connection is in ‘keep-alive’ - the default for http 1.1 -
> the headers are “flushed” here
> https://github.com/openjdk/jdk21/blob/890adb6410dab4606a4f26a942aed02fb2f55387/src/jdk.httpserver/share/classes/sun/net/httpserver/ExchangeImpl.java#L281
>
> This means that after the handler runs and it sends data - e.g. /hello
> sends “hello” on the connection, the connection will stall due to the Nagel
> algorithm - usually incurring a 50 ms delay. The stall occurs since the
> client will not see the expected data until after the delay, so it is
> unable to send the next (when reusing the same connection/HttpClient).
>
> You can set the TCP_NODELAY on the server to work-around this, but a
> better solution would be to override the flush() on the
> BufferedOutputStream to not flush() the underlying connection - i.e. only
> write the buffered bytes, or rework it a bit to only flush when there is no
> content to send.
>
>