6348631 - request for review

[Volker Simonis]

Hi David,

thank you for clarifying the restartable/interruptible stuff. The
Solaris implementation has always drives me crazy when I try to
understand it:)

Regarding your comment about code duplication in os_<os>.cpp. I also
relized this and I wonder if there are any plans to reduce it?

We now have essentially three copies of the old HPI code. The one that
remained in the jdk/ space and two nearly equal version in
os_solaris.cpp and os_linux.cpp. (Not to speak about the poor guys who
also have to support AIX, HPUX, ..).

Are there any plans to at least export this functionality from HptSpot
to the JDK to get at least rid of the HPI implementation in the JDK?

Regards,
Volker

PS: and although I have no real vote here I forgot to mention that the
change looks good otherwise:)

On Fri, Nov 19, 2010 at 1:41 AM, David Holmes <David.Holmes at oracle.com> 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.
>
>> 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/
>>>
>>>
>>>
>