RFR 8145635 : Add TCP_QUICKACK socket option

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR 8145635 : Add TCP_QUICKACK socket option

Vyom Tewari
Hi All,

Please review my code changes for the below issue.

Bug: JDK-8145635 : Add TCP_QUICKACK socket option

Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.0/index.html

Currently TCP_QUICKACK is only supported on Linux( since Linux 2.4.4) so i implemented is as "ExtendedSocketOption".

Thanks,
Vyom

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Alan Bateman

On 24/02/2016 09:16, vyom wrote:
Hi All,

Please review my code changes for the below issue.

Bug: JDK-8145635 : Add TCP_QUICKACK socket option

Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.0/index.html

Currently TCP_QUICKACK is only supported on Linux( since Linux 2.4.4) so i implemented is as "ExtendedSocketOption".

Is there any urgency on this one? I'm just wondering if we should try to the jdk.net API moved out of java.base before we add more socket options. This is currently tracked as JDK-8044773 and important to get into JDK 9.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Vyom Tewari

Hi All,

As jdk.net API already moved out of java.base, Please review the below code change for jdk10.

Bug: https://bugs.openjdk.java.net/browse/JDK-8145635

Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html

Thanks,

Vyom


On Wednesday 24 February 2016 03:16 PM, Alan Bateman wrote:

On 24/02/2016 09:16, vyom wrote:
Hi All,

Please review my code changes for the below issue.

Bug: JDK-8145635 : Add TCP_QUICKACK socket option

Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.0/index.html

Currently TCP_QUICKACK is only supported on Linux( since Linux 2.4.4) so i implemented is as "ExtendedSocketOption".

Is there any urgency on this one? I'm just wondering if we should try to the jdk.net API moved out of java.base before we add more socket options. This is currently tracked as JDK-8044773 and important to get into JDK 9.

-Alan

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Vyom Tewari

ping!!

Vyom


On Monday 11 September 2017 09:08 PM, vyom tewari wrote:

Hi All,

As jdk.net API already moved out of java.base, Please review the below code change for jdk10.

Bug: https://bugs.openjdk.java.net/browse/JDK-8145635

Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html

Thanks,

Vyom


On Wednesday 24 February 2016 03:16 PM, Alan Bateman wrote:

On 24/02/2016 09:16, vyom wrote:
Hi All,

Please review my code changes for the below issue.

Bug: JDK-8145635 : Add TCP_QUICKACK socket option

Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.0/index.html

Currently TCP_QUICKACK is only supported on Linux( since Linux 2.4.4) so i implemented is as "ExtendedSocketOption".

Is there any urgency on this one? I'm just wondering if we should try to the jdk.net API moved out of java.base before we add more socket options. This is currently tracked as JDK-8044773 and important to get into JDK 9.

-Alan


Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Chris Hegarty
In reply to this post by Vyom Tewari
Vyom,

> On 11 Sep 2017, at 16:38, vyom tewari <[hidden email]> wrote:
>
> Hi All,
>
> As jdk.net API already moved out of java.base, Please review the below code change for jdk10.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
> Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html
>
This looks quite good. Some comments:

1) I wonder if we should just call the option TCP_QUICKACK, rather than SO_QUICKACK? Similar to TCP_NODELAY.

2) I think LinuxSocketOptions.h is largely unnecessary, if we do 1) above.

3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint, rather than jobject, thus avoiding the need for createBoolean. The conversation can happen in the Java layer.  Can you please reduce the lint length in this file, by wrapping similar to the style of the Solaris version.

4) ExtendedSocketOptions.java
  - drop the <p>, they are unnecessary.
  - I think, similar to TCP_NODELAY, the spec should say that the options is TCP specific. From TCP_NODELAY: "The socket option is specific to stream-oriented sockets using the TCP/IP protocol.”
  - "In TCP_QUICKACK mode”, maybe “When the option is enabled…”

-Chris.


Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Vyom Tewari
Hi Chris,

Thanks for review, please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html).
I incorporated review comments from you and re-base the patch to our
consolidated repo(jdk10/master).

Thanks,

Vyom


On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:

> Vyom,
>
>> On 11 Sep 2017, at 16:38, vyom tewari <[hidden email]> wrote:
>>
>> Hi All,
>>
>> As jdk.net API already moved out of java.base, Please review the below code change for jdk10.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
>> Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html
>>
> This looks quite good. Some comments:
>
> 1) I wonder if we should just call the option TCP_QUICKACK, rather than SO_QUICKACK? Similar to TCP_NODELAY.
>
> 2) I think LinuxSocketOptions.h is largely unnecessary, if we do 1) above.
>
> 3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint, rather than jobject, thus avoiding the need for createBoolean. The conversation can happen in the Java layer.  Can you please reduce the lint length in this file, by wrapping similar to the style of the Solaris version.
>
> 4) ExtendedSocketOptions.java
>    - drop the <p>, they are unnecessary.
>    - I think, similar to TCP_NODELAY, the spec should say that the options is TCP specific. From TCP_NODELAY: "The socket option is specific to stream-oriented sockets using the TCP/IP protocol.”
>    - "In TCP_QUICKACK mode”, maybe “When the option is enabled…”
>
> -Chris.
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Chris Hegarty
Vyom,

