[PATCH] Add a `read' method for ports

[PATCH] Add a `read' method for ports

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 1490 bytes --]

Hello,

This is a followup to these threads:

  http://thread.gmane.org/gmane.lisp.guile.devel/6549
  http://thread.gmane.org/gmane.lisp.guile.devel/7155 (to a lesser extent)

The attached patch aims to allow an `scm_c_read ()' call for N bytes to
translate into a `read(2)' for N bytes in the case of unbuffered file
ports (such as sockets); it also allows the port's buffer to be bypassed
when more data is requested than can fit in the buffer.

The first patch adds a `read' method to ports, which should read up to N
bytes into a caller-supplied buffer.  It is meant as a replacement for
`fill_input', which fills in the port's buffer regardless of the
application request, that is, reads a single byte in the case of
unbuffered ports.

The change is ABI-compatible since:

  1. `scm_t_ptob_descriptor's are always allocated by libguile code, not
     by the application code, so changing its size doesn't break the
     ABI;

  2. Apart from the added `read' field, the layout of
     `scm_t_ptob_descriptor' is unchanged, so inlines and macros that
     refer to it still work;

  3. `fill_input' is still honored when provided.

Subsequent patches make use of `scm_set_port_read ()' in file and string
ports, change `uniform-vector-read!' to use `scm_c_read ()', and add a
benchmark for this.  The benchmark shows that `uniform-vector-read!' is
around 3 orders of magnitude (!) faster on unbuffered file ports with
the new method.

OK to commit to 1.8 and master?

Thanks,
Ludovic.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: The patch --]
[-- Type: text/x-patch, Size: 6276 bytes --]

From 3f9bae2580bca420b20d31e1074a2bab19aa3c09 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Sat, 31 May 2008 22:24:45 +0200
Subject: [PATCH] Add `scm_set_port_read ()'.

---
 libguile/ports.c |  120 +++++++++++++++++++++++++++++++++++++++++++++---------
 libguile/ports.h |    5 ++-
 2 files changed, 104 insertions(+), 21 deletions(-)

diff --git a/libguile/ports.c b/libguile/ports.c
index b25a7d0..f887ec9 100644
--- a/libguile/ports.c
+++ b/libguile/ports.c
@@ -83,6 +83,10 @@
 #define HAVE_FTRUNCATE 1
 #endif
 
+#ifndef SCM_MIN
+# define SCM_MIN(A, B) ((A) < (B) ? (A) : (B))
+#endif
+
 \f
 /* The port kind table --- a dynamically resized array of port types.  */
 
@@ -162,6 +166,8 @@ scm_make_port_type (char *name,
       scm_ptobs[scm_numptob].seek = 0;
       scm_ptobs[scm_numptob].truncate = 0;
 
+      scm_ptobs[scm_numptob].read = NULL;
+
       scm_numptob++;
     }
   SCM_CRITICAL_SECTION_END;
@@ -177,6 +183,15 @@ scm_make_port_type (char *name,
 }
 
 void
+scm_set_port_read (scm_t_bits tc,
+		   size_t (* read) (SCM, void *, size_t))
+{
+  /* The provided `read' method overrides `fill_input'.  */
+  scm_ptobs[SCM_TC2PTOBNUM (tc)].fill_input = NULL;
+  scm_ptobs[SCM_TC2PTOBNUM (tc)].read = read;
+}
+
+void
 scm_set_port_mark (scm_t_bits tc, SCM (*mark) (SCM))
 {
   scm_ptobs[SCM_TC2PTOBNUM (tc)].mark = mark;
@@ -946,6 +961,7 @@ int
 scm_fill_input (SCM port)
 {
   scm_t_port *pt = SCM_PTAB_ENTRY (port);
+  const scm_t_ptob_descriptor *ptob = &scm_ptobs[SCM_PTOBNUM (port)];
 
   if (pt->read_buf == pt->putback_buf)
     {
@@ -957,7 +973,23 @@ scm_fill_input (SCM port)
       if (pt->read_pos < pt->read_end)
 	return *(pt->read_pos);
     }
-  return scm_ptobs[SCM_PTOBNUM (port)].fill_input (port);
+
+  if (ptob->fill_input != NULL)
+    /* Kept for backward compatibility.  */
+    return ptob->fill_input (port);
+  else
+    {
+      size_t count;
+
+      count = ptob->read (port, pt->read_buf, pt->read_buf_size);
+      pt->read_pos = pt->read_buf;
+      pt->read_end = pt->read_buf + count;
+
+      if (count == 0)
+	return EOF;
+      else
+	return (int) *pt->read_buf;
+    }
 }
 
 
@@ -1015,45 +1047,93 @@ scm_c_read (SCM port, void *buffer, size_t size)
 {
   scm_t_port *pt;
   size_t n_read = 0, n_available;
+  const scm_t_ptob_descriptor *ptob = &scm_ptobs[SCM_PTOBNUM (port)];
 
   SCM_VALIDATE_OPINPORT (1, port);
 
   pt = SCM_PTAB_ENTRY (port);
+  ptob = &scm_ptobs[SCM_PTOBNUM (port)];
   if (pt->rw_active == SCM_PORT_WRITE)
-    scm_ptobs[SCM_PTOBNUM (port)].flush (port);
+    ptob->flush (port);
 
   if (pt->rw_random)
     pt->rw_active = SCM_PORT_READ;
 
   if (SCM_READ_BUFFER_EMPTY_P (pt))
     {
-      if (scm_fill_input (port) == EOF)
-	return 0;
+      if ((ptob->fill_input != NULL) || (size <= pt->read_buf_size))
+	{
+	  /* Fill in PORT's buffer.  */
+	  if (scm_fill_input (port) == EOF)
+	    return 0;
+	}
+      else
+	/* Read directly into BUFFER, bypassing PORT's own buffer.  */
+	return (ptob->read (port, buffer, size));
     }
-  
+
   n_available = pt->read_end - pt->read_pos;
-  
-  while (n_available < size)
+
+  if (ptob->fill_input == NULL)
     {
-      memcpy (buffer, pt->read_pos, n_available);
-      buffer = (char *) buffer + n_available;
-      pt->read_pos += n_available;
-      n_read += n_available;
-      
-      if (SCM_READ_BUFFER_EMPTY_P (pt))
+      /* Use PTOB's `read' method.  */
+      size_t count;
+
+      count = SCM_MIN (n_available, size);
+      memcpy (buffer, pt->read_pos, count);
+      pt->read_pos += count;
+      n_read += count;
+
+      if (n_read < size)
 	{
-	  if (scm_fill_input (port) == EOF)
-	    return n_read;
+	  /* PORT's buffer is now empty but we haven't yet read SIZE
+	     bytes.  */
+	  size_t remaining = size - n_read;
+
+	  pt->read_pos = pt->read_end = pt->read_buf;
+
+	  if (remaining >= pt->read_buf_size)
+	    /* Read directly into BUFFER.  */
+	    n_read += ptob->read (port, buffer + n_read, remaining);
+	  else
+	    {
+	      /* The remaining size is less than PORT's buffer size.  */
+	      count = ptob->read (port, pt->read_buf, pt->read_buf_size);
+	      pt->read_end += count;
+
+	      memcpy (buffer + n_read, pt->read_buf, SCM_MIN (remaining, count));
+	      pt->read_pos += SCM_MIN (remaining, count);
+	      n_read += SCM_MIN (remaining, count);
+	    }
 	}
 
-      size -= n_available;
-      n_available = pt->read_end - pt->read_pos;
+      return n_read;
     }
+  else
+    {
+      /* For backward compatibility: use the `fill_input' method.  */
+      while (n_available < size)
+	{
+	  memcpy (buffer, pt->read_pos, n_available);
+	  buffer = (char *) buffer + n_available;
+	  pt->read_pos += n_available;
+	  n_read += n_available;
+
+	  if (SCM_READ_BUFFER_EMPTY_P (pt))
+	    {
+	      if (scm_fill_input (port) == EOF)
+		return n_read;
+	    }
+
+	  size -= n_available;
+	  n_available = pt->read_end - pt->read_pos;
+	}
 
-  memcpy (buffer, pt->read_pos, size);
-  pt->read_pos += size;
+      memcpy (buffer, pt->read_pos, size);
+      pt->read_pos += size;
 
-  return n_read + size;
+      return n_read + size;
+    }
 }
 #undef FUNC_NAME
 
diff --git a/libguile/ports.h b/libguile/ports.h
index 084a555..17bbe09 100644
--- a/libguile/ports.h
+++ b/libguile/ports.h
@@ -178,12 +178,13 @@ typedef struct scm_t_ptob_descriptor
   void (*flush) (SCM port);
 
   void (*end_input) (SCM port, int offset);
-  int (*fill_input) (SCM port);
+  int (*fill_input) (SCM port);   /* deprecated: use `read' instead */
   int (*input_waiting) (SCM port);
 
   off_t (*seek) (SCM port, off_t OFFSET, int WHENCE);
   void (*truncate) (SCM port, off_t length);
 
+  size_t (*read) (SCM port, void *data, size_t size);
 } scm_t_ptob_descriptor;
 
 #define SCM_TC2PTOBNUM(x) (0x0ff & ((x) >> 8))
@@ -205,6 +206,8 @@ SCM_API scm_t_bits scm_make_port_type (char *name,
 				       void (*write) (SCM port, 
 						      const void *data,
 						      size_t size));
