6348631 - request for review

[Coleen Phillimore]

Volker Simonis wrote:
> 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, ..).
>   
One of our ongoing discussions is to add an os/posix/vm directory and 
move the solaris and linux duplicated code there.  This might be helpful 
for AIX and HPUX as well.

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