What about suggestion 1) below, the name of the socket option?

-Chris.

> On 27 Sep 2017, at 09:56, vyom tewari <[hidden email]> wrote:
>
> Hi Chris,
>
> Thanks for review, please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html). I incorporated review comments from you and re-base the patch to our consolidated repo(jdk10/master).
>
> Thanks,
>
> Vyom
>
>
> On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:
>> Vyom,
>>
>>> On 11 Sep 2017, at 16:38, vyom tewari <[hidden email]> wrote:
>>>
>>> Hi All,
>>>
>>> As jdk.net API already moved out of java.base, Please review the below code change for jdk10.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
>>> Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html
>>>
>> This looks quite good. Some comments:
>>
>> 1) I wonder if we should just call the option TCP_QUICKACK, rather than SO_QUICKACK? Similar to TCP_NODELAY.
>>
>> 2) I think LinuxSocketOptions.h is largely unnecessary, if we do 1) above.
>>
>> 3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint, rather than jobject, thus avoiding the need for createBoolean. The conversation can happen in the Java layer.  Can you please reduce the lint length in this file, by wrapping similar to the style of the Solaris version.
>>
>> 4) ExtendedSocketOptions.java
>>   - drop the <p>, they are unnecessary.
>>   - I think, similar to TCP_NODELAY, the spec should say that the options is TCP specific. From TCP_NODELAY: "The socket option is specific to stream-oriented sockets using the TCP/IP protocol.”
>>   - "In TCP_QUICKACK mode”, maybe “When the option is enabled…”
>>
>> -Chris.
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Vyom Tewari
Hi Chris,


On Wednesday 11 October 2017 12:28 AM, Chris Hegarty wrote:
> Vyom,
>
> What about suggestion 1) below, the name of the socket option?
to be consistent with SO_FLOW_SLA in ExtendedSocketOptions.java, i
choose the "SO" prefix. But I don't know the history behind the "SO"
prefix  so i changed the socket option name to "TCP_QUICKACK" as suggested.

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.3/index.html).

Thanks,
Vyom

>
> -Chris.
>
>> On 27 Sep 2017, at 09:56, vyom tewari <[hidden email]> wrote:
>>
>> Hi Chris,
>>
>> Thanks for review, please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html). I incorporated review comments from you and re-base the patch to our consolidated repo(jdk10/master).
>>
>> Thanks,
>>
>> Vyom
>>
>>
>> On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:
>>> Vyom,
>>>
>>>> On 11 Sep 2017, at 16:38, vyom tewari <[hidden email]> wrote:
>>>>
>>>> Hi All,
>>>>
>>>> As jdk.net API already moved out of java.base, Please review the below code change for jdk10.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
>>>> Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html
>>>>
>>> This looks quite good. Some comments:
>>>
>>> 1) I wonder if we should just call the option TCP_QUICKACK, rather than SO_QUICKACK? Similar to TCP_NODELAY.
>>>
>>> 2) I think LinuxSocketOptions.h is largely unnecessary, if we do 1) above.
>>>
>>> 3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint, rather than jobject, thus avoiding the need for createBoolean. The conversation can happen in the Java layer.  Can you please reduce the lint length in this file, by wrapping similar to the style of the Solaris version.
>>>
>>> 4) ExtendedSocketOptions.java
>>>    - drop the <p>, they are unnecessary.
>>>    - I think, similar to TCP_NODELAY, the spec should say that the options is TCP specific. From TCP_NODELAY: "The socket option is specific to stream-oriented sockets using the TCP/IP protocol.”
>>>    - "In TCP_QUICKACK mode”, maybe “When the option is enabled…”
>>>
>>> -Chris.
>>>
>>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Chris Hegarty

> On 11 Oct 2017, at 10:43, vyom tewari <[hidden email]> wrote:
>
> Hi Chris,
>
>
> On Wednesday 11 October 2017 12:28 AM, Chris Hegarty wrote:
>> Vyom,
>>
>> What about suggestion 1) below, the name of the socket option?
> to be consistent with SO_FLOW_SLA in ExtendedSocketOptions.java, i choose the "SO" prefix. But I don't know the history behind the "SO" prefix  so i changed the socket option name to "TCP_QUICKACK" as suggested.

Given that this option is specific to TCP, then the `TCP_` prefix is more appropriate.

-Chris.
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Alan Bateman


On 11/10/2017 21:08, Chris Hegarty wrote:
>
> Given that this option is specific to TCP, then the `TCP_` prefix is more appropriate.
>
I agree. We have StandardSocketOptions.TCP_NODELAY as an example to look at.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Vyom Tewari
In reply to this post by Chris Hegarty
Hi Alan,

thanks for pointing out, i am forwarding it to net-dev list.

Vyom


