Re: RFR 8175814: HttpClient protocol version needs unspecified value

[Daniel Fuchs]

Hi Michael,

On 26/04/2017 16:22, Michael McMahon wrote:

Hi,

This webrev has been updated with a number of additional changes since
the first review.

The latest webrev is at:
http://cr.openjdk.java.net/~michaelm/8175814/webrev.3/index.html


The updates look good to me. Might be good to have another
set of eyes looking at it though.

best regards,

-- daniel



Thanks
Michael


On 06/03/2017, 11:29, Michael McMahon wrote:



On 06/03/2017, 11:12, Chris Hegarty wrote:

On 06/03/17 11:00, Daniel Fuchs wrote:

On 01/03/17 15:40, Michael McMahon wrote:

Hi

Could I get the following JDK 9 change reviewed, please?
In addition to fixing the spec problem around HTTP version,
it fixes an implementation issue with version also, where the
per-request
version (if set) was not being picked up.

http://cr.openjdk.java.net/~michaelm/8175814/webrev.1/index.html


Looks good to me Michael.


+1. I am happy to see the default version changed to HTTP/2.

Could a test for the default spec'ed HTTP/2 version be added.
We typically line up the module names, one per line, in the at
modules tag.


Yes, I'll add a test.

Thanks,
Michael.

-Chris.

Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[Chris Hegarty]

> On 27 Apr 2017, at 05:15, Vyom Tewari  wrote:
> 
> Hi,
> 
> please find the updated 
> webrev(http://cr.openjdk.java.net/~vtewari/8165437/webrev0.7/index.html).

This looks ok to me Vyom, but I think you have misinterpreted my comment...

>> ...
>> 1) src/java.base/unix/native/libnet/PlainSocketImpl.c
>> 
>>L235 jlong nanoTimeout = timeout * NET_NSEC_PER_MSEC;
>> 
>>Can you please move this to the latest block of code that requires it, 
>> i.e..
>>just after L327 if (connect_rv != 0) { …

You seem to have moved this line too far, the declaration should
be at the beginning of the if block, just before the  prevNanoTime
declaration and assignment.  No need for another webrev, you
can just change it before pushing.

-Chris.

RFR [9] 8179392: Fix warnings in the httpclient javadoc

[Chris Hegarty]

A few @param and @return tags have missing descriptions. The
description wording, in a few cases, has just been taken from sibling
method descriptions ( nothing new here ).

http://cr.openjdk.java.net/~chegar/8179392/

-Chris.

Re: RFR [9] 8179392: Fix warnings in the httpclient javadoc

[Daniel Fuchs]

Looks good Chris!

Maybe the copyright year should be updated as well?

cheers,

-- daniel

On 27/04/2017 11:53, Chris Hegarty wrote:

A few @param and @return tags have missing descriptions. The
description wording, in a few cases, has just been taken from sibling
method descriptions ( nothing new here ).

http://cr.openjdk.java.net/~chegar/8179392/

-Chris.

Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[Bernd Eckenfels]

Hello,

It looks to me like using nanoseconds in the NET_Timeout Timeout Parameter 
would remove quite a few conversions. Callsides mostly already have 
timeoutNanoseconds for calculating reminder.

Gruss
Bernd
--
http://bernd.eckenfels.net


From: net-dev 
mailto:net-dev-boun...@openjdk.java.net>> on 
behalf of Thomas Stüfe mailto:thomas.stu...@gmail.com>>
Sent: Monday, April 24, 2017 1:07:52 PM
To: Vyom Tewari
Cc: net-dev
Subject: Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking 
code

Hi Vyom,

sorry for the late response, I had vacation.

Thanks for taking my suggestions! Here some remarks:

---

I looked a little bit closer into the question why JVM_LEAF is used to wrap 
simple little functions like JVM_NanoTime or JVM_CurrentTimeMillis (among 
others). There is no hard technical reason why a function could not just be 
exported from the libjvm.so using JNIEXPORT, in any form one wishes too. (In 
fact, we do this in our jvm port a lot).

However, JVM_NanoTime() and JVM_CurrentTimeMillis() are used as direct native 
implementations for System.currentTimeMillis() and System.nanoTime() (see 
share/native/libjava/System.c), using RegisterNatives(). So, the original 
author saved the glue functions he would have to write otherwise 
(Java_java_lang_System_currentTimeMillis etc).

The comments in System.c indicate that this was done for performance reasons 
(you save one call frame by calling JVM_NanoTime directly).

Because they are used as direct JNI implementations for static java methods in 
System, JVM_NanoTime() and JVM_CurrentTimeMillis() have to carry around JNIEnv* 
and jclass parameters. But they are ignored by the functions - the jclass 
argument is even called "ignored" in jvm.h. And it seems to be perfectly okay 
to call JVM_CurrentTimeMillis() with NULL as JNIEnv* argument, which is done in 
many places in the jdk. Presumably this would be okay too for JVM_NanoTime(), 
so you could at least avoid the new JNIEnv* argument in NET_Timeout and just 
call JVM_NanoTime with NULL as first parameter.

-

That aside, I am not a big fan of the removal of the old NET_Timeout. Before, 
there were two functions, "NET_Timeout" just taking the timeout value, 
"NET_Timeout0" taking a timeout and a start value. You removed the first 
variant and therefore added the many additional JVM_NanoTime() calls at 
NET_Timeout callsites. This makes the code harder to read and also kind of 
exposes the internal implementation of NET_Timeout (namely the fact that 
NET_Timeout uses JVM_NanoTime for time measurement). Could you please let both 
variants live, optionally with better names?

-

Otherwise, I think the change looks good. Thanks for your patience!

Kind Regards, Thomas






On Tue, Apr 18, 2017 at 7:08 AM, Vyom Tewari 
mailto:vyom.tew...@oracle.com>> wrote:

Hi Thomas,

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8165437/webrev0.6/index.html) i 
incorporated all the review comments. Regarding why JVM_NanoTime is defined 
JVM_LEAF i don't know much, but all the functions which are defined in jvm.h 
used some sort of macro(JVM_LEAF,JVM_ENTRY,JVM_ENTRY_NO_ENV etc). You can not 
call "os::javaTimeNanos()" directly as this is not visible, i did a small 
prototype which simply export this without any JVM macro as below.