+SCM_API void scm_set_port_read (scm_t_bits tc,
+				size_t (* read) (SCM, void *, size_t));
 SCM_API void scm_set_port_mark (scm_t_bits tc, SCM (*mark) (SCM));
 SCM_API void scm_set_port_free (scm_t_bits tc, size_t (*free) (SCM));
 SCM_API void scm_set_port_print (scm_t_bits tc,
-- 
1.5.5


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Use-scm_set_port_read-in-file-ports.patch --]
[-- Type: text/x-patch, Size: 1863 bytes --]

From 406d36e849d2d88452f816e2325c5607250b28dc Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Sat, 31 May 2008 22:26:29 +0200
Subject: [PATCH] Use `scm_set_port_read ()' in file ports.

---
 libguile/fports.c |   23 ++++++++---------------
 1 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/libguile/fports.c b/libguile/fports.c
index efbd278..1208836 100644
--- a/libguile/fports.c
+++ b/libguile/fports.c
@@ -582,29 +582,21 @@ fport_wait_for_input (SCM port)
 
 static void fport_flush (SCM port);
 
-/* fill a port's read-buffer with a single read.  returns the first
-   char or EOF if end of file.  */
-static int
-fport_fill_input (SCM port)
+/* Read up to SIZE bytes from PORT into BUFFER.  */
+static size_t
+fport_read (SCM port, void *buffer, size_t size)
 {
   long count;
-  scm_t_port *pt = SCM_PTAB_ENTRY (port);
   scm_t_fport *fp = SCM_FSTREAM (port);
 
 #ifndef __MINGW32__
   fport_wait_for_input (port);
 #endif /* !__MINGW32__ */
-  SCM_SYSCALL (count = read (fp->fdes, pt->read_buf, pt->read_buf_size));
+  SCM_SYSCALL (count = read (fp->fdes, buffer, size));
   if (count == -1)
     scm_syserror ("fport_fill_input");
-  if (count == 0)
-    return EOF;
-  else
-    {
-      pt->read_pos = pt->read_buf;
-      pt->read_end = pt->read_buf + count;
-      return *pt->read_buf;
-    }
+
+  return (size_t) count;
 }
 
 static off_t_or_off64_t
@@ -906,8 +898,9 @@ fport_free (SCM port)
 static scm_t_bits
 scm_make_fptob ()
 {
-  scm_t_bits tc = scm_make_port_type ("file", fport_fill_input, fport_write);
+  scm_t_bits tc = scm_make_port_type ("file", NULL, fport_write);
 
+  scm_set_port_read            (tc, fport_read);
   scm_set_port_free            (tc, fport_free);
   scm_set_port_print           (tc, fport_print);
   scm_set_port_flush           (tc, fport_flush);
-- 
1.5.5


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Use-scm_c_read-in-uniform-vector-read.patch --]
[-- Type: text/x-patch, Size: 3307 bytes --]

From a4e465c07d450566115f43844a042e127cb0eeea Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Sat, 31 May 2008 22:27:46 +0200
Subject: [PATCH] Use `scm_c_read ()' in `uniform-vector-read!'.

---
 libguile/srfi-4.c |   61 ++++++++++++++++++++---------------------------------
 1 files changed, 23 insertions(+), 38 deletions(-)