On Thursday 12 October 2017 03:54 PM, Alan Bateman wrote:

> Best to reply on net-dev as that is where the main review should be
> going on (seems there are at two review threads going, maybe they
> could unite on net-dev).
>
>
> On 12/10/2017 10:01, vyom tewari wrote:
>> Hi Roger,
>>
>> Thanks for the review, i incorporated all review comments from you
>> except("you can use ExtendedSocketOptions.TCP_QUICKACK to check for
>> the option to omit without
>>  embedding the name."). ExtendedSocketOptions.java is part of
>> "jdk.net"  so we can not directly use it in java.base, hence i used
>> the option name to filter "TCP_QUICKACK".
>>
>> Please find the update
>> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.4/index.html).
>>
>> Thanks,
>>
>> Vyom
>>
>>
>> On Wednesday 11 October 2017 08:46 PM, Roger Riggs wrote:
>>> Hi Vyom,
>>>
>>> Comments:
>>>
>>>  - update copyright
>>>  - use @since 18.3 instead of @since 10
>>>
>>> - ExtendedSocketOptions.java: 265,269  include the "TCP_QUICKACK" in
>>> the exception messages.
>>>
>>>     Line 144: if you are going to keep the assert, add a explanation
>>> about how it could happen.
>>>     I'd remove the assert.
>>>
>>>  - The first sentence should be a complete sentence: "TCP_QUICKACK
>>> socket option enables sending TCP/IP acks immediately" or similar.
>>>
>>>  - A reference to the appropriate TCP/IP spec for quickack would be
>>> a good addition. At least the RFC # and section.
>>>  - 85: space after "."  The phrasing is a bit odd in places in the
>>> javadoc.
>>>  - line 81/82 the value is true to enable and false to disable.
>>>  - This phrase is confusing: "it only enables a switch to or from
>>> TCP_QUICKACK mode";
>>>    Since it is set on a socket, it should remain set on that socket
>>> until it is changed.
>>>
>>>  - 203: be consistent in using enable/disable in parameters, etc
>>> even for private methods.
>>>     "on" -> "enable"
>>>
>>> PlainDatagramSocketImpl: 89:
>>>   Why create a new set with zero or one items just to throw it away?
>>>   Use the iterator to add only the non-TCP_QUICKACK options to the
>>> supported options.
>>>  Also, you can use ExtendedSocketOptions.TCP_QUICKACK to check for
>>> the option to omit without
>>>  embedding the name.
>>>
>>>
>>> Sockets.java:
>>>   - The initialization of isQuickAckAvailable might be cleaner as an
>>> nested static class
>>>     that initializes the value in its static initializer. That would
>>> delay the init til needed
>>>     and avoid extra flags.
>>>
>>> LinuxSocketOptions.java:
>>>    - the native methods should be static; since the instance is unused.
>>>
>>>  - line 41: the return type should be Boolean
>>>
>>>  - line 46: the return type of getQuickAck0 should be Boolean like
>>> the argument to set.
>>>
>>>  - line 34:  using JNU_ThrowByNameWithLastError directly is
>>> sufficient; if the errno does not have a string unix supplies
>>> "unknown error nnn".
>>>
>>>
>>> Regards, Roger
>>>
>>> On 10/10/2017 2:58 PM, Chris Hegarty wrote:
>>>> Vyom,
>>>>
>>>> What about suggestion 1) below, the name of the socket option?
>>>>
>>>> -Chris.
>>>>
>>>>> On 27 Sep 2017, at 09:56, vyom tewari <[hidden email]> wrote:
>>>>>
>>>>> Hi Chris,
>>>>>
>>>>> Thanks for review, please find the latest
>>>>> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html).
>>>>> I incorporated review comments from you and re-base the patch to
>>>>> our consolidated repo(jdk10/master).
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Vyom
>>>>>
>>>>>
>>>>> On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:
>>>>>> Vyom,
>>>>>>
>>>>>>> On 11 Sep 2017, at 16:38, vyom tewari <[hidden email]>
>>>>>>> wrote:
>>>>>>>
>>>>>>> Hi All,
>>>>>>>
>>>>>>> As jdk.net API already moved out of java.base, Please review the
>>>>>>> below code change for jdk10.
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html
>>>>>>>
>>>>>> This looks quite good. Some comments:
>>>>>>
>>>>>> 1) I wonder if we should just call the option TCP_QUICKACK,
>>>>>> rather than SO_QUICKACK? Similar to TCP_NODELAY.
>>>>>>
>>>>>> 2) I think LinuxSocketOptions.h is largely unnecessary, if we do
>>>>>> 1) above.
>>>>>>
>>>>>> 3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint,
>>>>>> rather than jobject, thus avoiding the need for createBoolean.
>>>>>> The conversation can happen in the Java layer.  Can you please
>>>>>> reduce the lint length in this file, by wrapping similar to the
>>>>>> style of the Solaris version.
>>>>>>
>>>>>> 4) ExtendedSocketOptions.java
>>>>>>    - drop the <p>, they are unnecessary.
>>>>>>    - I think, similar to TCP_NODELAY, the spec should say that
>>>>>> the options is TCP specific. From TCP_NODELAY: "The socket option
>>>>>> is specific to stream-oriented sockets using the TCP/IP protocol.”
>>>>>>    - "In TCP_QUICKACK mode”, maybe “When the option is enabled…”
>>>>>>
>>>>>> -Chris.
>>>>>>
>>>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Chris Hegarty
In reply to this post by Chris Hegarty
Vyom,