This prototype did worked but i am not sure if this is right way to do. I need 
some more input, why all jvm.h functions are defined with macro and if it OK to 
defined function as below, maybe some one from JVM team can give some more 
comment on this. I decided not to include this prototype as part of review 
because i am not  sure if this is right way.

Thanks,

Vyom

##

diff -r 26d689c621f6 make/symbols/symbols-unix
--- a/make/symbols/symbols-unixWed Apr 12 19:28:47 2017 -0700
+++ b/make/symbols/symbols-unixTue Apr 18 08:40:25 2017 +0530
@@ -51,6 +51,7 @@
 JVM_CurrentLoadedClass
 JVM_CurrentThread
 JVM_CurrentTimeMillis
+JVM_CurrentTimeNano
 JVM_DefineClass
 JVM_DefineClassWithSource
 JVM_DesiredAssertionStatus
diff -r 26d689c621f6 src/share/vm/prims/jvm.cpp
--- a/src/share/vm/prims/jvm.cppWed Apr 12 19:28:47 2017 -0700
+++ b/src/share/vm/prims/jvm.cppTue Apr 18 08:40:25 2017 +0530
@@ -273,7 +273,11 @@
   JVMWrapper("JVM_NanoTime");
   return os::javaTimeNanos();
 JVM_END
-
+
+long JVM_CurrentTimeNano(){
+return os::javaTimeNanos();
+}
+
 // The function below is actually exposed by jdk.internal.misc.VM and not
 // java.lang.System, but we choose to keep it here so that it stays next
 // to JVM_CurrentTimeMillis and JVM_NanoTime
diff -r 26d689c621f6 src/share/vm/prims/jvm.h
--- a/src/share/vm/prims/jvm.hWed Apr 12 19:28:47 2017 -0700
+++ b/src/share/vm/prims/jvm.hTue Apr 18 08:40:25 2017 +0530
@@ -119,6 +119,9 @@
 JNIEXPORT jlong JNICALL
 JVM_NanoTime(JNIEnv *env, jclass ignored);

+JNIEXPORT jlong
+JVM_CurrentTimeNano();
+
 JN

Re: RFR [9] 8179392: Fix warnings in the httpclient javadoc

[Chris Hegarty]

> On 27 Apr 2017, at 11:56, Daniel Fuchs  wrote:
> 
> Looks good Chris!
> 
> Maybe the copyright year should be updated as well?

Thanks for the review Daniel. I updated the copyright years before
pushing.

-Chris.

> cheers,
> 
> -- daniel
> 
> On 27/04/2017 11:53, Chris Hegarty wrote:
>> A few @param and @return tags have missing descriptions. The
>> description wording, in a few cases, has just been taken from sibling
>> method descriptions ( nothing new here ).
>> 
>> http://cr.openjdk.java.net/~chegar/8179392/
>> 
>> -Chris.
>> 
> 

Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[Vyom Tewari]

