6348631 - request for review

[Volker Simonis]

Hi Ivan,

thank you for this change. Please find some comments inline:

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:)

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.


os_linux.inline.hpp
===================
- replace:

inline void os::dll_unload(void *lib) {
  dlclose(lib);
}

by:

inline void os::dll_unload(void *lib) {
  ::dlclose(lib);
}

This is just a cosmetic change to make it more clear that a global function gets
called.

- replace:

inline size_t os::hpi_read(int fd, void *buf, unsigned int nBytes) {

by

inline size_t os::restartable_read(int fd, void *buf, unsigned int nBytes) {

See comment above.


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!

And as far as I can see, _print_ascii_file() is only called from within
os_linux.ccp. I would therefore suggest to make it static.


line 4293:
----------
you define O_DELETE like this was done src/solaris/hpi/src/system_md.c

The question is, if we really need this 'delete' functionality in the hotspot?

I searched for usages of O_DELETE (or 0x10000) in the 'jdk' space and found
only one in:

jdk/src/share/native/java/util/zip/ZipFile.c:    if (mode &
OPEN_DELETE) flag |= JVM_O_DELETE;

(It must be noted that in the 'jdk' world, 'O_DELETE' is exported as
'JVM_O_DELETE' in 'jvm_md.h'.

I found no usage of this use case in the HotSpot and 'O_DELETE' isn't exported
anyway, so I suggest to remove this use case to simplify the code.

If you decide to keep it anyway, I would reformat the code as follows to make
it more readable:

#ifndef O_DELETE
#define O_DELETE 0x10000
#endif

// Open a file. Unlink the file immediately after open returns
// if the specified oflag has the O_DELETE flag set.
int os::open(const char *path, int oflag, int mode) {

  if (strlen(path) > MAX_PATH - 1) {
    tty->print_cr("File name is too long (maximum allowed length=%d)",
MAX_PATH-1);
    errno = ENAMETOOLONG;
    return -1;
  }


line 4259:
----------
remove the commented out line after you've folded in the code from open64_w()

//fd = open64_w(path, oflag, mode);



os_solaris.cpp
==============

- make:

 _print_ascii_file
 bool isT2_libthread() {

static because they are only used in os_solaris.cpp

- remove 'O_DELETE' code (see os_linux.cpp)


os_solaris.inline.hpp
=====================
- just for consitency, replace:

inline void os::dll_unload(void *lib) { dlclose(lib); }

by:

inline void os::dll_unload(void *lib) { ::dlclose(lib); }


os_windows.cpp
==============
- better define:

#define MAX_INPUT_EVENTS 2000

which is now defined in line 61, somewhere near the place where it is used
(before the function 'static int stdinAvailable(int fd, long *pbytes')


os_windows.hpp
==============
- has only a copyright change, so better don't change it at all!


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()'


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