> On 12 Oct 2017, at 10:01, vyom tewari <[hidden email]> wrote:
>
> Hi Roger,
>
> Thanks for the review, i incorporated all review comments from you except("you can use ExtendedSocketOptions.TCP_QUICKACK to check for the option to omit without
>  embedding the name."). ExtendedSocketOptions.java is part of "jdk.net"  so we can not directly use it in java.base, hence i used the option name to filter "TCP_QUICKACK".
>
> Please find the update webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.4/index.html).

This looks much better.

Some suggested wordings of the JDK-specific option:

/**
 * Disable Delayed Acknowledgements.
 *
 * <p> This socket option can be used to reduce or disable delayed
 * acknowledgments (ACKs).
 *
 * <p> The value of this socket option is a {@code Boolean} that represents
 * whether the option is enabled or disabled. The socket option is specific
 * to stream-oriented sockets using the TCP/IP protocol.
 *
 * <p> The exact semantics of this socket option are socket type and system
 * dependent.
 *
 * <p> When TCP_QUICKACK is enabled, ACKs are sent immediately, rather than
 * delayed if needed in accordance to normal TCP operation. This option is
 * not permanent, it only enables a switch to or from TCP_QUICKACK mode.
 *
 * <p> Subsequent operations of the TCP protocol will once again enter/leave
 * TCP_QUICKACK mode depending on internal protocol processing and factors
 * such as delayed ack timeouts occurring and data transfer.
 *
 * @since 18.3
 */

-Chris.

P.S. D’oh, sorry, of course you need the paragraph tags.


> Thanks,
>
> Vyom
>
>
> On Wednesday 11 October 2017 08:46 PM, Roger Riggs wrote:
>> Hi Vyom,
>>
>> Comments:
>>
>>  - update copyright
>>  - use @since 18.3 instead of @since 10
>>
>> - ExtendedSocketOptions.java: 265,269  include the "TCP_QUICKACK" in the exception messages.
>>
>>     Line 144: if you are going to keep the assert, add a explanation about how it could happen.
>>     I'd remove the assert.
>>
>>  - The first sentence should be a complete sentence: "TCP_QUICKACK socket option enables sending TCP/IP acks immediately" or similar.
>>
>>  - A reference to the appropriate TCP/IP spec for quickack would be a good addition. At least the RFC # and section.
>>  - 85: space after "."  The phrasing is a bit odd in places in the javadoc.
>>  - line 81/82 the value is true to enable and false to disable.
>>  - This phrase is confusing: "it only enables a switch to or from TCP_QUICKACK mode";
>>    Since it is set on a socket, it should remain set on that socket until it is changed.
>>
>>  - 203: be consistent in using enable/disable in parameters, etc even for private methods.
>>     "on" -> "enable"
>>
>> PlainDatagramSocketImpl: 89:
>>   Why create a new set with zero or one items just to throw it away?
>>   Use the iterator to add only the non-TCP_QUICKACK options to the supported options.
>>  Also, you can use ExtendedSocketOptions.TCP_QUICKACK to check for the option to omit without
>>  embedding the name.
>>
>>
>> Sockets.java:
>>   - The initialization of isQuickAckAvailable might be cleaner as an nested static class
>>     that initializes the value in its static initializer. That would delay the init til needed
>>     and avoid extra flags.
>>
>> LinuxSocketOptions.java:
>>    - the native methods should be static; since the instance is unused.
>>
>>  - line 41: the return type should be Boolean
>>
>>  - line 46: the return type of getQuickAck0 should be Boolean like the argument to set.
>>
>>  - line 34:  using JNU_ThrowByNameWithLastError directly is sufficient; if the errno does not have a string unix supplies "unknown error nnn".
>>
>>
>> Regards, Roger
>>
>> On 10/10/2017 2:58 PM, Chris Hegarty wrote:
>>> Vyom,
>>>
>>> What about suggestion 1) below, the name of the socket option?
>>>
>>> -Chris.
>>>
>>>> On 27 Sep 2017, at 09:56, vyom tewari <[hidden email]> wrote:
>>>>
>>>> Hi Chris,
>>>>
>>>> Thanks for review, please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html). I incorporated review comments from you and re-base the patch to our consolidated repo(jdk10/master).
>>>>
>>>> Thanks,
>>>>
>>>> Vyom
>>>>
>>>>
>>>> On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:
>>>>> Vyom,
>>>>>
>>>>>> On 11 Sep 2017, at 16:38, vyom tewari <[hidden email]> wrote:
>>>>>>
>>>>>> Hi All,
>>>>>>
>>>>>> As jdk.net API already moved out of java.base, Please review the below code change for jdk10.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
>>>>>> Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html
>>>>>>
>>>>> This looks quite good. Some comments:
>>>>>
>>>>> 1) I wonder if we should just call the option TCP_QUICKACK, rather than SO_QUICKACK? Similar to TCP_NODELAY.
>>>>>
>>>>> 2) I think LinuxSocketOptions.h is largely unnecessary, if we do 1) above.
>>>>>
>>>>> 3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint, rather than jobject, thus avoiding the need for createBoolean. The conversation can happen in the Java layer.  Can you please reduce the lint length in this file, by wrapping similar to the style of the Solaris version.
>>>>>
>>>>> 4) ExtendedSocketOptions.java
>>>>>    - drop the <p>, they are unnecessary.
>>>>>    - I think, similar to TCP_NODELAY, the spec should say that the options is TCP specific. From TCP_NODELAY: "The socket option is specific to stream-oriented sockets using the TCP/IP protocol.”
>>>>>    - "In TCP_QUICKACK mode”, maybe “When the option is enabled…”
>>>>>
>>>>> -Chris.
>>>>>
>>>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Vyom Tewari
Hi Chris,