diff --git a/libguile/srfi-4.c b/libguile/srfi-4.c
index 7d22f8b..5bcdcd5 100644
--- a/libguile/srfi-4.c
+++ b/libguile/srfi-4.c
@@ -1,6 +1,6 @@
 /* srfi-4.c --- Uniform numeric vector datatypes.
  *
- * 	Copyright (C) 2001, 2004, 2006 Free Software Foundation, Inc.
+ * 	Copyright (C) 2001, 2004, 2006, 2008 Free Software Foundation, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -824,6 +824,17 @@ SCM_DEFINE (scm_uniform_vector_length, "uniform-vector-length", 1, 0, 0,
 }
 #undef FUNC_NAME
 
+/* Release the array handle pointed to by ARG.  */
+static void
+release_array (void *arg)
+{
+  scm_t_array_handle *handle;
+
+  handle = (scm_t_array_handle *) arg;
+  scm_array_handle_release (handle);
+}
+
+
 SCM_DEFINE (scm_uniform_vector_read_x, "uniform-vector-read!", 1, 3, 0,
            (SCM uvec, SCM port_or_fd, SCM start, SCM end),
 	    "Fill the elements of @var{uvec} by reading\n"
@@ -846,7 +857,7 @@ SCM_DEFINE (scm_uniform_vector_read_x, "uniform-vector-read!", 1, 3, 0,
 #define FUNC_NAME s_scm_uniform_vector_read_x
 {
   scm_t_array_handle handle;
-  size_t vlen, sz, ans;
+  size_t vlen, sz, ans, bytes_read;
   ssize_t inc;
   size_t cstart, cend;
   size_t remaining, off;
@@ -886,52 +897,26 @@ SCM_DEFINE (scm_uniform_vector_read_x, "uniform-vector-read!", 1, 3, 0,
 
   if (SCM_NIMP (port_or_fd))
     {
-      scm_t_port *pt = SCM_PTAB_ENTRY (port_or_fd);
+      scm_dynwind_begin (0);
+      scm_dynwind_unwind_handler (release_array, &handle, 0);
 
-      if (pt->rw_active == SCM_PORT_WRITE)
-	scm_flush (port_or_fd);
+      bytes_read = scm_c_read (port_or_fd, base + off, remaining);
 
-      ans = cend - cstart;
-      while (remaining > 0)
-	{
-	  if (pt->read_pos < pt->read_end)
-	    {
-	      size_t to_copy = min (pt->read_end - pt->read_pos,
-				    remaining);
-	      
-	      memcpy (base + off, pt->read_pos, to_copy);
-	      pt->read_pos += to_copy;
-	      remaining -= to_copy;
-	      off += to_copy;
-	    }
-	  else
-	    {
-	      if (scm_fill_input (port_or_fd) == EOF)
-		{
-		  if (remaining % sz != 0)
-		    SCM_MISC_ERROR ("unexpected EOF", SCM_EOL);
-		  ans -= remaining / sz;
-		  break;
-		}
-	    }
-	}
-      
-      if (pt->rw_random)
-	pt->rw_active = SCM_PORT_READ;
+      scm_dynwind_end ();
     }
   else /* file descriptor.  */
     {
       int fd = scm_to_int (port_or_fd);
-      int n;
 
-      SCM_SYSCALL (n = read (fd, base + off, remaining));
-      if (n == -1)
+      SCM_SYSCALL (bytes_read = read (fd, base + off, remaining));
+      if (bytes_read == -1)
 	SCM_SYSERROR;
-      if (n % sz != 0)
-	SCM_MISC_ERROR ("unexpected EOF", SCM_EOL);
-      ans = n / sz;
     }
 
+  if (bytes_read % sz != 0)
+    SCM_MISC_ERROR ("unexpected EOF", SCM_EOL);
+  ans = bytes_read / sz;
+
   scm_array_handle_release (&handle);
 
   return scm_from_size_t (ans);
-- 
1.5.5


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Use-scm_set_port_read-in-string-ports.patch --]
[-- Type: text/x-patch, Size: 2042 bytes --]

From 9ff77d3b9fe8384841743c4e7a3ef5b33d9a83b5 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Sun, 1 Jun 2008 19:21:35 +0200
Subject: [PATCH] Use `scm_set_port_read ()' in string ports.

---
 libguile/strports.c |   27 ++++++++++++++++++---------
 1 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/libguile/strports.c b/libguile/strports.c
index 8659ccf..f6fb24b 100644
--- a/libguile/strports.c
+++ b/libguile/strports.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 1995,1996,1998,1999,2000,2001,2002, 2003, 2005, 2006 Free Software Foundation, Inc.
+/* Copyright (C) 1995,1996,1998,1999,2000,2001,2002, 2003, 2005, 2006, 2008 Free Software Foundation, Inc.
  * 
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -45,6 +45,10 @@
 #include <string.h>
 #endif
 
+#ifndef SCM_MIN
+# define SCM_MIN(A, B) ((A) < (B) ? (A) : (B))
+#endif
+
 \f
 
 /* {Ports - string ports}
@@ -93,15 +97,19 @@
 scm_t_bits scm_tc16_strport;
 
 
-static int
-stfill_buffer (SCM port)
+/* Read up to SIZE bytes from PORT into BUFFER.  */
+static size_t
+st_read (SCM port, void *buffer, size_t size)
 {
+  size_t available, count;
   scm_t_port *pt = SCM_PTAB_ENTRY (port);
-  
-  if (pt->read_pos >= pt->read_end)
-    return EOF;
-  else
-    return scm_return_first_int (*pt->read_pos, port);
+
+  available = pt->read_end - pt->read_pos;
+  count = SCM_MIN (available, size);
+
+  memcpy (buffer, pt->read_pos, count);
+
+  return count;
 }
 
 /* change the size of a port's string to new_size.  this doesn't
@@ -538,8 +546,9 @@ scm_eval_string (SCM string)
 static scm_t_bits
 scm_make_stptob ()
 {
-  scm_t_bits tc = scm_make_port_type ("string", stfill_buffer, st_write);
+  scm_t_bits tc = scm_make_port_type ("string", NULL, st_write);
 
+  scm_set_port_read        (tc, st_read);
   scm_set_port_mark        (tc, scm_markstream);
   scm_set_port_end_input   (tc, st_end_input);
   scm_set_port_flush       (tc, st_flush);
-- 
1.5.5


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0005-Add-uniform-vector-read-benchmark.patch --]
[-- Type: text/x-patch, Size: 3132 bytes --]

From a627bb3639d2a1cffccddf55de679c129ee0cda1 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Sun, 1 Jun 2008 19:31:36 +0200
Subject: [PATCH] Add `uniform-vector-read!' benchmark.

---
 benchmark-suite/Makefile.am                       |   11 ++--
 benchmark-suite/benchmarks/uniform-vector-read.bm |   53 +++++++++++++++++++++
 2 files changed, 59 insertions(+), 5 deletions(-)
 create mode 100644 benchmark-suite/benchmarks/uniform-vector-read.bm

diff --git a/benchmark-suite/Makefile.am b/benchmark-suite/Makefile.am
index a8f4719..3993faf 100644
--- a/benchmark-suite/Makefile.am
+++ b/benchmark-suite/Makefile.am
@@ -1,7 +1,8 @@
-SCM_BENCHMARKS = benchmarks/0-reference.bm	\
-	         benchmarks/continuations.bm	\
-                 benchmarks/if.bm		\
-                 benchmarks/logand.bm		\
-		 benchmarks/read.bm
+SCM_BENCHMARKS = benchmarks/0-reference.bm		\
+	         benchmarks/continuations.bm		\
+                 benchmarks/if.bm			\
+                 benchmarks/logand.bm			\
+		 benchmarks/read.bm			\
+		 benchmarks/uniform-vector-read.bm
 
 EXTRA_DIST = guile-benchmark lib.scm $(SCM_BENCHMARKS)
diff --git a/benchmark-suite/benchmarks/uniform-vector-read.bm b/benchmark-suite/benchmarks/uniform-vector-read.bm
new file mode 100644
index 0000000..d288f0b
--- /dev/null
+++ b/benchmark-suite/benchmarks/uniform-vector-read.bm
@@ -0,0 +1,53 @@
+;;; uniform-vector-read.bm --- Exercise binary I/O primitives.  -*- Scheme -*-
+;;;
+;;; Copyright (C) 2008 Free Software Foundation, Inc.
+;;;
+;;; This program 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.
+;;;
+;;; This program 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 this software; see the file COPYING.  If not, write to
+;;; the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+;;; Boston, MA 02110-1301 USA
+
+(define-module (benchmarks uniform-vector-read)
+  :use-module (benchmark-suite lib)
+  :use-module (srfi srfi-4))
+
+(define file-name
+  (tmpnam))
+
+(define %buffer-size
+  7777)
+
+(define buf
+  (make-u8vector %buffer-size))
+
+(define str
+  (make-string %buffer-size))
+
+\f
+(with-benchmark-prefix "uniform-vector-read!"
+
+  (benchmark "uniform-vector-write" 500
+    (let ((output (open-output-file file-name)))
+      (uniform-vector-write buf output)
+      (close output)))
+
+  (benchmark "uniform-vector-read!" 500
+    (let ((input (open-input-file file-name)))
+      (setvbuf input _IONBF)
+      (uniform-vector-read! buf input)
+      (close input)))
+
+  (benchmark "string port" 5000
+    (let ((input (open-input-string str)))
+      (uniform-vector-read! buf input)
+      (close input))))
-- 
1.5.5

Re: [PATCH] Add a `read' method for ports

[Ludovic Courtès]

Hello Guilers!

ludo@gnu.org (Ludovic Courtès) writes:

> The attached patch aims to allow an `scm_c_read ()' call for N bytes to
> translate into a `read(2)' for N bytes in the case of unbuffered file
> ports (such as sockets); it also allows the port's buffer to be bypassed
> when more data is requested than can fit in the buffer.

I'll commit it to both branches by the end of the week if nobody
disagrees.

Thanks,
Ludovic.

Re: [PATCH] Add a `read' method for ports

[Neil Jerram]

2008/6/8 Ludovic Courtès <ludo@gnu.org>:
> Hello Guilers!
>
> ludo@gnu.org (Ludovic Courtès) writes:
>
>> The attached patch aims to allow an `scm_c_read ()' call for N bytes to
>> translate into a `read(2)' for N bytes in the case of unbuffered file
>> ports (such as sockets); it also allows the port's buffer to be bypassed
>> when more data is requested than can fit in the buffer.
>
> I'll commit it to both branches by the end of the week if nobody
> disagrees.
>
> Thanks,
> Ludovic.

Hi Ludovic,

Sorry for not commenting before...  I've reviewed the history, and
started looking through the patches, and I'm feeling (possibly)
confused.  Am I correct in thinking that the "problem" here (which you
are aiming to solve) only affects unbuffered ports?

If that's right, I'm not clear why the solution is involving so much
restructuring - can't it be done without introducing the new "read"
field?

Regards,
     Neil

Re: [PATCH] Add a `read' method for ports

[Ludovic Courtès]

Hi Neil,

"Neil Jerram" <neiljerram@googlemail.com> writes:

> Sorry for not commenting before...  I've reviewed the history, and
> started looking through the patches, and I'm feeling (possibly)
> confused.  Am I correct in thinking that the "problem" here (which you
> are aiming to solve) only affects unbuffered ports?

Yes, such as sockets.

> If that's right, I'm not clear why the solution is involving so much
> restructuring - can't it be done without introducing the new "read"
> field?

I don't think so.  Currently, when asking for N bytes at the application
level, e.g., via `uniform-vector-read!' or `scm_c_read ()', we end up
going through a series of `scm_fill_input ()'.  For unbuffered ports,
this translates into N `scm_fill_input ()', and for unbuffered file
ports, into N one-byte `read(2)' calls.

IOW, the information about how many bytes are requested by the
application isn't currently transferred to port implementations.  This
is what the `read' method aims to solve.

The `read' method also has the advantage that it doesn't need to fiddle
with `port->read_buf', unlike `fill_input' (but one still has to deal
with it when instantiating a port).

Thanks,
Ludovic.

Re: [PATCH] Add a `read' method for ports

[Neil Jerram]

Hi Ludovic,

2008/6/9 Ludovic Courtès <ludo@gnu.org>:
> Hi Neil,
>
> "Neil Jerram" <neiljerram@googlemail.com> writes:
>
>> Sorry for not commenting before...  I've reviewed the history, and
>> started looking through the patches, and I'm feeling (possibly)
>> confused.  Am I correct in thinking that the "problem" here (which you
>> are aiming to solve) only affects unbuffered ports?
>
> Yes, such as sockets.

OK, understood, thanks.

>> If that's right, I'm not clear why the solution is involving so much
>> restructuring - can't it be done without introducing the new "read"
>> field?
>
> I don't think so.  Currently, when asking for N bytes at the application
> level, e.g., via `uniform-vector-read!' or `scm_c_read ()', we end up
> going through a series of `scm_fill_input ()'.  For unbuffered ports,
> this translates into N `scm_fill_input ()', and for unbuffered file
> ports, into N one-byte `read(2)' calls.

Yes, I see that now, and I agree that it is bad.

> IOW, the information about how many bytes are requested by the
> application isn't currently transferred to port implementations.  This
> is what the `read' method aims to solve.

I think there is a simpler solution, without introducing `read'.

One way of seeing this is that when a higher level function such as
scm_c_read or scm_uniform_vector_read requests a read of a specific
size from an `unbuffered' port, it is actually fine - and desirable -
for the port to use a buffer of that size.  When the user asks for
`unbuffered', they really only mean that they never want excess data
(i.e. more than they asked for) to be left in a buffer somewhere.

So, one way of solving this would be for an `unbuffered' port
temporarily to get a buffer of the right size, and then to use the
fill_input logic as it currently stands.  That would achieve your
objective of mapping onto a single N-byte read(2) call.

Then the question is just where the temporary buffer comes from.  Is
it provided by the caller, is it malloc'd and realloc'd and lives with
the port, or what?  (Note that if the buffer is allowed to persist
with the port, we can still get correct 'unbuffered' semantics by
reducing pt->read_buf_size back to 1.

I haven't thought through all the cases yet, but it feels as though
there should be a nice answer here.  For scm_c_read and
scm_uniform_vector_read, for example, the callers already have a
buffer, so all we need is a way to temporarily use that as the port's
read buffer.  Overall, it feels that this would involve less change
than your current patches, and it also feels more consistent (than the
read approach) with the existing API for explicitly adding a buffer to
a port.

(Are there actually _any_ cases where a caller does a read on a
possibly-unbuffered port, and either (i) has no idea of how much data
it is wanting, or (ii) doesn't already have a buffer that could be
used to do the reading?)

> The `read' method also has the advantage that it doesn't need to fiddle
> with `port->read_buf', unlike `fill_input' (but one still has to deal
> with it when instantiating a port).

But that advantage is also (in some cases) the disadvantage of having
to do an additional memcpy of the data.  I don't think it's too bad
for callers to be fiddling with read_buf - although we could
encapsulate that a bit more clearly.

What do you think?

      Neil

Re: [PATCH] Add a `read' method for ports

[Neil Jerram]

[-- Attachment #1: Type: text/plain, Size: 566 bytes --]

2008/6/11 Neil Jerram <neiljerram@googlemail.com>:
>
> I haven't thought through all the cases yet, but it feels as though
> there should be a nice answer here.  For scm_c_read and
> scm_uniform_vector_read, for example, the callers already have a
> buffer, so all we need is a way to temporarily use that as the port's
> read buffer.

I couldn't resist trying this out... attached is a patch that
hopefully shows what I was trying to describe.  Please let me know
what you think.  (I haven't tried benchmarking it, but it at least
passes "make check".)

      Neil

[-- Attachment #2: scm_fill_input_buf.patch --]
[-- Type: application/octet-stream, Size: 4815 bytes --]

diff --git a/libguile/ports.c b/libguile/ports.c
index b97c826..41d54c1 100644
--- a/libguile/ports.c
+++ b/libguile/ports.c
@@ -28,6 +28,7 @@
 #include <stdio.h>
 #include <errno.h>
 #include <fcntl.h>  /* for chsize on mingw */
+#include <assert.h>
 
 #include "libguile/_scm.h"
 #include "libguile/async.h"
@@ -987,6 +988,45 @@ scm_fill_input (SCM port)
   return scm_ptobs[SCM_PTOBNUM (port)].fill_input (port);
 }
 
+int
+scm_fill_input_buf (SCM port, unsigned char *buf, size_t buf_size, size_t *n_available)
+{
+  scm_t_port *pt = SCM_PTAB_ENTRY (port);
+  int ans;
+
+  /* There's no point calling this routine with any null args. */
+  assert (buf != NULL);
+  assert (buf_size != 0);
+  assert (n_available != NULL);
+
+  if (pt->read_buf == &pt->shortbuf)
+    {
+      /* Port is using the `shortbuf', which has space for 1 character
+         only.  For the next operation, use the caller's buffer
+         instead.  Then put back the shortbuf afterwards. */
+      pt->read_pos = pt->read_buf = pt->read_end = buf;
+      pt->read_buf_size = buf_size;
+
+      ans = scm_fill_input (port);
+      *n_available = pt->read_end - pt->read_pos;
+
+      pt->read_pos = pt->read_buf = pt->read_end = &pt->shortbuf;
+      pt->read_buf_size = 1;
+    }
+  else
+    {
+      /* Port is already using a proper buffer.  Use that, then copy
+         what was read into the caller's buffer. */
+      ans = scm_fill_input (port);
+      *n_available = min (buf_size, pt->read_end - pt->read_pos);
+
+      memcpy (buf, pt->read_pos, *n_available);
+      pt->read_pos += *n_available;
+    }
+
+  return ans;
+}
+
 
 /* scm_lfwrite
  *
@@ -1052,35 +1092,28 @@ scm_c_read (SCM port, void *buffer, size_t size)
   if (pt->rw_random)
     pt->rw_active = SCM_PORT_READ;
 
-  if (SCM_READ_BUFFER_EMPTY_P (pt))
-    {
-      if (scm_fill_input (port) == EOF)
-	return 0;
-    }
-  
-  n_available = pt->read_end - pt->read_pos;
-  
-  while (n_available < size)
+  /* Take bytes first from the port's read buffer. */
+  if (pt->read_pos < pt->read_end)
     {
+      n_available = min (size, pt->read_end - pt->read_pos);
       memcpy (buffer, pt->read_pos, n_available);
       buffer = (char *) buffer + n_available;
       pt->read_pos += n_available;
       n_read += n_available;
-      
-      if (SCM_READ_BUFFER_EMPTY_P (pt))
-	{
-	  if (scm_fill_input (port) == EOF)
-	    return n_read;
-	}
-
       size -= n_available;
-      n_available = pt->read_end - pt->read_pos;
     }
 
-  memcpy (buffer, pt->read_pos, size);
-  pt->read_pos += size;
+  /* For as long as we need more, refill the port's buffer and copy
+     into our own one.  (If the port doesn't have its own buffer, it
+     will read directly into our one.) */
+  while (size && (scm_fill_input_buf (port, buffer, size, &n_available) != EOF))
+    {
+      buffer = (char *) buffer + n_available;
+      n_read += n_available;
+      size -= n_available;
+    }
 
-  return n_read + size;
+  return n_read;
 }
 #undef FUNC_NAME
 
diff --git a/libguile/ports.h b/libguile/ports.h
index d1aafb4..d3a1240 100644
--- a/libguile/ports.h
+++ b/libguile/ports.h
@@ -270,6 +270,7 @@ SCM_API void scm_lfwrite (const char *ptr, size_t size, SCM port);
 SCM_API void scm_flush (SCM port);
 SCM_API void scm_end_input (SCM port);
 SCM_API int scm_fill_input (SCM port);
+SCM_API int scm_fill_input_buf (SCM port, unsigned char *buf, size_t buf_size, size_t *n_available);
 SCM_API void scm_ungetc (int c, SCM port);
 SCM_API void scm_ungets (const char *s, int n, SCM port);
 SCM_API SCM scm_peek_char (SCM port);
diff --git a/libguile/srfi-4.c b/libguile/srfi-4.c
index 7d22f8b..a01f86e 100644
--- a/libguile/srfi-4.c
+++ b/libguile/srfi-4.c
@@ -886,38 +886,11 @@ SCM_DEFINE (scm_uniform_vector_read_x, "uniform-vector-read!", 1, 3, 0,
 
   if (SCM_NIMP (port_or_fd))
     {
-      scm_t_port *pt = SCM_PTAB_ENTRY (port_or_fd);
-
-      if (pt->rw_active == SCM_PORT_WRITE)
-	scm_flush (port_or_fd);
-
       ans = cend - cstart;
-      while (remaining > 0)
-	{
-	  if (pt->read_pos < pt->read_end)
-	    {
-	      size_t to_copy = min (pt->read_end - pt->read_pos,
-				    remaining);
-	      
-	      memcpy (base + off, pt->read_pos, to_copy);
-	      pt->read_pos += to_copy;
-	      remaining -= to_copy;
-	      off += to_copy;
-	    }
-	  else
-	    {
-	      if (scm_fill_input (port_or_fd) == EOF)
-		{
-		  if (remaining % sz != 0)
-		    SCM_MISC_ERROR ("unexpected EOF", SCM_EOL);
-		  ans -= remaining / sz;
-		  break;
-		}
-	    }
-	}
-      
-      if (pt->rw_random)
-	pt->rw_active = SCM_PORT_READ;
+      remaining -= scm_c_read (port_or_fd, base + off, remaining);
+      if (remaining % sz != 0)
+        SCM_MISC_ERROR ("unexpected EOF", SCM_EOL);
+      ans -= remaining / sz;
     }
   else /* file descriptor.  */
     {

Re: [PATCH] Add a `read' method for ports

[Ludovic Courtès]

Hi Neil,

"Neil Jerram" <neiljerram@googlemail.com> writes:

> So, one way of solving this would be for an `unbuffered' port
> temporarily to get a buffer of the right size, and then to use the
> fill_input logic as it currently stands.  That would achieve your
> objective of mapping onto a single N-byte read(2) call.

Surely this would work, but I can think of several issues:

  1. Thread safety.  OK, ports are not thread-safe and it'd take more
     than this to make them thread-safe, but passing arguments
     explicitly (as the BUFFER and SIZE arguments of `read') is a step
     forward, while mutating the buffer pointer and size in the port is
     a step backward.

  2. Performance.  Another case where the `read' method improves on
     performance: when more data is requested than can fit in the port's
     buffer, the port buffer is completely bypassed.  This allows the
     data to be read in one shot, avoiding memcpys from the port's
     buffer to the caller's buffer.

  3. Aesthetics.  The `read' method looks more natural to me, since it's
     just like `read(2)'.  Also, as mentioned earlier, `read' methods
     don't have to be aware of the port's internal buffer: they just
     need to fill in the caller-supplied buffer.  When `scm_fill_input ()' 
     is called, it just invokes `read', passing it the port's internal
     buffer and using its return value to update `pt->read_end'.

The downside with the `read' method is that it requires changes in port
implementations to take advantage of it.

Overall, I'd prefer `read', at least for the longer term.  Changing
`pt->read_buf' et al. right before calling `fill_input ()' looks to me
like a hack to pass arguments implicitly.

> But that advantage is also (in some cases) the disadvantage of having
> to do an additional memcpy of the data.

There's no additional memcpy, as mentioned above.

> I don't think it's too bad for callers to be fiddling with read_buf -
> although we could encapsulate that a bit more clearly.

Indeed.  :-)

Thanks,
Ludovic.

Re: [PATCH] Add a `read' method for ports

[Neil Jerram]

[-- Attachment #1: Type: text/plain, Size: 3250 bytes --]

Hi Ludovic,

I hope you don't mind if I continue to pursue this a little...

2008/6/12 Ludovic Courtès <ludo@gnu.org>:
> Hi Neil,
>
> "Neil Jerram" <neiljerram@googlemail.com> writes:
>
>> So, one way of solving this would be for an `unbuffered' port
>> temporarily to get a buffer of the right size, and then to use the
>> fill_input logic as it currently stands.  That would achieve your
>> objective of mapping onto a single N-byte read(2) call.
>
> Surely this would work, but I can think of several issues:
>
>  1. Thread safety.  OK, ports are not thread-safe and it'd take more
>     than this to make them thread-safe, but passing arguments
>     explicitly (as the BUFFER and SIZE arguments of `read') is a step
>     forward, while mutating the buffer pointer and size in the port is
>     a step backward.

Agreed in theory, but I don't think it's a significant step backwards,
because ports are fundamentally not thread-safe (as you've already
said above), and already have lots of implicit arguments.

>  2. Performance.  Another case where the `read' method improves on
>     performance: when more data is requested than can fit in the port's
>     buffer, the port buffer is completely bypassed.  This allows the
>     data to be read in one shot, avoiding memcpys from the port's
>     buffer to the caller's buffer.

Agreed.  Yesterday I was worried about possible confusion from
bypassing a port's read buffer at a time when the read buffer has some
data in it.  But in fact this is not a problem, because
scm_fill_input() should be (and in practice is) only called when the
port's read buffer is empty.

>  3. Aesthetics.  The `read' method looks more natural to me, since it's
>     just like `read(2)'.  Also, as mentioned earlier, `read' methods
>     don't have to be aware of the port's internal buffer: they just
>     need to fill in the caller-supplied buffer.  When `scm_fill_input ()'
>     is called, it just invokes `read', passing it the port's internal
>     buffer and using its return value to update `pt->read_end'.

Aesthetics are always tricky.  I accept your point here, but against
that we have to count

- the cost of adding a new method to libguile's port definition

- the benefit of (some) existing fill_input code being able to work
with caller-supplied buffers automatically.  (I say "some", because
some fill_input implementations may make assumptions about the read
buffer size and so not automatically take advantage of a larger
caller-supplied buffer.  But at least the important fport case will
work automatically.)

>> But that advantage is also (in some cases) the disadvantage of having
>> to do an additional memcpy of the data.
>
> There's no additional memcpy, as mentioned above.

Indeed, my mistake.

Based on all this, I'd like to update my suggestion so that

- scm_fill_input_buf always uses the caller's buffer

- the remaining scm_fill_input_buf logic is inlined into scm_c_read
(since that is the only place that uses scm_fill_input_buf, and
inlining it permits some further simplifications).

New patch (smaller and simpler) is attached; please let me know if
this sways your opinion at all!

         Neil

[-- Attachment #2: scm_fill_input_buf.patch --]
[-- Type: application/octet-stream, Size: 3643 bytes --]

diff --git a/libguile/ports.c b/libguile/ports.c
index b97c826..f9f7a2d 100644
--- a/libguile/ports.c
+++ b/libguile/ports.c
@@ -1042,6 +1042,8 @@ scm_c_read (SCM port, void *buffer, size_t size)
 {
   scm_t_port *pt;
   size_t n_read = 0, n_available;
+  unsigned char *saved_buf;
+  size_t saved_buf_size;
 
   SCM_VALIDATE_OPINPORT (1, port);
 
@@ -1052,35 +1054,43 @@ scm_c_read (SCM port, void *buffer, size_t size)
   if (pt->rw_random)
     pt->rw_active = SCM_PORT_READ;
 
-  if (SCM_READ_BUFFER_EMPTY_P (pt))
-    {
-      if (scm_fill_input (port) == EOF)
-	return 0;
-    }
-  
-  n_available = pt->read_end - pt->read_pos;
-  
-  while (n_available < size)
+  /* Take bytes first from the port's read buffer. */
+  if (pt->read_pos < pt->read_end)
     {
+      n_available = min (size, pt->read_end - pt->read_pos);
       memcpy (buffer, pt->read_pos, n_available);
       buffer = (char *) buffer + n_available;
       pt->read_pos += n_available;
       n_read += n_available;
-      
-      if (SCM_READ_BUFFER_EMPTY_P (pt))
-	{
-	  if (scm_fill_input (port) == EOF)
-	    return n_read;
-	}
-
       size -= n_available;
-      n_available = pt->read_end - pt->read_pos;
     }
 
-  memcpy (buffer, pt->read_pos, size);
-  pt->read_pos += size;
+  /* Now we will call scm_fill_input repeatedly until we have read the
+     requested number of bytes.  (Note that a single scm_fill_input
+     call does not guarantee to fill the whole of the port's read
+     buffer.)  For these calls, since we already have a buffer here to
+     read into, we bypass the port's own read buffer (if it has one),
+     by saving it off and modifying the port structure to point to our
+     own buffer. */
+  saved_buf = pt->read_buf;
+  saved_buf_size = pt->read_buf_size;
+  pt->read_pos = pt->read_buf = pt->read_end = buffer;
+  pt->read_buf_size = size;
+
+  /* Call scm_fill_input until we have all the bytes that we need, or
+     we hit EOF. */
+  while (pt->read_buf_size && (scm_fill_input (port) != EOF))
+    {
+      pt->read_buf_size -= (pt->read_end - pt->read_pos);
+      pt->read_pos = pt->read_buf = pt->read_end;
+    }
+  n_read += pt->read_buf - (unsigned char *) buffer;
+
+  /* Reinstate the port's normal buffer. */
+  pt->read_pos = pt->read_buf = pt->read_end = saved_buf;
+  pt->read_buf_size = saved_buf_size;
 
-  return n_read + size;
+  return n_read;
 }
 #undef FUNC_NAME
 
diff --git a/libguile/srfi-4.c b/libguile/srfi-4.c
index 7d22f8b..a01f86e 100644
--- a/libguile/srfi-4.c
+++ b/libguile/srfi-4.c
@@ -886,38 +886,11 @@ SCM_DEFINE (scm_uniform_vector_read_x, "uniform-vector-read!", 1, 3, 0,
 
   if (SCM_NIMP (port_or_fd))
     {
-      scm_t_port *pt = SCM_PTAB_ENTRY (port_or_fd);
-
-      if (pt->rw_active == SCM_PORT_WRITE)
-	scm_flush (port_or_fd);
-
       ans = cend - cstart;
-      while (remaining > 0)
-	{
-	  if (pt->read_pos < pt->read_end)
-	    {
-	      size_t to_copy = min (pt->read_end - pt->read_pos,
-				    remaining);
-	      
-	      memcpy (base + off, pt->read_pos, to_copy);
-	      pt->read_pos += to_copy;
-	      remaining -= to_copy;
-	      off += to_copy;
-	    }
-	  else
-	    {
-	      if (scm_fill_input (port_or_fd) == EOF)
-		{
-		  if (remaining % sz != 0)
-		    SCM_MISC_ERROR ("unexpected EOF", SCM_EOL);
-		  ans -= remaining / sz;
-		  break;
-		}
-	    }
-	}
-      
-      if (pt->rw_random)
-	pt->rw_active = SCM_PORT_READ;
+      remaining -= scm_c_read (port_or_fd, base + off, remaining);
+      if (remaining % sz != 0)
+        SCM_MISC_ERROR ("unexpected EOF", SCM_EOL);
+      ans -= remaining / sz;
     }
   else /* file descriptor.  */
     {

Re: [PATCH] Add a `read' method for ports

[Ludovic Courtès]

Hi Neil,

Sorry for the delay, I was off-line last week.

"Neil Jerram" <neiljerram@googlemail.com> writes:

> Agreed.  Yesterday I was worried about possible confusion from
> bypassing a port's read buffer at a time when the read buffer has some
> data in it.  But in fact this is not a problem, because
> scm_fill_input() should be (and in practice is) only called when the
> port's read buffer is empty.

Right.  Putting an assertion in there to make sure can't hurt.

> Aesthetics are always tricky.  I accept your point here, but against
> that we have to count
>
> - the cost of adding a new method to libguile's port definition
>
> - the benefit of (some) existing fill_input code being able to work
> with caller-supplied buffers automatically.  (I say "some", because
> some fill_input implementations may make assumptions about the read
> buffer size and so not automatically take advantage of a larger
> caller-supplied buffer.  But at least the important fport case will
> work automatically.)

Yeah, right.

These arguments make a lot of sense in 1.8.  In the longer run, though,
I'd prefer to see `fill_input' superseded by a `read' method akin to
what I proposed.  What do you think?

> Based on all this, I'd like to update my suggestion so that
>
> - scm_fill_input_buf always uses the caller's buffer
>
> - the remaining scm_fill_input_buf logic is inlined into scm_c_read
> (since that is the only place that uses scm_fill_input_buf, and
> inlining it permits some further simplifications).

OK.  The issue is that if the caller-supplied buffer is smaller than the
port's buffer, we end up reading less data than we'd otherwise do.  I
think this scenario should be addressed (by comparing `read_buf_size'
and `size'), as was the case in my proposal.

> +  /* Now we will call scm_fill_input repeatedly until we have read the
> +     requested number of bytes.  (Note that a single scm_fill_input
> +     call does not guarantee to fill the whole of the port's read
> +     buffer.)  For these calls, since we already have a buffer here to
> +     read into, we bypass the port's own read buffer (if it has one),
> +     by saving it off and modifying the port structure to point to our
> +     own buffer. */
> +  saved_buf = pt->read_buf;
> +  saved_buf_size = pt->read_buf_size;
> +  pt->read_pos = pt->read_buf = pt->read_end = buffer;
> +  pt->read_buf_size = size;

`fill_input' methods can raise an exception, as is the case with soft
parts.  Thus, this code should be protected by a `dynwind' that
restores the port's buffer and size.

> +      remaining -= scm_c_read (port_or_fd, base + off, remaining);
> +      if (remaining % sz != 0)
> +        SCM_MISC_ERROR ("unexpected EOF", SCM_EOL);
> +      ans -= remaining / sz;

Likewise, `scm_c_read ()' might raise an exception, so we may need a
`dynwind' here (my original patch for `uniform-vector-read!' did that).

Other than that, I'm OK with your patch.

Thanks,
Ludovic.

Re: [PATCH] Add a `read' method for ports

[Ludovic Courtès]

Hi Neil,

ludo@gnu.org (Ludovic Courtès) writes:

> Other than that, I'm OK with your patch.

Can you update your patch?  (Note how I subtly moved the burden from me
to you.  :-))

Thanks,
Ludo'.

Re: [PATCH] Add a `read' method for ports

[Neil Jerram]

2008/7/15 Ludovic Courtès <ludo@gnu.org>:
> Hi Neil,
>
> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Other than that, I'm OK with your patch.
>
> Can you update your patch?  (Note how I subtly moved the burden from me
> to you.  :-))

Yes, planned for this evening!

     Neil

Re: [PATCH] Add a `read' method for ports

[Neil Jerram]

[-- Attachment #1: Type: text/plain, Size: 3570 bytes --]

2008/6/24 Ludovic Courtès <ludo@gnu.org>:
> Hi Neil,
>
> Sorry for the delay, I was off-line last week.

No problem!

> "Neil Jerram" <neiljerram@googlemail.com> writes:
>
>> Agreed.  Yesterday I was worried about possible confusion from
>> bypassing a port's read buffer at a time when the read buffer has some
>> data in it.  But in fact this is not a problem, because
>> scm_fill_input() should be (and in practice is) only called when the
>> port's read buffer is empty.
>
> Right.  Putting an assertion in there to make sure can't hurt.

OK, done.

> These arguments make a lot of sense in 1.8.  In the longer run, though,
> I'd prefer to see `fill_input' superseded by a `read' method akin to
> what I proposed.  What do you think?

To be honest, I don't yet see a clear need for it.  If there is no
drawback to what we have now, and there would be a compatibility cost
to introducing a new read method, then why do that?

>> Based on all this, I'd like to update my suggestion so that
>>
>> - scm_fill_input_buf always uses the caller's buffer
>>
>> - the remaining scm_fill_input_buf logic is inlined into scm_c_read
>> (since that is the only place that uses scm_fill_input_buf, and
>> inlining it permits some further simplifications).
>
> OK.  The issue is that if the caller-supplied buffer is smaller than the
> port's buffer, we end up reading less data than we'd otherwise do.  I
> think this scenario should be addressed (by comparing `read_buf_size'
> and `size'), as was the case in my proposal.

True in theory, but I feel very confident that this will never matter
in practice, and hence that we don't need to have code for this.

Confidence is based on experience from my day job, which is in network
protocols - where buffered reads are a very common operation.  On the
other hand, it is possible I've missed some other application where
different patterns/rules apply, so please let me know if you have
specific counterexamples in mind.

>> +  /* Now we will call scm_fill_input repeatedly until we have read the
>> +     requested number of bytes.  (Note that a single scm_fill_input
>> +     call does not guarantee to fill the whole of the port's read
>> +     buffer.)  For these calls, since we already have a buffer here to
>> +     read into, we bypass the port's own read buffer (if it has one),
>> +     by saving it off and modifying the port structure to point to our
>> +     own buffer. */
>> +  saved_buf = pt->read_buf;
>> +  saved_buf_size = pt->read_buf_size;
>> +  pt->read_pos = pt->read_buf = pt->read_end = buffer;
>> +  pt->read_buf_size = size;
>
> `fill_input' methods can raise an exception, as is the case with soft
> parts.  Thus, this code should be protected by a `dynwind' that
> restores the port's buffer and size.

Good point.  I've added this.

>> +      remaining -= scm_c_read (port_or_fd, base + off, remaining);
>> +      if (remaining % sz != 0)
>> +        SCM_MISC_ERROR ("unexpected EOF", SCM_EOL);
>> +      ans -= remaining / sz;
>
> Likewise, `scm_c_read ()' might raise an exception, so we may need a
> `dynwind' here (my original patch for `uniform-vector-read!' did that).

This is covered by the dynwind inside scm_c_read, isn't it?

Updated patch is attached.  As well as taking a look, could you check
that it really improves performance in the cases that motivated this?
I tried running the read.bm benchmarks, but in those cases it didn't
seem to make much difference; is that expected?

Regards,
        Neil

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: unbuffered-read.diff --]
[-- Type: text/x-patch; name=unbuffered-read.diff, Size: 5735 bytes --]

diff --git a/libguile/ChangeLog b/libguile/ChangeLog
index 5c30574..d1bf0d9 100644
--- a/libguile/ChangeLog
+++ b/libguile/ChangeLog
@@ -1,3 +1,15 @@
+2008-07-15  Neil Jerram  <neil@ossau.uklinux.net>
+
+	* srfi-4.c (scm_uniform_vector_read_x): Use scm_c_read, instead of
+	equivalent code here.
+
+	* ports.c (scm_fill_input): Add assertion that read buffer is
+	empty when called.
+	(port_and_swap_buffer, swap_buffer): New, for...
+	(scm_c_read): Use caller's buffer for reading, to avoid making N
+	1-byte low-level read calls, in the case where the port is
+	unbuffered (or has a very small buffer).
+
 2008-07-05  Ludovic Courtès  <ludo@gnu.org>
 
 	* strings.c (scm_c_symbol_length): New function.
diff --git a/libguile/ports.c b/libguile/ports.c
index b97c826..9563001 100644
--- a/libguile/ports.c
+++ b/libguile/ports.c
@@ -28,6 +28,7 @@
 #include <stdio.h>
 #include <errno.h>
 #include <fcntl.h>  /* for chsize on mingw */
+#include <assert.h>
 
 #include "libguile/_scm.h"
 #include "libguile/async.h"
@@ -974,6 +975,8 @@ scm_fill_input (SCM port)
 {
   scm_t_port *pt = SCM_PTAB_ENTRY (port);
 
+  assert (pt->read_pos == pt->read_end);
+
   if (pt->read_buf == pt->putback_buf)
     {
       /* finished reading put-back chars.  */
@@ -1036,12 +1039,35 @@ scm_lfwrite (const char *ptr, size_t size, SCM port)
  *
  * Warning: Doesn't update port line and column counts!  */
 
+struct port_and_swap_buffer {
+  scm_t_port *pt;
+  unsigned char *buffer;
+  size_t size;
+};
+
+static void
+swap_buffer (void *data)
+{
+  struct port_and_swap_buffer *psb = (struct port_and_swap_buffer *) data;
+  unsigned char *old_buf = psb->pt->read_buf;
+  size_t old_size = psb->pt->read_buf_size;
+
+  /* Make the port use (buffer, size) from the struct. */
+  psb->pt->read_pos = psb->pt->read_buf = psb->pt->read_end = psb->buffer;
+  psb->pt->read_buf_size = psb->size;
+
+  /* Save the port's old (buffer, size) in the struct. */
+  psb->buffer = old_buf;
+  psb->size = old_size;
+}
+
 size_t
 scm_c_read (SCM port, void *buffer, size_t size)
 #define FUNC_NAME "scm_c_read"
 {
   scm_t_port *pt;
   size_t n_read = 0, n_available;
+  struct port_and_swap_buffer psb;
 
   SCM_VALIDATE_OPINPORT (1, port);
 
@@ -1052,35 +1078,48 @@ scm_c_read (SCM port, void *buffer, size_t size)
   if (pt->rw_random)
     pt->rw_active = SCM_PORT_READ;
 
-  if (SCM_READ_BUFFER_EMPTY_P (pt))
-    {
-      if (scm_fill_input (port) == EOF)
-	return 0;
-    }
-  
-  n_available = pt->read_end - pt->read_pos;
-  
-  while (n_available < size)
+  /* Take bytes first from the port's read buffer. */
+  if (pt->read_pos < pt->read_end)
     {
+      n_available = min (size, pt->read_end - pt->read_pos);
       memcpy (buffer, pt->read_pos, n_available);
       buffer = (char *) buffer + n_available;
       pt->read_pos += n_available;
       n_read += n_available;
-      
-      if (SCM_READ_BUFFER_EMPTY_P (pt))
-	{
-	  if (scm_fill_input (port) == EOF)
-	    return n_read;
-	}
-
       size -= n_available;
-      n_available = pt->read_end - pt->read_pos;
     }
 
-  memcpy (buffer, pt->read_pos, size);
-  pt->read_pos += size;
+  /* Now we will call scm_fill_input repeatedly until we have read the
+     requested number of bytes.  (Note that a single scm_fill_input
+     call does not guarantee to fill the whole of the port's read
+     buffer.)  For these calls, since we already have a buffer here to
+     read into, we bypass the port's own read buffer (if it has one),
+     by saving it off and modifying the port structure to point to our
+     own buffer.
+
+     We need to make sure that the port's normal buffer is reinstated
+     in case one of the scm_fill_input () calls throws an exception;
+     we use the scm_dynwind_* API to achieve that. */
+  psb.pt = pt;
+  psb.buffer = buffer;
+  psb.size = size;
+  scm_dynwind_begin (SCM_F_DYNWIND_REWINDABLE);
+  scm_dynwind_rewind_handler (swap_buffer, &psb, SCM_F_WIND_EXPLICITLY);
+  scm_dynwind_unwind_handler (swap_buffer, &psb, SCM_F_WIND_EXPLICITLY);
+
+  /* Call scm_fill_input until we have all the bytes that we need, or
+     we hit EOF. */
+  while (pt->read_buf_size && (scm_fill_input (port) != EOF))
+    {
+      pt->read_buf_size -= (pt->read_end - pt->read_pos);
+      pt->read_pos = pt->read_buf = pt->read_end;
+    }
+  n_read += pt->read_buf - (unsigned char *) buffer;
+
+  /* Reinstate the port's normal buffer. */
+  scm_dynwind_end ();
 
-  return n_read + size;
+  return n_read;
 }
 #undef FUNC_NAME
 
diff --git a/libguile/srfi-4.c b/libguile/srfi-4.c
index 7d22f8b..a01f86e 100644
--- a/libguile/srfi-4.c
+++ b/libguile/srfi-4.c
@@ -886,38 +886,11 @@ SCM_DEFINE (scm_uniform_vector_read_x, "uniform-vector-read!", 1, 3, 0,
 
   if (SCM_NIMP (port_or_fd))
     {
-      scm_t_port *pt = SCM_PTAB_ENTRY (port_or_fd);
-
-      if (pt->rw_active == SCM_PORT_WRITE)
-	scm_flush (port_or_fd);
-
       ans = cend - cstart;
-      while (remaining > 0)
-	{
-	  if (pt->read_pos < pt->read_end)
-	    {
-	      size_t to_copy = min (pt->read_end - pt->read_pos,
-				    remaining);
-	      
-	      memcpy (base + off, pt->read_pos, to_copy);
-	      pt->read_pos += to_copy;
-	      remaining -= to_copy;
-	      off += to_copy;
-	    }
-	  else
-	    {
-	      if (scm_fill_input (port_or_fd) == EOF)
-		{
-		  if (remaining % sz != 0)
-		    SCM_MISC_ERROR ("unexpected EOF", SCM_EOL);
-		  ans -= remaining / sz;
-		  break;
-		}
-	    }
-	}
-      
-      if (pt->rw_random)
-	pt->rw_active = SCM_PORT_READ;
+      remaining -= scm_c_read (port_or_fd, base + off, remaining);
+      if (remaining % sz != 0)
+        SCM_MISC_ERROR ("unexpected EOF", SCM_EOL);
+      ans -= remaining / sz;
     }
   else /* file descriptor.  */
     {

Re: [PATCH] Add a `read' method for ports

[Ludovic Courtès]

Good evening!

"Neil Jerram" <neiljerram@googlemail.com> writes:

> 2008/6/24 Ludovic Courtès <ludo@gnu.org>:

>> These arguments make a lot of sense in 1.8.  In the longer run, though,
>> I'd prefer to see `fill_input' superseded by a `read' method akin to
>> what I proposed.  What do you think?
>
> To be honest, I don't yet see a clear need for it.  If there is no
> drawback to what we have now, and there would be a compatibility cost
> to introducing a new read method, then why do that?

Well, mostly for robustness (passing arguments explicitly) and
aesthetics (removing part of the burden of the internal buffer
management from port implementors, following the "rule of least
surprise").  Also, the port API is currently undocumented, and I
wouldn't feel comfortable documenting `fill_input ()'.

That said, it would indeed have a compatibility cost, which may be a
stronger argument given the current state of affairs.

>> OK.  The issue is that if the caller-supplied buffer is smaller than the
>> port's buffer, we end up reading less data than we'd otherwise do.  I
>> think this scenario should be addressed (by comparing `read_buf_size'
>> and `size'), as was the case in my proposal.
>
> True in theory, but I feel very confident that this will never matter
> in practice, and hence that we don't need to have code for this.

Yeah, the performance difference may not be noticeable.

>>> +      remaining -= scm_c_read (port_or_fd, base + off, remaining);
>>> +      if (remaining % sz != 0)
>>> +        SCM_MISC_ERROR ("unexpected EOF", SCM_EOL);
>>> +      ans -= remaining / sz;
>>
>> Likewise, `scm_c_read ()' might raise an exception, so we may need a
>> `dynwind' here (my original patch for `uniform-vector-read!' did that).
>
> This is covered by the dynwind inside scm_c_read, isn't it?

No, HANDLE must be released here (see my original patch).

My post also contained a `uniform-vector-read!' benchmark, which showed
a noticeable performance improvement on unbuffered ports.  Could you try
(and commit) that also?


And now for some gratuitous (but friendly) nitpicking...

> +struct port_and_swap_buffer {

Please follow the GNU style.  :-)

A small comment above might be nice.

> +static void
> +swap_buffer (void *data)

Likewise, a comment would help.

> +  /* Reinstate the port's normal buffer. */

I like adding a space after periods at the end of a sentence.

Thanks,
Ludo'.

Re: [PATCH] Add a `read' method for ports

[Ludovic Courtès]

Hi Neil,

Can you consider updating and committing your patch to 1.8?  It's one of
the things that I think should go in before we release 1.8.6.

Thanks,
Ludo'.

Re: [PATCH] Add a `read' method for ports

[Neil Jerram]

Hopefully nearly finishing this off now...!

2008/7/16 Ludovic Courtès <ludo@gnu.org>:

>>> Likewise, `scm_c_read ()' might raise an exception, so we may need a
>>> `dynwind' here (my original patch for `uniform-vector-read!' did that).
>>
>> This is covered by the dynwind inside scm_c_read, isn't it?
>
> No, HANDLE must be released here (see my original patch).

I don't think that's right; see "Accessing Arrays from C" in the
manual, especially:

"
   You must take care to always unreserve an array after reserving it,
also in the presence of non-local exits.  To simplify this, reserving
and unreserving work like a dynwind context (*note Dynamic Wind::): a
call to `scm_array_get_handle' can be thought of as beginning a dynwind
context and `scm_array_handle_release' as ending it.  When a non-local
exit happens between these two calls, the array is implicitely
unreserved.

   That is, you need to properly pair reserving and unreserving in your
code, but you don't need to worry about non-local exits.
"

Now, as it happens, the code doesn't actually implement what the
manual says, and in fact scm_array_handle_release() is currently a
no-op!  But I believe the manual's intention is sensible, so it think
I think we should commit this patch as-is for now, and raise a bug to
track the fact that the array/handle API isn't fully implemented.

What do you think?

> My post also contained a `uniform-vector-read!' benchmark, which showed
> a noticeable performance improvement on unbuffered ports.  Could you try
> (and commit) that also?

Yes, I'll do that shortly (before and separately from the libguile
code changes).

> And now for some gratuitous (but friendly) nitpicking...
>
>> +struct port_and_swap_buffer {
>
> Please follow the GNU style.  :-)

Well, they say "The open-brace that starts a struct body can go in
column one if you find it useful to treat that definition as a
defun.".  Is that what you meant?  In this case I don't think it's
important to "treat that definition as a defun" - and also there are
lots of other examples in  libguile where the opening brace is on the
same line...

> A small comment above might be nice.

Right, added:

/* This structure, and the following swap_buffer function, are used
   for temporarily swapping a port's own read buffer, and the buffer
   that the caller of scm_c_read provides. */

>> +  /* Reinstate the port's normal buffer. */
>
> I like adding a space after periods at the end of a sentence.

But there is a space there - so I don't understand!

Regards,
        Neil

Re: [PATCH] Add a `read' method for ports

[Neil Jerram]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

2008/9/15 Neil Jerram <neiljerram@googlemail.com>:
>
> 2008/7/16 Ludovic Courtès <ludo@gnu.org>:
>
>> My post also contained a `uniform-vector-read!' benchmark, which showed
>> a noticeable performance improvement on unbuffered ports.  Could you try
>> (and commit) that also?
>
> Yes, I'll do that shortly (before and separately from the libguile
> code changes).

I've attached the benchmark results before and after the libguile
changes.  Can you check that they are as you would expect?  (I assume
the important change is the reduction from 2.69 to 0.01 for the
uniform-vector-read benchmark!)

Regards,
       Neil

[-- Attachment #2: benchmarks-before.txt --]
[-- Type: text/plain, Size: 3216 bytes --]

neil@arudy:~/SW/Guile/guile-git$ ./benchmark-guile
Benchmarking /home/neil/SW/Guile/guile-git/pre-inst-guile ...
with GUILE_LOAD_PATH=/home/neil/SW/Guile/guile-git/benchmark-suite
;; running guile version 1.8.5
;; calibrating the benchmarking framework...
;; framework time per iteration: 1.23977661132813e-6
("0-reference.bm: reference benchmark for iteration counts" 330000 user 0.53 benchmark 0.120873718261719 bench/interp 0.0308737182617187 gc 0.09)
("continuations.bm: call/cc" 300 user 0.47 benchmark 0.469628067016601 bench/interp 0.259628067016602 gc 0.21)
("if.bm: if-<expr>-then-else: executing then" 330000 user 0.53 benchmark 0.120873718261719 bench/interp 0.0808737182617188 gc 0.04)
("if.bm: if-<expr>-then-else: executing else" 330000 user 0.54 benchmark 0.130873718261719 bench/interp 0.0908737182617188 gc 0.04)
("if.bm: if-<expr>-then: executing then" 330000 user 0.55 benchmark 0.140873718261719 bench/interp 0.110873718261719 gc 0.03)
("if.bm: if-<expr>-then: executing else" 330000 user 0.55 benchmark 0.140873718261719 bench/interp 0.100873718261719 gc 0.04)
("if.bm: if-<iloc>-then-else: executing then" 330000 user 0.51 benchmark 0.100873718261719 bench/interp 0.0608737182617188 gc 0.04)
("if.bm: if-<iloc>-then-else: executing else" 330000 user 0.49 benchmark 0.0808737182617188 bench/interp 0.0608737182617188 gc 0.02)
("if.bm: if-<iloc>-then: executing then" 330000 user 0.5 benchmark 0.0908737182617188 bench/interp 0.0608737182617188 gc 0.03)
("if.bm: if-<iloc>-then: executing else" 330000 user 0.5 benchmark 0.0908737182617188 bench/interp 0.0508737182617188 gc 0.04)
("if.bm: if-<bool>-then-else: executing then" 330000 user 0.48 benchmark 0.0708737182617187 bench/interp 0.0508737182617188 gc 0.02)
("if.bm: if-<bool>-then-else: executing else" 330000 user 0.49 benchmark 0.0808737182617188 bench/interp 0.0508737182617188 gc 0.03)
("if.bm: if-<bool>-then: executing then" 330000 user 0.48 benchmark 0.0708737182617187 bench/interp 0.0408737182617188 gc 0.03)
("if.bm: if-<bool>-then: executing else" 330000 user 0.49 benchmark 0.0808737182617188 bench/interp 0.0408737182617188 gc 0.04)
("logand.bm: bignum" 130000 user 0.43 benchmark 0.268829040527344 bench/interp 0.258829040527344 gc 0.01)
("read.bm: read: _IONBF" 5 user 1.85 benchmark 1.84999380111694 bench/interp 1.72999380111694 gc 0.12)
("read.bm: read: _IOLBF" 100 user 10.85 benchmark 10.8498760223389 bench/interp 9.07987602233887 gc 1.77)
("read.bm: read: _IOFBF 4096" 100 user 10.87 benchmark 10.8698760223389 bench/interp 9.05987602233887 gc 1.81)
("read.bm: read: _IOFBF 8192" 100 user 9.66 benchmark 9.65987602233887 bench/interp 8.16987602233887 gc 1.49)
("read.bm: read: _IOFBF 16384" 100 user 10.01 benchmark 10.0098760223389 bench/interp 8.49987602233887 gc 1.51)
("uniform-vector-read.bm: uniform-vector-read!: uniform-vector-write" 500 user 0.06 benchmark 0.0593801116943359 bench/interp 0.0593801116943359 gc 0.0)
("uniform-vector-read.bm: uniform-vector-read!: uniform-vector-read!" 500 user 2.69 benchmark 2.68938011169434 bench/interp 2.68938011169434 gc 0.0)
("uniform-vector-read.bm: uniform-vector-read!: string port" 5000 user 0.11 benchmark 0.103801116943359 bench/interp 0.103801116943359 gc 0.0)

[-- Attachment #3: benchmarks-after.txt --]
[-- Type: text/plain, Size: 3221 bytes --]

neil@arudy:~/SW/Guile/guile-git$ ./benchmark-guile
Benchmarking /home/neil/SW/Guile/guile-git/pre-inst-guile ...
with GUILE_LOAD_PATH=/home/neil/SW/Guile/guile-git/benchmark-suite
;; running guile version 1.8.5
;; calibrating the benchmarking framework...
;; framework time per iteration: 1.23977661132813e-6
("0-reference.bm: reference benchmark for iteration counts" 330000 user 0.54 benchmark 0.130873718261719 bench/interp 0.0508737182617188 gc 0.08)
("continuations.bm: call/cc" 300 user 0.47 benchmark 0.469628067016601 bench/interp 0.279628067016602 gc 0.19)
("if.bm: if-<expr>-then-else: executing then" 330000 user 0.54 benchmark 0.130873718261719 bench/interp 0.0708737182617187 gc 0.06)
("if.bm: if-<expr>-then-else: executing else" 330000 user 0.56 benchmark 0.150873718261719 bench/interp 0.100873718261719 gc 0.05)
("if.bm: if-<expr>-then: executing then" 330000 user 0.56 benchmark 0.150873718261719 bench/interp 0.110873718261719 gc 0.04)
("if.bm: if-<expr>-then: executing else" 330000 user 0.55 benchmark 0.140873718261719 bench/interp 0.0908737182617188 gc 0.05)
("if.bm: if-<iloc>-then-else: executing then" 330000 user 0.53 benchmark 0.120873718261719 bench/interp 0.0708737182617187 gc 0.05)
("if.bm: if-<iloc>-then-else: executing else" 330000 user 0.5 benchmark 0.0908737182617188 bench/interp 0.0608737182617188 gc 0.03)
("if.bm: if-<iloc>-then: executing then" 330000 user 0.51 benchmark 0.100873718261719 bench/interp 0.0708737182617187 gc 0.03)
("if.bm: if-<iloc>-then: executing else" 330000 user 0.5 benchmark 0.0908737182617188 bench/interp 0.0708737182617187 gc 0.02)
("if.bm: if-<bool>-then-else: executing then" 330000 user 0.49 benchmark 0.0808737182617188 bench/interp 0.0508737182617188 gc 0.03)
("if.bm: if-<bool>-then-else: executing else" 330000 user 0.49 benchmark 0.0808737182617188 bench/interp 0.0608737182617188 gc 0.02)
("if.bm: if-<bool>-then: executing then" 330000 user 0.49 benchmark 0.0808737182617188 bench/interp 0.0508737182617188 gc 0.03)
("if.bm: if-<bool>-then: executing else" 330000 user 0.49 benchmark 0.0808737182617188 bench/interp 0.0508737182617188 gc 0.03)
("logand.bm: bignum" 130000 user 0.41 benchmark 0.248829040527344 bench/interp 0.228829040527344 gc 0.02)
("read.bm: read: _IONBF" 5 user 1.76 benchmark 1.75999380111694 bench/interp 1.64999380111694 gc 0.11)
("read.bm: read: _IOLBF" 100 user 10.91 benchmark 10.9098760223389 bench/interp 9.09987602233887 gc 1.81)
("read.bm: read: _IOFBF 4096" 100 user 10.93 benchmark 10.9298760223389 bench/interp 9.14987602233887 gc 1.78)
("read.bm: read: _IOFBF 8192" 100 user 9.69 benchmark 9.68987602233887 bench/interp 8.16987602233887 gc 1.52)
("read.bm: read: _IOFBF 16384" 100 user 10.03 benchmark 10.0298760223389 bench/interp 8.46987602233887 gc 1.56)
("uniform-vector-read.bm: uniform-vector-read!: uniform-vector-write" 500 user 0.06 benchmark 0.0593801116943359 bench/interp 0.0593801116943359 gc 0.0)
("uniform-vector-read.bm: uniform-vector-read!: uniform-vector-read!" 500 user 0.01 benchmark 0.00938011169433594 bench/interp 0.00938011169433594 gc 0.0)
("uniform-vector-read.bm: uniform-vector-read!: string port" 5000 user 0.11 benchmark 0.103801116943359 bench/interp 0.103801116943359 gc 0.0)

Re: [PATCH] Add a `read' method for ports

[Ludovic Courtès]

"Neil Jerram" <neiljerram@googlemail.com> writes:

> I've attached the benchmark results before and after the libguile
> changes.  Can you check that they are as you would expect?  (I assume
> the important change is the reduction from 2.69 to 0.01 for the
> uniform-vector-read benchmark!)

Yes, perfect!

Thanks,
Ludo'.

Re: [PATCH] Add a `read' method for ports

[Ludovic Courtès]

Hi Neil,

"Neil Jerram" <neiljerram@googlemail.com> writes:

> Hopefully nearly finishing this off now...!

Great, thanks!

> 2008/7/16 Ludovic Courtès <ludo@gnu.org>:

>> No, HANDLE must be released here (see my original patch).
>
> I don't think that's right; see "Accessing Arrays from C" in the
> manual, especially:

[...]

>    That is, you need to properly pair reserving and unreserving in your
> code, but you don't need to worry about non-local exits.
> "
>
> Now, as it happens, the code doesn't actually implement what the
> manual says, and in fact scm_array_handle_release() is currently a
> no-op!  But I believe the manual's intention is sensible, so it think
> I think we should commit this patch as-is for now, and raise a bug to
> track the fact that the array/handle API isn't fully implemented.
>
> What do you think?

I'd prefer fixing the manual rather than `scm_array_get_handle ()',
because (1) setting up a dynwind is "costly" (involves some consing),
and (2) I don't know of any other function that does a dynwind behind
the scenes (IOW, let's not break the "rule of least surprise").

OTOH, it may be the case that people have been relied on the described
behavior, in which case it would be wiser to fix `scm_array_get_handle ()' 
to conform to the manual...

>>> +struct port_and_swap_buffer {
>>
>> Please follow the GNU style.  :-)
>
> Well, they say "The open-brace that starts a struct body can go in
> column one if you find it useful to treat that definition as a
> defun.".  Is that what you meant?

Yes, but well...

>> A small comment above might be nice.
>
> Right, added:
>
> /* This structure, and the following swap_buffer function, are used
>    for temporarily swapping a port's own read buffer, and the buffer
>    that the caller of scm_c_read provides. */

Great.

>>> +  /* Reinstate the port's normal buffer. */
>>
>> I like adding a space after periods at the end of a sentence.
>
> But there is a space there - so I don't understand!

I meant two spaces, to distinguish periods that end a sentence from
abbreviations.

Thanks,
Ludo'.

Re: [PATCH] Add a `read' method for ports

[Neil Jerram]

2008/9/15 Ludovic Courtès <ludo@gnu.org>:
> Hi Neil,
>
> "Neil Jerram" <neiljerram@googlemail.com> writes:
>
>> Hopefully nearly finishing this off now...!
>
> Great, thanks!

OK, I've committed this now, with...

>>>> +struct port_and_swap_buffer {
>>>
>>> Please follow the GNU style.  :-)
>>
>> Well, they say "The open-brace that starts a struct body can go in
>> column one if you find it useful to treat that definition as a
>> defun.".  Is that what you meant?
>
> Yes, but well...

...this opening brace moved onto the next line, but...

>>>> +  /* Reinstate the port's normal buffer. */
>>>
>>> I like adding a space after periods at the end of a sentence.
>>
>> But there is a space there - so I don't understand!
>
> I meant two spaces, to distinguish periods that end a sentence from
> abbreviations.

...without an extra space here - simply because there are tons of
existing comments that end with ". */", so it doesn't seem right to
put two spaces at the end of one new comment.

I think we can treat the array handle release point separately, and
I'll follow up on that in a separate email.

Regards,
        Neil

Re: [PATCH] Add a `read' method for ports

[Ludovic Courtès]

Hi Neil,

"Neil Jerram" <neiljerram@googlemail.com> writes:

> OK, I've committed this now, with...

Thanks!

Ludo'.