Re: PATCH for java.nio FileChannel and MappedByteBuffer

[Per Bothner <per at bothner dot com>]

Michael Koch wrote:

> - You move some files around. That makes it more difficult to detect 
> changes in them. Can you do the move in one patch and the changes to 
> them in another ?

Not easily.  They're moved because of the changes.  The 'map' support
could probably be split out to a later patch.

> - I dont like the inclusion of the natFileChannel{Posix,Win32,Ecos}.cc. 
> We use the old way for a long time and had no real reason to change 
> this. We spoke on IRC about this and nobody seemed to like it too.
> Can you please revert this change ?

I feel strongly that using cpp is a much better mechanism that
symlinks for reasons I've explained.  The only reasons I can think
of for preferring symlinks:
- Symlinks allow aliasing of non-C/C++ files.  That's not a strong
reason (use the appropriate tool for each job), since there is
only one non-.h/-.cc SYMLINK in libjava/configure.in
(ConcreteProcess.java) and that should probably be done differently.
- People like having a separate Posix vs Win32 vs Ecos implementation
file.  However, using symlinks doesn't preclude that - as I do in
my patch.  It allows any degree you like of sharing or separation.
- Symlinks work and people are used to them.  But people are *more*
used to cpp #include; only a small minority of developers know about
how to add to a symlink.

> - I thought about your changes and I still dont think that changing 
> FileDescriptor to utilize FileChannel. As I said before the java.net 
> socket impl classes use a FileDescriptor to store its socket handle.
> It seems logical wrong to put a FileChannel object into FileDescriptor.

Well, they are "equivalent".  They are both Java objects that wrap an
open file descriptor (or Win32 handle).  So given a FileOutputStream,
and the corresponding FileDescriptor and FileChannel, which object
"owns" the file descriptor and is responsible for closing it?  It has
to be only one of them, I think, with the other objects indirecting
via the owning object.  It doesn't make sense for it to be the
FileOutputStream (since you can create a FileOutputStream from a
FileDescriptor).  So either the FileDescriptor or the FileChannel
can "own" the file descriptor, with the other class accessing the
file descriptor from the owning class.

Which is the better way to do it is not obvious.  There might be
some observable difference (in which case we can see how the JDK
behaves).

I mentioned one argument in favor of making FileChannel primary:
Performance is likely to be better for applications that make use
of java.nio, which is what preformance-critical application should
be using.  Another argument is that FileDescriptor is a very
minimal and final class, with only two public methods.  That means
making it primary means implementing private fields plus a slew
of private methods.  And these have to be accessible from FileChannel,
which is awkward.  It makes the logic in FileChannelImpl clumsy,
since it also needs a reference to the user-mode FileOutputStream
etc to do certain operations, at least as currently implemented.

Much more elegant, it seems to me, is to make the FileChannel
primary.  It has a much more complete set of methods, which can
be used to implement the java.io functionality without the
convolutions needed by going the other way.  And it can be
subclassed, so we can have separate FileChannelPosix and
FileChannelWin32 classes, if we want.  The alternative people
have suggested of an implementation-specific stub object seems
to be less efficient:  I'm basically using the FileChannel
in place of the stub class people have suggested.

Sockets do admittedly complicate the situation, and my patch
only hints at a solution.  But note that the FileDescriptor
doesn't actually warp a FileChannel - it wraps a ByteChannel.
So the fd field of a SocketImpl would be a FileDescriptor
whose channel is a SocketChannel instead of a FileChannel.
A SocketImpl would have a private SocketChannel field that
is the same object that is wrapped in its fd field.

I probably need to flesh out the socket implementation before
checking this code in, but feedback would be welcome before then.
--
	--Per Bothner
per@bothner.com   http://per.bothner.com/