Thanks for review. Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.5/index.html).

Thanks,

Vyom


On Saturday 14 October 2017 02:25 AM, Chris Hegarty wrote:

> Vyom,
>
>> On 12 Oct 2017, at 10:01, vyom tewari <[hidden email]> wrote:
>>
>> Hi Roger,
>>
>> Thanks for the review, i incorporated all review comments from you except("you can use ExtendedSocketOptions.TCP_QUICKACK to check for the option to omit without
>>   embedding the name."). ExtendedSocketOptions.java is part of "jdk.net"  so we can not directly use it in java.base, hence i used the option name to filter "TCP_QUICKACK".
>>
>> Please find the update webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.4/index.html).
> This looks much better.
>
> Some suggested wordings of the JDK-specific option:
>
> /**
>   * Disable Delayed Acknowledgements.
>   *
>   * <p> This socket option can be used to reduce or disable delayed
>   * acknowledgments (ACKs).
>   *
>   * <p> The value of this socket option is a {@code Boolean} that represents
>   * whether the option is enabled or disabled. The socket option is specific
>   * to stream-oriented sockets using the TCP/IP protocol.
>   *
>   * <p> The exact semantics of this socket option are socket type and system
>   * dependent.
>   *
>   * <p> When TCP_QUICKACK is enabled, ACKs are sent immediately, rather than
>   * delayed if needed in accordance to normal TCP operation. This option is
>   * not permanent, it only enables a switch to or from TCP_QUICKACK mode.
>   *
>   * <p> Subsequent operations of the TCP protocol will once again enter/leave
>   * TCP_QUICKACK mode depending on internal protocol processing and factors
>   * such as delayed ack timeouts occurring and data transfer.
>   *
>   * @since 18.3
>   */
>
> -Chris.
>
> P.S. D’oh, sorry, of course you need the paragraph tags.
>
>
>> Thanks,
>>
>> Vyom
>>
>>
>> On Wednesday 11 October 2017 08:46 PM, Roger Riggs wrote:
>>> Hi Vyom,
>>>
>>> Comments:
>>>
>>>   - update copyright
>>>   - use @since 18.3 instead of @since 10
>>>
>>> - ExtendedSocketOptions.java: 265,269  include the "TCP_QUICKACK" in the exception messages.
>>>
>>>      Line 144: if you are going to keep the assert, add a explanation about how it could happen.
>>>      I'd remove the assert.
>>>
>>>   - The first sentence should be a complete sentence: "TCP_QUICKACK socket option enables sending TCP/IP acks immediately" or similar.
>>>
>>>   - A reference to the appropriate TCP/IP spec for quickack would be a good addition. At least the RFC # and section.
>>>   - 85: space after "."  The phrasing is a bit odd in places in the javadoc.
>>>   - line 81/82 the value is true to enable and false to disable.
>>>   - This phrase is confusing: "it only enables a switch to or from TCP_QUICKACK mode";
>>>     Since it is set on a socket, it should remain set on that socket until it is changed.
>>>
>>>   - 203: be consistent in using enable/disable in parameters, etc even for private methods.
>>>      "on" -> "enable"
>>>
>>> PlainDatagramSocketImpl: 89:
>>>    Why create a new set with zero or one items just to throw it away?
>>>    Use the iterator to add only the non-TCP_QUICKACK options to the supported options.
>>>   Also, you can use ExtendedSocketOptions.TCP_QUICKACK to check for the option to omit without
>>>   embedding the name.
>>>
>>>
>>> Sockets.java:
>>>    - The initialization of isQuickAckAvailable might be cleaner as an nested static class
>>>      that initializes the value in its static initializer. That would delay the init til needed
>>>      and avoid extra flags.
>>>
>>> LinuxSocketOptions.java:
>>>     - the native methods should be static; since the instance is unused.
>>>
>>>   - line 41: the return type should be Boolean
>>>
>>>   - line 46: the return type of getQuickAck0 should be Boolean like the argument to set.
>>>
>>>   - line 34:  using JNU_ThrowByNameWithLastError directly is sufficient; if the errno does not have a string unix supplies "unknown error nnn".
>>>
>>>
>>> Regards, Roger
>>>
>>> On 10/10/2017 2:58 PM, Chris Hegarty wrote:
>>>> Vyom,
>>>>
>>>> What about suggestion 1) below, the name of the socket option?
>>>>
>>>> -Chris.
>>>>
>>>>> On 27 Sep 2017, at 09:56, vyom tewari <[hidden email]> wrote:
>>>>>
>>>>> Hi Chris,
>>>>>
>>>>> Thanks for review, please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html). I incorporated review comments from you and re-base the patch to our consolidated repo(jdk10/master).
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Vyom
>>>>>
>>>>>
>>>>> On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:
>>>>>> Vyom,
>>>>>>
>>>>>>> On 11 Sep 2017, at 16:38, vyom tewari <[hidden email]> wrote:
>>>>>>>
>>>>>>> Hi All,
>>>>>>>
>>>>>>> As jdk.net API already moved out of java.base, Please review the below code change for jdk10.
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
>>>>>>> Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html
>>>>>>>
>>>>>> This looks quite good. Some comments:
>>>>>>
>>>>>> 1) I wonder if we should just call the option TCP_QUICKACK, rather than SO_QUICKACK? Similar to TCP_NODELAY.
>>>>>>
>>>>>> 2) I think LinuxSocketOptions.h is largely unnecessary, if we do 1) above.
>>>>>>
>>>>>> 3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint, rather than jobject, thus avoiding the need for createBoolean. The conversation can happen in the Java layer.  Can you please reduce the lint length in this file, by wrapping similar to the style of the Solaris version.
>>>>>>
>>>>>> 4) ExtendedSocketOptions.java
>>>>>>     - drop the <p>, they are unnecessary.
>>>>>>     - I think, similar to TCP_NODELAY, the spec should say that the options is TCP specific. From TCP_NODELAY: "The socket option is specific to stream-oriented sockets using the TCP/IP protocol.”
>>>>>>     - "In TCP_QUICKACK mode”, maybe “When the option is enabled…”
>>>>>>
>>>>>> -Chris.
>>>>>>
>>>>>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