Hi,

Even i thought the same, pass nanosecond timeout to "NET_Timeout" but if 
you see the implementation it uses " *int poll(struct pollfd **/fds/*, 
nfds_t */nfds/*, int */timeout/*);*


"  where timeout is in millisecond so we have to do conversion anyway in 
loop if we pass timeout in NS. So there will not be much difference,  
will save just one conversion.


thanks,

Vyom

On Thursday 27 April 2017 04:58 PM, Bernd Eckenfels wrote:

Hello,

It looks to me like using nanoseconds in the NET_Timeout Timeout 
Parameter would remove quite a few conversions. Callsides mostly 
already have timeoutNanoseconds for calculating reminder.


Gruss
Bernd
--
http://bernd.eckenfels.net


*From:* net-dev > on behalf of Thomas Stüfe 
mailto:thomas.stu...@gmail.com>>

*Sent:* Monday, April 24, 2017 1:07:52 PM
*To:* Vyom Tewari
*Cc:* net-dev
*Subject:* Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in 
Networking code

Hi Vyom,

sorry for the late response, I had vacation.

Thanks for taking my suggestions! Here some remarks:

---

I looked a little bit closer into the question why JVM_LEAF is used to 
wrap simple little functions like JVM_NanoTime or 
JVM_CurrentTimeMillis (among others). There is no hard technical 
reason why a function could not just be exported from the libjvm.so 
using JNIEXPORT, in any form one wishes too. (In fact, we do this in 
our jvm port a lot).


However, JVM_NanoTime() and JVM_CurrentTimeMillis() are used as direct 
native implementations for System.currentTimeMillis() and 
System.nanoTime() (see share/native/libjava/System.c), using 
RegisterNatives(). So, the original author saved the glue functions he 
would have to write otherwise (Java_java_lang_System_currentTimeMillis 
etc).


The comments in System.c indicate that this was done for performance 
reasons (you save one call frame by calling JVM_NanoTime directly).


Because they are used as direct JNI implementations for static java 
methods in System, JVM_NanoTime() and JVM_CurrentTimeMillis() have to 
carry around JNIEnv* and jclass parameters. But they are ignored by 
the functions - the jclass argument is even called "ignored" in jvm.h. 
And it seems to be perfectly okay to call JVM_CurrentTimeMillis() with 
NULL as JNIEnv* argument, which is done in many places in the jdk. 
Presumably this would be okay too for JVM_NanoTime(), so you could at 
least avoid the new JNIEnv* argument in NET_Timeout and just call 
JVM_NanoTime with NULL as first parameter.


-

That aside, I am not a big fan of the removal of the old NET_Timeout. 
Before, there were two functions, "NET_Timeout" just taking the 
timeout value, "NET_Timeout0" taking a timeout and a start value. You 
removed the first variant and therefore added the many additional 
JVM_NanoTime() calls at NET_Timeout callsites. This makes the code 
harder to read and also kind of exposes the internal implementation of 
NET_Timeout (namely the fact that NET_Timeout uses JVM_NanoTime for 
time measurement). Could you please let both variants live, optionally 
with better names?


-

Otherwise, I think the change looks good. Thanks for your patience!

Kind Regards, Thomas





On Tue, Apr 18, 2017 at 7:08 AM, Vyom Tewari > wrote:


Hi Thomas,

Thanks for review, please find the updated
webrev(http://cr.openjdk.java.net/~vtewari/8165437/webrev0.6/index.html
)
i incorporated all the review comments. Regarding why JVM_NanoTime
is defined JVM_LEAF i don't know much, but all the functions which
are defined in jvm.h used some sort of
macro(JVM_LEAF,JVM_ENTRY,JVM_ENTRY_NO_ENV etc). You can not call
"os::javaTimeNanos()" directly as this is not visible, i did a
small prototype which simply export this without any JVM macro as
below.

This prototype did worked but i am not sure if this is right way
to do. I need some more input, why all jvm.h functions are defined
with macro and if it OK to defined function as below, maybe some
one from JVM team can give some more comment on this. I decided
not to include this prototype as part of review because i am not
sure if this is right way.

Thanks,

Vyom

##

diff -r 26d689c621f6 make/symbols/symbols-unix
--- a/make/symbols/symbols-unixWed Apr 12 19:28:47 2017 -0700
+++ b/make/symbols/symbols-unixTue Apr 18 08:40:25 2017 +0530
@@ -51,6 +51,7 @@
 JVM_CurrentLoadedClass
 JVM_CurrentThread
 JVM_CurrentTimeMillis
+JVM_CurrentTimeNano
 JVM_DefineClass
 JVM_DefineClassWithSource
 JVM_DesiredAssertionStatus
diff -r 26d689c621f6 src/share/vm/prims/jvm.cpp
--- a/src/share/vm/prims/jvm.cpp   Wed Apr 12 19:28:47 2017 -0700
+++ b/src/share/vm/prims/jvm.cpp   Tue A

Re: RFR 8175814: HttpClient protocol version needs unspecified value

[Chris Hegarty]

> On 27 Apr 2017, at 10:18, Daniel Fuchs  wrote:
> 
> Hi Michael,
> 
> On 26/04/2017 16:22, Michael McMahon wrote:
>> Hi,
>> 
>> This webrev has been updated with a number of additional changes since
>> the first review.
>> 
>> The latest webrev is at:
>> http://cr.openjdk.java.net/~michaelm/8175814/webrev.3/index.html

1) AsyncDataReadQueue

  I think you can ( should ) remove the type param from this class.
  It appears to always deal with Http2Frame types. 

  Is the endItem a settable thing? Seems that it is only ever one
  possibility. L149 can either be removed or reinstated ( with the 
  knock on minor refactoring )

2) Stream

  Remove the extra long line by adding a newline after the arrow:
  
  private final static Supplier endItem = () ->
new DataFrame(0, DataFrame.END_STREAM, new ByteBufferReference[0]);

3) AsyncSSLDelegate typo L264  don’t

4) AsyncConnection / Queue

  I find the term ‘block’ confusing here. It seems that the input channel,
  in the AsyncSSLDelegate implicitly puts itself into “blocking” mode
  when performing the initial handshake. The unblocking happens then
  after successful negotiation of ALPN. Maybe some term to indicate
  no higher-level callback? OR something else.

5) Is the AsyncSSLConnection effectively dead once stopAsyncReading
  is invoke on it? It can only be used to seed another SSLConnection?
  If so, maybe a comment to indicate this.

-Chris.
  
  

Re: RFR 8175814: HttpClient protocol version needs unspecified value

[Michael McMahon]

Hi Chris,

Comments below

On 27/04/2017, 14:32, Chris Hegarty wrote:

On 27 Apr 2017, at 10:18, Daniel Fuchs  wrote:

Hi Michael,

On 26/04/2017 16:22, Michael McMahon wrote:

Hi,

This webrev has been updated with a number of additional changes since
the first review.

The latest webrev is at:
http://cr.openjdk.java.net/~michaelm/8175814/webrev.3/index.html

1) AsyncDataReadQueue

   I think you can ( should ) remove the type param from this class.
   It appears to always deal with Http2Frame types.

I was originally trying to merge this class with Queue, which is generic.
It's something I would like to come back to later, but in the meantime
I can revert that change.

   Is the endItem a settable thing? Seems that it is only ever one
   possibility. L149 can either be removed or reinstated ( with the
   knock on minor refactoring )

Again that was part of the effort to remove the dependency on Http2Frame.
But, I will come back to this later.

2) Stream

   Remove the extra long line by adding a newline after the arrow:

   private final static Supplier  endItem = () ->
 new DataFrame(0, DataFrame.END_STREAM, new ByteBufferReference[0]);

This code will be removed due to change above

3) AsyncSSLDelegate typo L264  don’t

ok

4) AsyncConnection / Queue

   I find the term ‘block’ confusing here. It seems that the input channel,
   in the AsyncSSLDelegate implicitly puts itself into “blocking” mode
   when performing the initial handshake. The unblocking happens then
   after successful negotiation of ALPN. Maybe some term to indicate
   no higher-level callback? OR something else.

As far as Queue is concerned, it either works asynchronously or blocking.
If asynchronous then data is passed on through the callback. If blocking
then data has to be obtained by calling the blocking take() call.

Within AsyncSSLDelegate, the handshake is then done in blocking mode
but when finished it switches to the async mode.

Does that make it any clearer?

5) Is the AsyncSSLConnection effectively dead once stopAsyncReading
   is invoke on it? It can only be used to seed another SSLConnection?
   If so, maybe a comment to indicate this.

Right. I'll add a comment about that.

Thanks
Michael

-Chris.

   

Re: RFR 8175814: HttpClient protocol version needs unspecified value

[Chris Hegarty]

> On 27 Apr 2017, at 16:41, Michael McMahon  
> wrote:
> 
> ...
>> 4) AsyncConnection / Queue
>> 
>>   I find the term ‘block’ confusing here. It seems that the input channel,
>>   in the AsyncSSLDelegate implicitly puts itself into “blocking” mode
>>   when performing the initial handshake. The unblocking happens then
>>   after successful negotiation of ALPN. Maybe some term to indicate
>>   no higher-level callback? OR something else.
> As far as Queue is concerned, it either works asynchronously or blocking.
> If asynchronous then data is passed on through the callback. If blocking
> then data has to be obtained by calling the blocking take() call.
> 
> Within AsyncSSLDelegate, the handshake is then done in blocking mode
> but when finished it switches to the async mode.
> 
> Does that make it any clearer?

Thanks, it does. For my understanding …

The Queue type is basically an unbounded blocking queue. Adding items
will never block, since the queue can grow unbounded. Taking items will
block when the queue becomes empty. There are methods to poll, and to 
push back items to the head. The queue also has a facility to register a
context-free trigger / callback when an item is added to the queue. The
callback has no knowledge of the queue or its internals, it just knows that
an item has been added.

How about block/unblock be renamed disable/enable [Put] Callback/Listener?

I’m less sure what, if anything, AsyncConnection.unblock could be renamed
to, since it has no knowledge of blocking or callbacks in the first place.

I’m just trying to move away from the overloading of “block” in this context.

-Chris.

RE: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[Langer, Christoph]

Hi Vyom,

I’ve just got a small formatting remark about the order of includes:

Generally I tried to follow the rule 1. Common os headers, 2. Platform os 
headers,  3. Jvm/jdk headers, 4. JNI headers in my latest refactorings. So, to 
keep this up, can you move #include “jvm.h” in the line before #include 
“net_util.h” in each file? And pull it before the JNI headers. Like, e.g. in 
net_util_md.c you should move #include "jvm.h" to line 52.

Thanks & Best regards
Christoph

From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of Vyom Tewari
Sent: Donnerstag, 27. April 2017 06:16
To: Thomas Stüfe ; Chris Hegarty 

Cc: net-dev 
Subject: Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking 
code


Hi,

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

Thanks,

Vyom



On Tuesday 25 April 2017 07:34 PM, Thomas Stüfe wrote:
Hi Chris, Vyom,

I have preferences as expressed earlier, but no strong emotions. I can live 
with the fix as it is now.

Thanks all, and Kind Regards, Thomas


On Tue, Apr 25, 2017 at 3:31 PM, Chris Hegarty 
mailto:chris.hega...@oracle.com>> wrote:
> On Tue, Apr 18, 2017 at 7:08 AM, Vyom Tewari 
> mailto:vyom.tew...@oracle.com>> wrote:
> ...
>
> Thanks for review, please find the updated 
> webrev(http://cr.openjdk.java.net/~vtewari/8165437/webrev0.6/index.html)

The changes mainly look good to me, just a few comments:

1) src/java.base/unix/native/libnet/PlainSocketImpl.c

   L235 jlong nanoTimeout = timeout * NET_NSEC_PER_MSEC;

   Can you please move this to the latest block of code that requires it, i.e..
   just after L327 if (connect_rv != 0) { …

2) src/java.base/share/native/libnet/net_util.h

   Should these definitions be moved to 
src/java.base/unix/native/libnet/net_util_md.h?


Regarding JVM_NanoTime being a JVM_LEAF and/or taking a JNIEnv (
both of which are, in todays hotspot VM, effectively ignored ), this is a
separate issue. I have raised it off list with others from the VM team,
without much interest. I will refrain from filing a JIRA issue to track 
potential
changes in the VM for this. Others are welcome to do so, if they wish. I
suggest that we simply continue to pass a valid JNIEnv, since it is not
much extra effort to do so. ( this can be refactored later, if the VM interface
is ever updated ).


> On 24 Apr 2017, at 12:07, Thomas Stüfe 
> mailto:thomas.stu...@gmail.com>> wrote:
>
> ...
> That aside, I am not a big fan of the removal of the old NET_Timeout. Before, 
> there were two functions, "NET_Timeout" just taking the timeout value, 
> "NET_Timeout0" taking a timeout and a start value. You removed the first 
> variant and therefore added the many additional JVM_NanoTime() calls at 
> NET_Timeout callsites. This makes the code harder to read and also kind of 
> exposes the internal implementation of NET_Timeout (namely the fact that 
> NET_Timeout uses JVM_NanoTime for time measurement). Could you please let 
> both variants live, optionally with better names?

I think that it may not be worth added back the simpler variant. It
would only be used by PlainDatagramSocketImpl, correct? All other
usages would use the variant that accepts the current nano time.

-Chris.