Re: NIO (Non-block & Scatter/Gather)

[Robert Schuster]

Hi Michael.

Michael Barker wrote:
> Hi,
> 
> My name is Mike Barker.  I am interesting in contributing to the GNU
> Classpath project.
Cool!

>  As a start I have looked the NIO implemenation with
> regards to Channels (specifically SocketChannel).  
Yes, that is an area that definitely needs some care. The current implementation
of the Channel classes reuse java.io classes for their functionality. That is
suboptimal for real NBIO. I am sure there are bug reports and a notes on
http://developer.classpath.org/mediation/ClasspathOpenTasks that explain this in
more detail.

Update: After looking at your patch I see that this is what we really need. :)

> Attached to this mail is a patch that adds support for configuring a
> socket to be non-blocking and uses readv and writev system calls to
> support native scatter/gather operations.  Native access is through a
> VMChannel.java class.  This class could also be added to other Channel
> types to implement native scatter/gather I/O across the board.
This is a good start. :)

> The patch is not quite complete.  It passes the Mauve tests, but
> requires a more comprehensive set of tests (e.g. testing readv/writev
> and different types of ByteBuffer, direct & mapped).  However if what I
> doing looks reasonably sane, I can continue in order to provide a more
> complete patch.  Comments, criticism, etc. is welcome.
No criticism but we need a written copyright assignment from you to be allowed
to accept the patch. Mark will send you a mail as soon as he reads that. :)

Since I am not an expert on the NIO I looked at the patch from a stylistic
perspective. I hope someone with more knowledge in that area (Michael Koch,
where are you?) can give you some technical feedback. As the CA thing will take
some time you can implement the missing features, write more mauve tests,
receive more feedback and meet the classpath hackers in #classpath on
irc.gnu.org in the meantime.

The patch would have been nice for 0.20 but there is no need to hurry. When it
goes in, in some weeks, people can start testing the code on strange
architectures and operating systems. This will lead to a rock solid
implementation for 0.21 :)

Thanks for doing this!

For the nitpicks see below.

> Quick question:
> I understand the info on the Wiki around developer tainting, which is OK
> as I have not studied the Java sources.  What about black-box testing of
> the functionality of the JVM?  Does doing this taint the developer?
No, black box tests are ok.

> Regards,
> Mike Barker.
> 
> 
> ------------------------------------------------------------------------
> 

This file should not be part of the patch.

> Index: .settings/org.eclipse.jdt.core.prefs
> ===================================================================
> RCS file: /cvsroot/classpath/classpath/.settings/org.eclipse.jdt.core.prefs,v
> retrieving revision 1.5
> diff -u -r1.5 org.eclipse.jdt.core.prefs
> --- .settings/org.eclipse.jdt.core.prefs      14 Sep 2005 01:33:27 -0000      
> 1.5
> +++ .settings/org.eclipse.jdt.core.prefs      11 Jan 2006 22:09:39 -0000
> +org.eclipse.jdt.core.formatter.use_tabs_only_for_leading_indentations=false
[...]

The same here.
> Index: .settings/org.eclipse.jdt.ui.prefs
> ===================================================================
> RCS file: /cvsroot/classpath/classpath/.settings/org.eclipse.jdt.ui.prefs,v
> retrieving revision 1.4
> diff -u -r1.4 org.eclipse.jdt.ui.prefs
> --- .settings/org.eclipse.jdt.ui.prefs        18 Sep 2005 04:19:36 -0000      
> 1.4
> +++ .settings/org.eclipse.jdt.ui.prefs        11 Jan 2006 22:09:39 -0000
> @@ -1,4 +1,4 @@
> -#Sat Sep 17 22:04:29 MDT 2005
> +#Thu Dec 22 16:04:10 GMT 2005
>  eclipse.preferences.version=1
>  formatter_settings_version=8
>  internal.default.compliance=user