roger riggs
Hi Vyom,

A few suggestions:

PlainDatagramSocketImpl.java:
  - line 95/96:  I think you can use just forEach, the order version is
not necessary.
     The code will be a bit more readable if the .filter and .forEach
are on a new line and don't wrap.
     You can also remove the extra "(" and ")

  - line 87-94: these are confusing and imply some implicit resetting of
the option.
  - use @since 10
- 209/268: the native setQuickAck method should use boolean as its
argument to enable/disable
   Since enable is a boolean; it does not need "== true'

LinuxSocketOptions.java/c:
   - 52: setQuickAck0 should use boolean for the 2nd argument; (The
native code already does)

Thanks, Roger


On 10/15/17 11:58 PM, vyom tewari wrote:
> Hi Chris,
>
> Thanks for review. Please find the latest
> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.5/index.html).
>
> Thanks,
>
> Vyom
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Vyom Tewari
Hi Roger,

Thanks for the review , please find the updated
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.6/index.html).

Thanks,

Vyom


On Tuesday 17 October 2017 06:35 AM, Roger Riggs wrote:

> Hi Vyom,
>
> A few suggestions:
>
> PlainDatagramSocketImpl.java:
>  - line 95/96:  I think you can use just forEach, the order version is
> not necessary.
>     The code will be a bit more readable if the .filter and .forEach
> are on a new line and don't wrap.
>     You can also remove the extra "(" and ")
>
>  - line 87-94: these are confusing and imply some implicit resetting
> of the option.
>  - use @since 10
> - 209/268: the native setQuickAck method should use boolean as its
> argument to enable/disable
>   Since enable is a boolean; it does not need "== true'
>
> LinuxSocketOptions.java/c:
>   - 52: setQuickAck0 should use boolean for the 2nd argument; (The
> native code already does)
>
> Thanks, Roger
>
>
> On 10/15/17 11:58 PM, vyom tewari wrote:
>> Hi Chris,
>>
>> Thanks for review. Please find the latest
>> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.5/index.html).
>>
>> Thanks,
>>
>> Vyom
>>
>

Reply | Threaded
Open this post in threaded view
|

RE: RFR 8145635 : Add TCP_QUICKACK socket option

Langer, Christoph
Hi Vyom,

I just looked at your latest webrev which in general looks fine to me.  Some minor remarks:

make/lib/Lib-jdk.net.gmk:
- copyright year needs to be updated

