6348631 - request for review

[Volker Simonis]

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