6348631 - request for review

[Ivan Krylov]

David, Volker,

Thanks for your feedbacks. My comments are inlined.
Here is a new webrev with suggested changes
http://cr.openjdk.java.net/~ikrylov/6348631.v2/

Thanks,

Ivan

On 19.11.2010 3:41, David Holmes wrote:
> Hi Volker,
>
> Just exploring a couple of your comments further ...
>
> Volker Simonis said the following on 11/19/10 04:18:
>> os.hpp
>> ======
>>
>> - I would suggest to rename:
>>
>> static size_t hpi_read(int fd, void *buf, unsigned int nBytes);
>>
>> into:
>>
>> static size_t restartable_read(int fd, void *buf, unsigned int nBytes);
>>
>> because this is exactly what it does in contrast to the vanilla 'read()'
>> function. After all this change wants to get rid of HPI, so there's no reason
>> why to still keep this acronym floating around:)
>
> On Linux os::read and os::hpi_read differ by the latter being restartable, but on Solaris they are both "restartable" and both interruptible - but 
> differ in their expectations/requirements as to the state of the thread when called. In that regard keeping the old hpi_read name reflects the fact 
> that this function replaces hpi::read and is only used for that case - whereas using restartable_read would not correctly distinguish the two use 
> cases.

Note that restartable behavior for Solaris will fade away. It will be non-default for jdk7 and presumably unsupported for jdk8.

>
>> line 410ff
>> ----------
>>
>> - I don't really understand the comment:
>>
>> // os::read for calls from non native state
>> // For performance, os::read is only callable from _thread_in_native
>>
>> and this is apparently only applicable to the Solaris version of
>> 'os::read()'. As far as I understand this is need for 'interruptible' IO which
>> is the default on Linux but requires special handling on Solaris. Make this
>> more explicit in the comment and explain the difference to the
>> 'restartable_read()' version below.
>
> Interruptible I/O is only supported on Solaris, not Linux**. On Linux os::read is simply ::read. The issue here is the state of the thread when it 
> calls the function. I agree that Solaris details should not be evident in the os.hpp header.

So I guess it is best to remove this comment from os.hpp as a platform-dependent statement.

>
> ** There are two sides to "interruptible I/O":
>
> 1. How does the syscall respond to a signal delivery
> 2. Does Thread.interrupt cause a signal to be generated
>
> I think #1 is configurable when you install the signal handler. But #2 is only done on Solaris (see os::interrupt).
>
>> os_linux.cpp
>> ============
>>
>> _print_ascii_file()
>> -------------------
>> you replace:
>>   open() -> ::open()
>>   close() -> ::close()
>>
>> BUT
>>
>>   read() -> os::read()
>>
>> Why do you change the behavior for open() while leaving open() and close()
>> unchanged? Notice that _print_ascii_file() called the global 'read()' function
>> before, because it is not a class method of the 'os' class!
>
> I agree. For Linux this change makes no difference as os::read == ::read, but then the change also makes no sense as even Solaris still calls ::read.

Good catch. I did not want to change and behavior, calls to libc should remain calls to libc,

>
> I'd even suggest _print_ascii file could be moved into os.cpp as a private/static member of os (there's way too much duplication in the os_<os>.cpp 
> files)

I would like to leave code refactoring outside of scope of this fix. There are many more places where code refactoring would be needed. Perhaps after 
the coming changes in the build system it will be much easier to perform this work.

>
>> jvm.cpp
>> =======
>> JVM_LEAF(jint, JVM_Read(jint fd, char *buf, jint nbytes))
>>   JVMWrapper2("JVM_Read (0x%x)", fd);
>>
>>   //%note jvm_r6
>>   // not doing restartable read to avoid performance hit
>>   return (jint)os::hpi_read(fd, buf, nbytes);
>> JVM_END
>>
>> - the new comment 'not doing restartable read to avoid performance hit' is
>>   misleading here, because on Linux 'os::hpi_read()' is restartable. That's
>>   exactly the difference between 'read()' and 'hpi_read()' and that's why I
>>   suggested to rename 'hpi_read()' to 'restartable_read()'
>
> As per the above that's only true on Linux. But I agree that the comment is wrong. It is also very unclear why hpi_read needs to be called in this 
> case. Again I suspect it is the state of the thread that affects what happens on Solaris.

I agree that th comment is not reflecting the code below and therefore removing it. I thing at some point I wanted to do os:read there but later 
encountered problems with it.
There was another earlier proposal to do libc read in the class loader but I seem to get problems with that as well therefore removed from this fix.

>
> Cheers,
> David
> -----
>
>
>> On Thu, Nov 18, 2010 at 8:07 AM, Ivan Krylov <Ivan.Krylov at oracle.com> wrote:
>>> With this fix we are removing the use of the HPI (Host Portable interface)
>>> library from jvm.
>>>
>>> Webrev: http://cr.openjdk.java.net/~ikrylov/6348631/
>>>
>>>
>>>