6348631 - request for review

[David Holmes]

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.

> 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.

** 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.

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)

> 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.

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/
>>
>>
>>