src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java:
- private void addExtSocketOptions(...) looks a bit awful concerning its formatting. I suggest something like this:

private void addExtSocketOptions(Set<SocketOption<?>> extOptions,
                                 Set<SocketOption<?>> options) {
    // TCP_QUICKACK is applicable for TCP Sockets only.
    extOptions.stream()
        .filter((option) -> !option.name().equals("TCP_QUICKACK"))
        .forEach((option) -> options.add(option));
}

src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java:
- copyright year needs to be updated
- the equals sign should be placed on line 98 here:

98     public static final SocketOption<Boolean> TCP_QUICKACK
99             = new ExtSocketOption<Boolean>("TCP_QUICKACK", Boolean.class);

Other than that you should consider it reviewed from my end. No need for further webrev...

Best regards
Christoph

> -----Original Message-----
> From: net-dev [mailto:[hidden email]] On Behalf Of
> vyom tewari
> Sent: Dienstag, 17. Oktober 2017 10:37
> To: [hidden email]
> Subject: Re: RFR 8145635 : Add TCP_QUICKACK socket option
>
> Hi Roger,
>
> Thanks for the review , please find the updated
> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.6/index.htm
> l).
>
> Thanks,
>
> Vyom
>
>
> On Tuesday 17 October 2017 06:35 AM, Roger Riggs wrote:
> > Hi Vyom,
> >
> > A few suggestions:
> >
> > PlainDatagramSocketImpl.java:
> >  - line 95/96:  I think you can use just forEach, the order version is
> > not necessary.
> >     The code will be a bit more readable if the .filter and .forEach
> > are on a new line and don't wrap.
> >     You can also remove the extra "(" and ")
> >
> >  - line 87-94: these are confusing and imply some implicit resetting
> > of the option.
> >  - use @since 10
> > - 209/268: the native setQuickAck method should use boolean as its
> > argument to enable/disable
> >   Since enable is a boolean; it does not need "== true'
> >
> > LinuxSocketOptions.java/c:
> >   - 52: setQuickAck0 should use boolean for the 2nd argument; (The
> > native code already does)
> >
> > Thanks, Roger
> >
> >
> > On 10/15/17 11:58 PM, vyom tewari wrote:
> >> Hi Chris,
> >>
> >> Thanks for review. Please find the latest
> >>
> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.5/index.htm
> l).
> >>
> >> Thanks,
> >>
> >> Vyom
> >>
> >

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Vyom Tewari
Hi All,

please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.7/index.html).

Thanks,

Vyom


On Wednesday 18 October 2017 08:21 PM, Langer, Christoph wrote:

> Hi Vyom,
>
> I just looked at your latest webrev which in general looks fine to me.  Some minor remarks:
>
> make/lib/Lib-jdk.net.gmk:
> - copyright year needs to be updated
>
> src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java:
> - private void addExtSocketOptions(...) looks a bit awful concerning its formatting. I suggest something like this:
>
> private void addExtSocketOptions(Set<SocketOption<?>> extOptions,
>                                   Set<SocketOption<?>> options) {
>      // TCP_QUICKACK is applicable for TCP Sockets only.
>      extOptions.stream()
>          .filter((option) -> !option.name().equals("TCP_QUICKACK"))
>          .forEach((option) -> options.add(option));
> }
>
> src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java:
> - copyright year needs to be updated
> - the equals sign should be placed on line 98 here:
>
> 98     public static final SocketOption<Boolean> TCP_QUICKACK
> 99             = new ExtSocketOption<Boolean>("TCP_QUICKACK", Boolean.class);
>
> Other than that you should consider it reviewed from my end. No need for further webrev...
>
> Best regards
> Christoph
>
>> -----Original Message-----
>> From: net-dev [mailto:[hidden email]] On Behalf Of
>> vyom tewari
>> Sent: Dienstag, 17. Oktober 2017 10:37
>> To: [hidden email]
>> Subject: Re: RFR 8145635 : Add TCP_QUICKACK socket option
>>
>> Hi Roger,
>>
>> Thanks for the review , please find the updated
>> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.6/index.htm
>> l).
>>
>> Thanks,
>>
>> Vyom
>>
>>
>> On Tuesday 17 October 2017 06:35 AM, Roger Riggs wrote:
>>> Hi Vyom,
>>>
>>> A few suggestions:
>>>
>>> PlainDatagramSocketImpl.java:
>>>   - line 95/96:  I think you can use just forEach, the order version is
>>> not necessary.
>>>      The code will be a bit more readable if the .filter and .forEach
>>> are on a new line and don't wrap.
>>>      You can also remove the extra "(" and ")
>>>
>>>   - line 87-94: these are confusing and imply some implicit resetting
>>> of the option.
>>>   - use @since 10
>>> - 209/268: the native setQuickAck method should use boolean as its
>>> argument to enable/disable
>>>    Since enable is a boolean; it does not need "== true'
>>>
>>> LinuxSocketOptions.java/c:
>>>    - 52: setQuickAck0 should use boolean for the 2nd argument; (The
>>> native code already does)
>>>
>>> Thanks, Roger
>>>
>>>
>>> On 10/15/17 11:58 PM, vyom tewari wrote:
>>>> Hi Chris,
>>>>
>>>> Thanks for review. Please find the latest
>>>>
>> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.5/index.htm
>> l).
>>>> Thanks,
>>>>
>>>> Vyom
>>>>

