RE: RFR(XS): 8166584: Remove obsolete utility function NET_ThrowSocketException in windows libnet
Thanks, I pushed: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/2b5229c75e93 Best regards Christoph > -Original Message- > From: Chris Hegarty [mailto:chris.hega...@oracle.com] > Sent: Dienstag, 27. September 2016 21:24 > To: Langer, Christoph <christoph.lan...@sap.com> > Cc: net-dev@openjdk.java.net > Subject: Re: RFR(XS): 8166584: Remove obsolete utility function > NET_ThrowSocketException in windows libnet > > > > On 27 Sep 2016, at 19:56, Langer, Christoph <christoph.lan...@sap.com> > wrote: > > > > Chris, > > > > thanks for your input. > > > > If there's no objections I'd push it like this later tomorrow: > > http://cr.openjdk.java.net/~clanger/webrevs/8166584.2/ > > Looks ok to me Christoph. > > Thanks, > -Chris. > > > I've replaced the JNU_JAVANETPKG and JNU_JAVAIOPKG macros with the full > exception class names. > > > > Best regards > > Christoph > > > >> -Original Message- > >> From: Chris Hegarty [mailto:chris.hega...@oracle.com] > >> Sent: Dienstag, 27. September 2016 10:10 > >> To: Langer, Christoph <christoph.lan...@sap.com> > >> Cc: net-dev@openjdk.java.net > >> Subject: Re: RFR(XS): 8166584: Remove obsolete utility function > >> NET_ThrowSocketException in windows libnet > >> > >> Christoph, > >> > >> On 26 Sep 2016, at 18:58, Langer, Christoph <christoph.lan...@sap.com> > >> wrote: > >>> > >>> Hi Chris, > >>> > >>> I agree with your comment on the NPE. It would probably be wrong. So I > >> restored the old code and also removed the comments suggesting the NPE. > >> Here is my new webrev: > >> http://cr.openjdk.java.net/~clanger/webrevs/8166584.1/ > >> > >> This looks fine. > >> > >>> What I was thinking a bit more about after I posted my initial webrev was > the > >> fact that the old NET_ThrowSocketException would register a GlobalRef to > >> java/net/SocketException whereas the other, more generic code would > always > >> use the lookup by name. Would you think it is a performance benefit to keep > a > >> reference to a standard exception class in some place and use it for > throwing > >> or is it better to always look up the class? Throwing those kind of > >> exceptions > is > >> probably not on the hot path anyway - but on the other hand it should be no > >> issue to keep references to these very basic class types. What's your view > >> on > >> that? > >> > >> I don’t believe that using a GlobalRef is worth it here. It adds a little > >> complication, while not offering much benefit. JNU_ThrowByName > >> should be fine. > >> > >>> And another probably aesthetic thing: I notice that sometimes a > >> JNU_JAVANETPKG "SocketException" is used and sometimes a > >> "java/net/SocketException", even within the same file like > SocketInputStream.c. > >> Maybe I should unify this in the files that I touch here and if yes, shall > >> I use > the > >> literal name or the JNU_JAVANETPKG define? Any opinion on that? > >> > >> My preference is to remove JNU_JAVANETPKG, and just use "java/net/“. > >> > >> -Chris > >> > >>> Thanks for taking care of this, > >>> Christoph > >>> > >>> > >>>> -Original Message- > >>>> From: Chris Hegarty [mailto:chris.hega...@oracle.com] > >>>> Sent: Montag, 26. September 2016 16:51 > >>>> To: Langer, Christoph <christoph.lan...@sap.com>; net- > >> d...@openjdk.java.net > >>>> Subject: Re: RFR(XS): 8166584: Remove obsolete utility function > >>>> NET_ThrowSocketException in windows libnet > >>>> > >>>> Christoph, > >>>> > >>>> On 22/09/16 21:59, Langer, Christoph wrote: > >>>>> Hi, > >>>>> > >>>>> while looking at utility functions for creating exceptions in > >>>>> libjava/libnet I found a small spot that should be consolidated right > away. > >>>>> > >>>>> > >>>>> The function NET_ThrowSocketException does only exist in the windows > >>>>> native implementation and is only used in 3 places in > >>>>> SocketInputStream.c. I removed this in favor of directly calling > >>>>> JNU_ThrowByName as the Unix variant of that code already does. > >>>>> > >>>>> > >>>>> In that function Java_java_net_SocketInputStream_socketRead0 I also > >>>>> replaced throwing a SocketException with throwing an NPE in the rare > >>>>> case that a the JNI input for the file descriptor is null. That's > >>>>> probably more natural and should virtually never occur anyways. > >>>> > >>>> Hmmm... I'm not sure about this. SocketException is thrown on > >>>> unix too for a similar situation. More significantly, a null value > >>>> represents that the socket has been, possibly asynchronously, > >>>> closed. > >>>> > >>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8166584 > >>>>> > >>>>> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8166584.0/ > >>>> > >>>> Other than the above concern, the remainder of the code looks ok > >>>> to me. > >>>> > >>>> -Chris. > >
Re: RFR(XS): 8166584: Remove obsolete utility function NET_ThrowSocketException in windows libnet
> On 27 Sep 2016, at 19:56, Langer, Christoph <christoph.lan...@sap.com> wrote: > > Chris, > > thanks for your input. > > If there's no objections I'd push it like this later tomorrow: > http://cr.openjdk.java.net/~clanger/webrevs/8166584.2/ Looks ok to me Christoph. Thanks, -Chris. > I've replaced the JNU_JAVANETPKG and JNU_JAVAIOPKG macros with the full > exception class names. > > Best regards > Christoph > >> -Original Message- >> From: Chris Hegarty [mailto:chris.hega...@oracle.com] >> Sent: Dienstag, 27. September 2016 10:10 >> To: Langer, Christoph <christoph.lan...@sap.com> >> Cc: net-dev@openjdk.java.net >> Subject: Re: RFR(XS): 8166584: Remove obsolete utility function >> NET_ThrowSocketException in windows libnet >> >> Christoph, >> >> On 26 Sep 2016, at 18:58, Langer, Christoph <christoph.lan...@sap.com> >> wrote: >>> >>> Hi Chris, >>> >>> I agree with your comment on the NPE. It would probably be wrong. So I >> restored the old code and also removed the comments suggesting the NPE. >> Here is my new webrev: >> http://cr.openjdk.java.net/~clanger/webrevs/8166584.1/ >> >> This looks fine. >> >>> What I was thinking a bit more about after I posted my initial webrev was >>> the >> fact that the old NET_ThrowSocketException would register a GlobalRef to >> java/net/SocketException whereas the other, more generic code would always >> use the lookup by name. Would you think it is a performance benefit to keep a >> reference to a standard exception class in some place and use it for throwing >> or is it better to always look up the class? Throwing those kind of >> exceptions is >> probably not on the hot path anyway - but on the other hand it should be no >> issue to keep references to these very basic class types. What's your view on >> that? >> >> I don’t believe that using a GlobalRef is worth it here. It adds a little >> complication, while not offering much benefit. JNU_ThrowByName >> should be fine. >> >>> And another probably aesthetic thing: I notice that sometimes a >> JNU_JAVANETPKG "SocketException" is used and sometimes a >> "java/net/SocketException", even within the same file like >> SocketInputStream.c. >> Maybe I should unify this in the files that I touch here and if yes, shall I >> use the >> literal name or the JNU_JAVANETPKG define? Any opinion on that? >> >> My preference is to remove JNU_JAVANETPKG, and just use "java/net/“. >> >> -Chris >> >>> Thanks for taking care of this, >>> Christoph >>> >>> >>>> -Original Message- >>>> From: Chris Hegarty [mailto:chris.hega...@oracle.com] >>>> Sent: Montag, 26. September 2016 16:51 >>>> To: Langer, Christoph <christoph.lan...@sap.com>; net- >> d...@openjdk.java.net >>>> Subject: Re: RFR(XS): 8166584: Remove obsolete utility function >>>> NET_ThrowSocketException in windows libnet >>>> >>>> Christoph, >>>> >>>> On 22/09/16 21:59, Langer, Christoph wrote: >>>>> Hi, >>>>> >>>>> while looking at utility functions for creating exceptions in >>>>> libjava/libnet I found a small spot that should be consolidated right >>>>> away. >>>>> >>>>> >>>>> The function NET_ThrowSocketException does only exist in the windows >>>>> native implementation and is only used in 3 places in >>>>> SocketInputStream.c. I removed this in favor of directly calling >>>>> JNU_ThrowByName as the Unix variant of that code already does. >>>>> >>>>> >>>>> In that function Java_java_net_SocketInputStream_socketRead0 I also >>>>> replaced throwing a SocketException with throwing an NPE in the rare >>>>> case that a the JNI input for the file descriptor is null. That's >>>>> probably more natural and should virtually never occur anyways. >>>> >>>> Hmmm... I'm not sure about this. SocketException is thrown on >>>> unix too for a similar situation. More significantly, a null value >>>> represents that the socket has been, possibly asynchronously, >>>> closed. >>>> >>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8166584 >>>>> >>>>> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8166584.0/ >>>> >>>> Other than the above concern, the remainder of the code looks ok >>>> to me. >>>> >>>> -Chris. >
RE: RFR(XS): 8166584: Remove obsolete utility function NET_ThrowSocketException in windows libnet
Chris, thanks for your input. If there's no objections I'd push it like this later tomorrow: http://cr.openjdk.java.net/~clanger/webrevs/8166584.2/ I've replaced the JNU_JAVANETPKG and JNU_JAVAIOPKG macros with the full exception class names. Best regards Christoph > -Original Message- > From: Chris Hegarty [mailto:chris.hega...@oracle.com] > Sent: Dienstag, 27. September 2016 10:10 > To: Langer, Christoph <christoph.lan...@sap.com> > Cc: net-dev@openjdk.java.net > Subject: Re: RFR(XS): 8166584: Remove obsolete utility function > NET_ThrowSocketException in windows libnet > > Christoph, > > On 26 Sep 2016, at 18:58, Langer, Christoph <christoph.lan...@sap.com> > wrote: > > > > Hi Chris, > > > > I agree with your comment on the NPE. It would probably be wrong. So I > restored the old code and also removed the comments suggesting the NPE. > Here is my new webrev: > http://cr.openjdk.java.net/~clanger/webrevs/8166584.1/ > > This looks fine. > > > What I was thinking a bit more about after I posted my initial webrev was > > the > fact that the old NET_ThrowSocketException would register a GlobalRef to > java/net/SocketException whereas the other, more generic code would always > use the lookup by name. Would you think it is a performance benefit to keep a > reference to a standard exception class in some place and use it for throwing > or is it better to always look up the class? Throwing those kind of > exceptions is > probably not on the hot path anyway - but on the other hand it should be no > issue to keep references to these very basic class types. What's your view on > that? > > I don’t believe that using a GlobalRef is worth it here. It adds a little > complication, while not offering much benefit. JNU_ThrowByName > should be fine. > > > And another probably aesthetic thing: I notice that sometimes a > JNU_JAVANETPKG "SocketException" is used and sometimes a > "java/net/SocketException", even within the same file like > SocketInputStream.c. > Maybe I should unify this in the files that I touch here and if yes, shall I > use the > literal name or the JNU_JAVANETPKG define? Any opinion on that? > > My preference is to remove JNU_JAVANETPKG, and just use "java/net/“. > > -Chris > > > Thanks for taking care of this, > > Christoph > > > > > >> -Original Message----- > >> From: Chris Hegarty [mailto:chris.hega...@oracle.com] > >> Sent: Montag, 26. September 2016 16:51 > >> To: Langer, Christoph <christoph.lan...@sap.com>; net- > d...@openjdk.java.net > >> Subject: Re: RFR(XS): 8166584: Remove obsolete utility function > >> NET_ThrowSocketException in windows libnet > >> > >> Christoph, > >> > >> On 22/09/16 21:59, Langer, Christoph wrote: > >>> Hi, > >>> > >>> while looking at utility functions for creating exceptions in > >>> libjava/libnet I found a small spot that should be consolidated right > >>> away. > >>> > >>> > >>> The function NET_ThrowSocketException does only exist in the windows > >>> native implementation and is only used in 3 places in > >>> SocketInputStream.c. I removed this in favor of directly calling > >>> JNU_ThrowByName as the Unix variant of that code already does. > >>> > >>> > >>> In that function Java_java_net_SocketInputStream_socketRead0 I also > >>> replaced throwing a SocketException with throwing an NPE in the rare > >>> case that a the JNI input for the file descriptor is null. That's > >>> probably more natural and should virtually never occur anyways. > >> > >> Hmmm... I'm not sure about this. SocketException is thrown on > >> unix too for a similar situation. More significantly, a null value > >> represents that the socket has been, possibly asynchronously, > >> closed. > >> > >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8166584 > >>> > >>> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8166584.0/ > >> > >> Other than the above concern, the remainder of the code looks ok > >> to me. > >> > >> -Chris.
Re: RFR(XS): 8166584: Remove obsolete utility function NET_ThrowSocketException in windows libnet
Christoph, On 26 Sep 2016, at 18:58, Langer, Christoph <christoph.lan...@sap.com> wrote: > > Hi Chris, > > I agree with your comment on the NPE. It would probably be wrong. So I > restored the old code and also removed the comments suggesting the NPE. Here > is my new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8166584.1/ This looks fine. > What I was thinking a bit more about after I posted my initial webrev was the > fact that the old NET_ThrowSocketException would register a GlobalRef to > java/net/SocketException whereas the other, more generic code would always > use the lookup by name. Would you think it is a performance benefit to keep a > reference to a standard exception class in some place and use it for throwing > or is it better to always look up the class? Throwing those kind of > exceptions is probably not on the hot path anyway - but on the other hand it > should be no issue to keep references to these very basic class types. What's > your view on that? I don’t believe that using a GlobalRef is worth it here. It adds a little complication, while not offering much benefit. JNU_ThrowByName should be fine. > And another probably aesthetic thing: I notice that sometimes a > JNU_JAVANETPKG "SocketException" is used and sometimes a > "java/net/SocketException", even within the same file like > SocketInputStream.c. Maybe I should unify this in the files that I touch here > and if yes, shall I use the literal name or the JNU_JAVANETPKG define? Any > opinion on that? My preference is to remove JNU_JAVANETPKG, and just use "java/net/“. -Chris > Thanks for taking care of this, > Christoph > > >> -Original Message- >> From: Chris Hegarty [mailto:chris.hega...@oracle.com] >> Sent: Montag, 26. September 2016 16:51 >> To: Langer, Christoph <christoph.lan...@sap.com>; net-dev@openjdk.java.net >> Subject: Re: RFR(XS): 8166584: Remove obsolete utility function >> NET_ThrowSocketException in windows libnet >> >> Christoph, >> >> On 22/09/16 21:59, Langer, Christoph wrote: >>> Hi, >>> >>> while looking at utility functions for creating exceptions in >>> libjava/libnet I found a small spot that should be consolidated right away. >>> >>> >>> The function NET_ThrowSocketException does only exist in the windows >>> native implementation and is only used in 3 places in >>> SocketInputStream.c. I removed this in favor of directly calling >>> JNU_ThrowByName as the Unix variant of that code already does. >>> >>> >>> In that function Java_java_net_SocketInputStream_socketRead0 I also >>> replaced throwing a SocketException with throwing an NPE in the rare >>> case that a the JNI input for the file descriptor is null. That's >>> probably more natural and should virtually never occur anyways. >> >> Hmmm... I'm not sure about this. SocketException is thrown on >> unix too for a similar situation. More significantly, a null value >> represents that the socket has been, possibly asynchronously, >> closed. >> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8166584 >>> >>> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8166584.0/ >> >> Other than the above concern, the remainder of the code looks ok >> to me. >> >> -Chris.
RE: RFR(XS): 8166584: Remove obsolete utility function NET_ThrowSocketException in windows libnet
Hi Chris, I agree with your comment on the NPE. It would probably be wrong. So I restored the old code and also removed the comments suggesting the NPE. Here is my new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8166584.1/ What I was thinking a bit more about after I posted my initial webrev was the fact that the old NET_ThrowSocketException would register a GlobalRef to java/net/SocketException whereas the other, more generic code would always use the lookup by name. Would you think it is a performance benefit to keep a reference to a standard exception class in some place and use it for throwing or is it better to always look up the class? Throwing those kind of exceptions is probably not on the hot path anyway - but on the other hand it should be no issue to keep references to these very basic class types. What's your view on that? And another probably aesthetic thing: I notice that sometimes a JNU_JAVANETPKG "SocketException" is used and sometimes a "java/net/SocketException", even within the same file like SocketInputStream.c. Maybe I should unify this in the files that I touch here and if yes, shall I use the literal name or the JNU_JAVANETPKG define? Any opinion on that? Thanks for taking care of this, Christoph > -Original Message- > From: Chris Hegarty [mailto:chris.hega...@oracle.com] > Sent: Montag, 26. September 2016 16:51 > To: Langer, Christoph <christoph.lan...@sap.com>; net-dev@openjdk.java.net > Subject: Re: RFR(XS): 8166584: Remove obsolete utility function > NET_ThrowSocketException in windows libnet > > Christoph, > > On 22/09/16 21:59, Langer, Christoph wrote: > > Hi, > > > > while looking at utility functions for creating exceptions in > > libjava/libnet I found a small spot that should be consolidated right away. > > > > > > The function NET_ThrowSocketException does only exist in the windows > > native implementation and is only used in 3 places in > > SocketInputStream.c. I removed this in favor of directly calling > > JNU_ThrowByName as the Unix variant of that code already does. > > > > > > In that function Java_java_net_SocketInputStream_socketRead0 I also > > replaced throwing a SocketException with throwing an NPE in the rare > > case that a the JNI input for the file descriptor is null. That's > > probably more natural and should virtually never occur anyways. > > Hmmm... I'm not sure about this. SocketException is thrown on > unix too for a similar situation. More significantly, a null value > represents that the socket has been, possibly asynchronously, > closed. > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8166584 > > > > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8166584.0/ > > Other than the above concern, the remainder of the code looks ok > to me. > > -Chris.
Re: RFR(XS): 8166584: Remove obsolete utility function NET_ThrowSocketException in windows libnet
Christoph, On 22/09/16 21:59, Langer, Christoph wrote: Hi, while looking at utility functions for creating exceptions in libjava/libnet I found a small spot that should be consolidated right away. The function NET_ThrowSocketException does only exist in the windows native implementation and is only used in 3 places in SocketInputStream.c. I removed this in favor of directly calling JNU_ThrowByName as the Unix variant of that code already does. In that function Java_java_net_SocketInputStream_socketRead0 I also replaced throwing a SocketException with throwing an NPE in the rare case that a the JNI input for the file descriptor is null. That’s probably more natural and should virtually never occur anyways. Hmmm... I'm not sure about this. SocketException is thrown on unix too for a similar situation. More significantly, a null value represents that the socket has been, possibly asynchronously, closed. Bug: https://bugs.openjdk.java.net/browse/JDK-8166584 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8166584.0/ Other than the above concern, the remainder of the code looks ok to me. -Chris.
RFR(XS): 8166584: Remove obsolete utility function NET_ThrowSocketException in windows libnet
Hi, while looking at utility functions for creating exceptions in libjava/libnet I found a small spot that should be consolidated right away. The function NET_ThrowSocketException does only exist in the windows native implementation and is only used in 3 places in SocketInputStream.c. I removed this in favor of directly calling JNU_ThrowByName as the Unix variant of that code already does. In that function Java_java_net_SocketInputStream_socketRead0 I also replaced throwing a SocketException with throwing an NPE in the rare case that a the JNI input for the file descriptor is null. That's probably more natural and should virtually never occur anyways. Bug: https://bugs.openjdk.java.net/browse/JDK-8166584 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8166584.0/ Thanks in advance for reviewing :) Best regards Christoph