> Index: gnu/java/nio/SelectorImpl.java
> ===================================================================
> RCS file: /cvsroot/classpath/classpath/gnu/java/nio/SelectorImpl.java,v
> retrieving revision 1.21
> diff -u -r1.21 SelectorImpl.java
> --- gnu/java/nio/SelectorImpl.java    27 Dec 2005 08:32:12 -0000      1.21
> +++ gnu/java/nio/SelectorImpl.java    11 Jan 2006 22:09:41 -0000
> @@ -379,6 +379,8 @@
>        result = new DatagramChannelSelectionKey (ch, this);
>      else if (ch instanceof ServerSocketChannelImpl)
>        result = new ServerSocketChannelSelectionKey (ch, this);
> +    else if (ch instanceof gnu.java.nio.channels.SocketChannelImpl)
> +      result = new 
> gnu.java.nio.channels.SocketChannelSelectionKeyImpl((gnu.java.nio.channels.SocketChannelImpl)ch,
>  this);
>      else
>        throw new InternalError ("No known channel type");
>  
> Index: gnu/java/nio/SelectorProviderImpl.java
> ===================================================================
> RCS file: 
> /cvsroot/classpath/classpath/gnu/java/nio/SelectorProviderImpl.java,v
> retrieving revision 1.8
> diff -u -r1.8 SelectorProviderImpl.java
> --- gnu/java/nio/SelectorProviderImpl.java    2 Jul 2005 20:32:13 -0000       
> 1.8
> +++ gnu/java/nio/SelectorProviderImpl.java    11 Jan 2006 22:09:41 -0000
> @@ -78,6 +78,7 @@
>    public SocketChannel openSocketChannel ()
>      throws IOException
>    {
> -    return new SocketChannelImpl (this);
> +    //return new SocketChannelImpl (this);
> +    return new gnu.java.nio.channels.SocketChannelImpl (this);
>    }
>  }
Watch your style: Do not leave unnused code paths as comments in the file. If we
find out the patch was wrong CVS can be used to retrieve and older version.


> Index: gnu/java/nio/ServerSocketChannelImpl.java
> ===================================================================
> RCS file: 
> /cvsroot/classpath/classpath/gnu/java/nio/ServerSocketChannelImpl.java,v
> retrieving revision 1.14
> diff -u -r1.14 ServerSocketChannelImpl.java
> --- gnu/java/nio/ServerSocketChannelImpl.java 2 Jul 2005 20:32:13 -0000       
> 1.14
> +++ gnu/java/nio/ServerSocketChannelImpl.java 11 Jan 2006 22:09:42 -0000
> @@ -102,7 +102,7 @@
>            // indicate that a channel is initiating the accept operation
>            // so that the socket ignores the fact that we might be in
>            // non-blocking mode.
> -        NIOSocket socket = (NIOSocket) serverSocket.accept();
> +        gnu.java.nio.channels.NIOSocket socket = 
> (gnu.java.nio.channels.NIOSocket) serverSocket.accept();
>          completed = true;
>          return socket.getChannel();
>        }
We usually use the import statement to avoid writing the class with complete
package name.

