6348631 - request for review

[Coleen Phillimore]

Oh, you can ask.  This would make a nice contribution from the openjdk 
community as well (hint, hint).

Coleen

Volker Simonis wrote:
> That would be really great!
> I just didn't dare to ask.. (again:)
>
>
> On Fri, Nov 19, 2010 at 3:54 PM, Coleen Phillimore
> <coleen.phillimore at oracle.com> wrote:
>   
>> 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/
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>