Reply | Threaded
Open this post in threaded view
|

RE: RFR 8145635 : Add TCP_QUICKACK socket option

Langer, Christoph
Hi Vyom,

looks good to me.

Best regards
Christoph

> -----Original Message-----
> From: net-dev [mailto:[hidden email]] On Behalf Of
> vyom tewari
> Sent: Donnerstag, 19. Oktober 2017 04:56
> To: [hidden email]
> Subject: Re: RFR 8145635 : Add TCP_QUICKACK socket option
>
> Hi All,
>
> please find the latest
> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.7/index.htm
> l).
>
> Thanks,
>
> Vyom
>
>
> On Wednesday 18 October 2017 08:21 PM, Langer, Christoph wrote:
> > Hi Vyom,
> >
> > I just looked at your latest webrev which in general looks fine to me.  Some
> minor remarks:
> >
> > make/lib/Lib-jdk.net.gmk:
> > - copyright year needs to be updated
> >
> > src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java:
> > - private void addExtSocketOptions(...) looks a bit awful concerning its
> formatting. I suggest something like this:
> >
> > private void addExtSocketOptions(Set<SocketOption<?>> extOptions,
> >                                   Set<SocketOption<?>> options) {
> >      // TCP_QUICKACK is applicable for TCP Sockets only.
> >      extOptions.stream()
> >          .filter((option) -> !option.name().equals("TCP_QUICKACK"))
> >          .forEach((option) -> options.add(option));
> > }
> >
> > src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java:
> > - copyright year needs to be updated
> > - the equals sign should be placed on line 98 here:
> >
> > 98     public static final SocketOption<Boolean> TCP_QUICKACK
> > 99             = new ExtSocketOption<Boolean>("TCP_QUICKACK",
> Boolean.class);
> >
> > Other than that you should consider it reviewed from my end. No need for
> further webrev...
> >
> > Best regards
> > Christoph
> >
> >> -----Original Message-----
> >> From: net-dev [mailto:[hidden email]] On Behalf Of
> >> vyom tewari
> >> Sent: Dienstag, 17. Oktober 2017 10:37
> >> To: [hidden email]
> >> Subject: Re: RFR 8145635 : Add TCP_QUICKACK socket option
> >>
> >> Hi Roger,
> >>
> >> Thanks for the review , please find the updated
> >>
> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.6/index.htm
> >> l).
> >>
> >> Thanks,
> >>
> >> Vyom
> >>
> >>
> >> On Tuesday 17 October 2017 06:35 AM, Roger Riggs wrote:
> >>> Hi Vyom,
> >>>
> >>> A few suggestions:
> >>>
> >>> PlainDatagramSocketImpl.java:
> >>>   - line 95/96:  I think you can use just forEach, the order version is
> >>> not necessary.
> >>>      The code will be a bit more readable if the .filter and .forEach
> >>> are on a new line and don't wrap.
> >>>      You can also remove the extra "(" and ")
> >>>
> >>>   - line 87-94: these are confusing and imply some implicit resetting
> >>> of the option.
> >>>   - use @since 10
> >>> - 209/268: the native setQuickAck method should use boolean as its
> >>> argument to enable/disable
> >>>    Since enable is a boolean; it does not need "== true'
> >>>
> >>> LinuxSocketOptions.java/c:
> >>>    - 52: setQuickAck0 should use boolean for the 2nd argument; (The
> >>> native code already does)
> >>>
> >>> Thanks, Roger
> >>>
> >>>
> >>> On 10/15/17 11:58 PM, vyom tewari wrote:
> >>>> Hi Chris,
> >>>>
> >>>> Thanks for review. Please find the latest
> >>>>
> >>
> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.5/index.htm
> >> l).
> >>>> Thanks,
> >>>>
> >>>> Vyom
> >>>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Chris Hegarty
In reply to this post by Vyom Tewari

> On 19 Oct 2017, at 03:56, vyom tewari <[hidden email]> wrote:
>
> Hi All,
>
> please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.7/index.html).

Thanks Vyom, this update looks good to me.

-Chris.
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8145635 : Add TCP_QUICKACK socket option

Alan Bateman
In reply to this post by Vyom Tewari


On 19/10/2017 03:56, vyom tewari wrote:
> Hi All,
>
> please find the latest
> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.7/index.html).
I can't recall if I brought this up already but we do have an issue to
deprecate-for-removal jdk.net.Sockets? Socket and friends were rev'ed in
Java 9 to add set/getOption methods so I assume the static methods on
jdk.net.Sockets can go away in time.

-Alan
12