> Index: include/Makefile.am
> ===================================================================
> RCS file: /cvsroot/classpath/classpath/include/Makefile.am,v
> retrieving revision 1.52
> diff -u -r1.52 Makefile.am
> --- include/Makefile.am       4 Jan 2006 16:37:55 -0000       1.52
> +++ include/Makefile.am       11 Jan 2006 22:09:43 -0000
> @@ -122,6 +122,7 @@
>  $(top_srcdir)/include/gnu_java_nio_VMPipe.h \
>  $(top_srcdir)/include/gnu_java_nio_VMSelector.h \
>  $(top_srcdir)/include/gnu_java_nio_channels_FileChannelImpl.h \
> +$(top_srcdir)/include/gnu_java_nio_channels_VMChannel.h \
>  $(top_srcdir)/include/gnu_java_nio_charset_iconv_IconvEncoder.h \
>  $(top_srcdir)/include/gnu_java_nio_charset_iconv_IconvDecoder.h \
>  $(top_srcdir)/include/java_io_VMFile.h \
> @@ -200,6 +201,8 @@
>       $(JAVAH) -o $@ java.nio.MappedByteBufferImpl
>  $(top_srcdir)/include/gnu_java_nio_channels_FileChannelImpl.h: 
> $(top_srcdir)/gnu/java/nio/channels/FileChannelImpl.java
>       $(JAVAH) -o $@ gnu.java.nio.channels.FileChannelImpl
> +$(top_srcdir)/include/gnu_java_nio_channels_FileChannelImpl.h: 
> $(top_srcdir)/gnu/java/nio/channels/VMChannel.java
> +     $(JAVAH) -o $@ gnu.java.nio.channels.VMChannel
>  $(top_srcdir)/include/gnu_java_nio_charset_iconv_IconvDecoder.h: 
> $(top_srcdir)/gnu/java/nio/charset/iconv/IconvDecoder.java
>       $(JAVAH) -o $@ gnu.java.nio.charset.iconv.IconvDecoder
>  $(top_srcdir)/include/gnu_java_nio_charset_iconv_IconvEncoder.h: 
> $(top_srcdir)/gnu/java/nio/charset/iconv/IconvEncoder.java
> Index: native/jni/java-nio/Makefile.am
> ===================================================================
> RCS file: /cvsroot/classpath/classpath/native/jni/java-nio/Makefile.am,v
> retrieving revision 1.19
> diff -u -r1.19 Makefile.am
> --- native/jni/java-nio/Makefile.am   23 Oct 2005 16:59:08 -0000      1.19
> +++ native/jni/java-nio/Makefile.am   11 Jan 2006 22:09:53 -0000
> @@ -5,8 +5,10 @@
>                       gnu_java_nio_channels_FileChannelImpl.c \
>                       gnu_java_nio_charset_iconv_IconvDecoder.c \
>                       gnu_java_nio_charset_iconv_IconvEncoder.c \
> +                     gnu_java_nio_channels_VMChannel.c \
>                       java_nio_MappedByteBufferImpl.c \
> -                     java_nio_VMDirectByteBuffer.c
> +                     java_nio_VMDirectByteBuffer.c 
> +                     
>  
>  libjavanio_la_LIBADD = $(top_builddir)/native/jni/classpath/jcl.lo \
>                      $(LTLIBICONV) 
> Index: gnu/java/nio/channels/NIOSocket.java
> ===================================================================
> RCS file: gnu/java/nio/channels/NIOSocket.java
> diff -N gnu/java/nio/channels/NIOSocket.java
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ gnu/java/nio/channels/NIOSocket.java      1 Jan 1970 00:00:00 -0000
> @@ -0,0 +1,79 @@
> +/* NIOSocket.java -- Wrapper socket implemenation class.
> +   Copyright (C) 2003 Free Software Foundation, Inc.
Its 2006. Please change the copyright line to:
   Copyright (C) 2003, 2006 Free Software Foundation, Inc.

Btw: This has to be done for all the files you touch.

> [...]
> +
> +/**
> + * @author Michael Koch
> + */
> +public final class NIOSocket extends Socket
> +{
> +  private PlainSocketImpl impl;
> +  private SocketChannelImpl channel;
> +    
> +  protected NIOSocket (PlainSocketImpl impl, SocketChannelImpl channel)
> +    throws IOException
> +  {
> +    super (impl);
> +    this.impl = impl;
> +    this.channel = channel;
> +  }
> +
> +  public final PlainSocketImpl getPlainSocketImpl()
> +  {
> +    return impl;
> +  }
> +
> +  final void setChannel (SocketChannelImpl channel)
> +  {
> +    this.impl = channel.getPlainSocketImpl();
> +    this.channel = channel;
> +  }
> +  
> +  public final SocketChannel getChannel()
> +  {
> +    return channel;
> +  }
> +}
> Index: gnu/java/nio/channels/SocketChannelImpl.java
> ===================================================================
> RCS file: gnu/java/nio/channels/SocketChannelImpl.java
> diff -N gnu/java/nio/channels/SocketChannelImpl.java
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ gnu/java/nio/channels/SocketChannelImpl.java      1 Jan 1970 00:00:00 
> -0000
> @@ -0,0 +1,285 @@
> +/* PosixSocketChannelImpl.java -- Non-blocking enable socket channel
> +   Copyright (C) 2005 Free Software Foundation, Inc.
Same here.

> [...]
> +
> +package gnu.java.nio.channels;
> +
> +import gnu.java.net.PlainSocketImpl;
> +import gnu.java.nio.NIOConstants;
> +
> +import java.io.IOException;
> +import java.net.InetSocketAddress;
> +import java.net.Socket;
> +import java.net.SocketAddress;
> +import java.net.SocketTimeoutException;
> +import java.nio.ByteBuffer;
> +import java.nio.channels.AlreadyConnectedException;
> +import java.nio.channels.ClosedChannelException;
> +import java.nio.channels.ConnectionPendingException;
> +import java.nio.channels.NoConnectionPendingException;
> +import java.nio.channels.SelectionKey;
> +import java.nio.channels.Selector;
> +import java.nio.channels.SocketChannel;
> +import java.nio.channels.UnresolvedAddressException;
> +import java.nio.channels.UnsupportedAddressTypeException;
> +import java.nio.channels.spi.SelectorProvider;
> +
> +/**
> + * @author Michael Barker
You can add an email address in round parentheses here. No fear the address is
not published on developer.classpath.org/doc .

> + *
> + */
> +public class SocketChannelImpl extends SocketChannel
> +{
> +  private VMChannel vmch;
> +  private PlainSocketImpl sImpl;
> +  private Socket socket;
> +  private boolean connectionPending;
> +  
> +  /**
> +   * @param provider
> +   * @throws IOException 
> +   */
> +  public SocketChannelImpl(SelectorProvider provider) 
> +    throws IOException
> +  {
> +    super(provider);
> +    this.vmch = new VMChannel();
> +    this.sImpl = new PlainSocketImpl();
> +    this.socket = new NIOSocket(sImpl, this);
> +  }
> +  
> +  /* (non-Javadoc)
> +   * @see java.nio.channels.SocketChannel#read(java.nio.ByteBuffer)
> +   */
Either write real javadoc or remove these Eclipse-generated stubs.
> +  public int read(ByteBuffer dst) 
> +    throws IOException
> +  {
> +    return vmch.read(getFd(), dst);
> +  }
> +  
> +
> [...]
> Index: gnu/java/nio/channels/SocketChannelSelectionKeyImpl.java
> ===================================================================
> RCS file: gnu/java/nio/channels/SocketChannelSelectionKeyImpl.java
> diff -N gnu/java/nio/channels/SocketChannelSelectionKeyImpl.java
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ gnu/java/nio/channels/SocketChannelSelectionKeyImpl.java  1 Jan 1970 
> 00:00:00 -0000
> @@ -0,0 +1,71 @@
> +/* SocketChannelSelectionKey.java -- Selection key for Socket Channel
> +   Copyright (C) 2005 Free Software Foundation, Inc.
> +
> +This file is part of GNU Classpath.
> +
> +GNU Classpath is free software; you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 2, or (at your option)
> +any later version.
> +
> +GNU Classpath is distributed in the hope that it will be useful, but
> +WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with GNU Classpath; see the file COPYING.  If not, write to the
> +Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library.  Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module.  An independent module is a module which is not derived from
> +or based on this library.  If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so.  If you do not wish to do so, delete this
> +exception statement from your version. */
> +
> +
> +package gnu.java.nio.channels;
> +
> +import gnu.java.nio.SelectionKeyImpl;
> +import gnu.java.nio.SelectorImpl;
> +
> +/**
> + * @author mike
Be consistent with the name.
> + *
> + */
> +public class SocketChannelSelectionKeyImpl extends SelectionKeyImpl
> +{
> +
> +  SocketChannelImpl ch;
> +
These macros look strange to me. What do our JNI experts say?

> +/* FIXME: This can't be right.  Need converter macros */
> +#define CONVERT_JINT_TO_INT(x) ((int)(x & 0xFFFFFFFF))
> +#define CONVERT_INT_TO_JINT(x) ((int)(x & 0xFFFFFFFF))
> +
> +/* FIXME: This can't be right.  Need converter macros. */
> +#define CONVERT_SSIZE_T_TO_JINT(x) ((jint)(x & 0xFFFFFFFF))
> +#define CONVERT_JINT_TO_SSIZE_T(x) (x)
> +
> +static jfieldID address_fid;
> +static jmethodID get_position_mid;
> +static jmethodID set_position_mid;
> +static jmethodID get_limit_mid;
> +static jmethodID set_limit_mid;
> +static jmethodID has_array_mid;
> +static jmethodID array_mid;
> +static jmethodID array_offset_mid;
> +/*
> +static jmethodID put_mid;
> +static jmethodID get_mid;
> +*/
> +

signature.asc
Description: OpenPGP digital signature