Add `scm_std_read ()'

Add `scm_std_read ()'

[Ludovic Courtès]

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

Hi,

This is roughly a followup to:

  http://thread.gmane.org/gmane.lisp.guile.devel/6549/focus=6613

The attached patch adds an `scm_std_read ()' call, which is to read(2)
what `scm_std_select ()' is to select(2).  It changes `fport_fill_input'
to use it, which removes the call to select(2) that precedes each
read(2) call (!).

Using the same "I/O benchmark" as in my previous message, and compared
to Guile with the inlined `scm_getc' patch, it yields an additional 5%
speedup when reading by 4096-octet blocks, and a negligible improvement
when using smaller buffer sizes.

OK to apply?

Thanks,
Ludovic.


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

From f305530cbd31b4a0963f40ed9a7dd27a94aa05a8 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Mon, 14 Apr 2008 22:52:37 +0200
Subject: [PATCH] Add `scm_std_read ()', use it in `fport_fill_input ()'.

---
 libguile/fports.c  |   32 +-------------------------------
 libguile/threads.c |   24 +++++++++++++++++++++++-
 libguile/threads.h |    2 ++
 3 files changed, 26 insertions(+), 32 deletions(-)

diff --git a/libguile/fports.c b/libguile/fports.c
index efbd278..7817808 100644
--- a/libguile/fports.c
+++ b/libguile/fports.c
@@ -553,33 +553,6 @@ fport_print (SCM exp, SCM port, scm_print_state *pstate SCM_UNUSED)
   return 1;
 }
 
-#ifndef __MINGW32__
-/* thread-local block for input on fport's fdes.  */
-static void
-fport_wait_for_input (SCM port)
-{
-  int fdes = SCM_FSTREAM (port)->fdes;
-
-  if (!fport_input_waiting (port))
-    {
-      int n;
-      SELECT_TYPE readfds;
-      int flags = fcntl (fdes, F_GETFL);
-
-      if (flags == -1)
-	scm_syserror ("scm_fdes_wait_for_input");
-      if (!(flags & O_NONBLOCK))
-	do
-	  {
-	    FD_ZERO (&readfds);
-	    FD_SET (fdes, &readfds);
-	    n = scm_std_select (fdes + 1, &readfds, NULL, NULL, NULL);
-	  }
-	while (n == -1 && errno == EINTR);
-    }
-}
-#endif /* !__MINGW32__ */
-
 static void fport_flush (SCM port);
 
 /* fill a port's read-buffer with a single read.  returns the first
@@ -591,10 +564,7 @@ fport_fill_input (SCM port)
   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));
+  count = scm_std_read (fp->fdes, pt->read_buf, pt->read_buf_size);
   if (count == -1)
     scm_syserror ("fport_fill_input");
   if (count == 0)
diff --git a/libguile/threads.c b/libguile/threads.c
index 68c5f79..84aa5ea 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -1690,7 +1690,8 @@ scm_threads_mark_stacks (void)
   SCM_MARK_BACKING_STORE ();
 }
 
-/*** Select */
+\f
+/* Convenience API for I/O that properly leaves guile mode when blocking.  */
 
 int
 scm_std_select (int nfds,
@@ -1741,6 +1742,27 @@ scm_std_select (int nfds,
   return res;
 }
 
+ssize_t
+scm_std_read (int fd, void *buf, size_t size)
+{
+  int eno;
+  ssize_t count;
+  scm_t_guile_ticket ticket;
+
+  SCM_TICK;
+
+  ticket = scm_leave_guile ();
+
+  SCM_SYSCALL (count = read (fd, buf, size));
+  eno = errno;
+
+  scm_enter_guile (ticket);
+
+  errno = eno;
+  return count;
+}
+
+\f
 /* Convenience API for blocking while in guile mode. */
 
 #if SCM_USE_PTHREAD_THREADS
diff --git a/libguile/threads.h b/libguile/threads.h
index e1944a5..aac6b0b 100644
--- a/libguile/threads.h
+++ b/libguile/threads.h
@@ -224,6 +224,8 @@ SCM_API int scm_pthread_cond_timedwait (pthread_cond_t *cond,
 
 SCM_API unsigned int scm_std_sleep (unsigned int);
 SCM_API unsigned long scm_std_usleep (unsigned long);
+SCM_API ssize_t scm_std_read (int, void *, size_t);
+
 
 #endif  /* SCM_THREADS_H */
 
-- 
1.5.5


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

From 823a559a47866bd6dbbe93161938581b32b050a0 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Tue, 15 Apr 2008 09:34:29 +0200
Subject: [PATCH] Document `scm_std_read ()'.

---
 doc/ref/api-scheduling.texi |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/doc/ref/api-scheduling.texi b/doc/ref/api-scheduling.texi
index ec136fb..9916e7e 100644
--- a/doc/ref/api-scheduling.texi
+++ b/doc/ref/api-scheduling.texi
@@ -1,6 +1,6 @@
 @c -*-texinfo-*-
 @c This is part of the GNU Guile Reference Manual.
-@c Copyright (C)  1996, 1997, 2000, 2001, 2002, 2003, 2004, 2007
+@c Copyright (C)  1996, 1997, 2000, 2001, 2002, 2003, 2004, 2007, 2008
 @c   Free Software Foundation, Inc.
 @c See the file guile.texi for copying conditions.
 
@@ -583,9 +583,12 @@ leaves guile mode while waiting for the condition variable.
 @end deftypefn
 
 @deftypefn {C Function} int scm_std_select (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, struct timeval *timeout)
-Like @code{select} but leaves guile mode while waiting.  Also, the
-delivery of a system async causes this function to be interrupted with
-error code @code{EINTR}.
+@deftypefnx {C Function} ssize_t scm_std_read (int fd, void *buffer, size_t size)
+Like the C library @code{select} and @code{read} (@pxref{Low-Level I/O,
+Low-Level Input/Output,, libc, The GNU C Library Reference Manual}),
+respectively, but leaves guile mode while waiting.  Also, the delivery
+of a system async causes this function to be interrupted with error code
+@code{EINTR}.
 @end deftypefn
 
 @deftypefn {C Function} {unsigned int} scm_std_sleep ({unsigned int} seconds)
-- 
1.5.5

Re: Add `scm_std_read ()'

[Neil Jerram]

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

> Hi,
>
> This is roughly a followup to:
>
>   http://thread.gmane.org/gmane.lisp.guile.devel/6549/focus=6613
>
> The attached patch adds an `scm_std_read ()' call, which is to read(2)
> what `scm_std_select ()' is to select(2).  It changes `fport_fill_input'
> to use it, which removes the call to select(2) that precedes each
> read(2) call (!).
>
> Using the same "I/O benchmark" as in my previous message, and compared
> to Guile with the inlined `scm_getc' patch, it yields an additional 5%
> speedup when reading by 4096-octet blocks, and a negligible improvement
> when using smaller buffer sizes.
>
> OK to apply?

I'm not sure.  There are two cases which previously didn't leave and
re-enter guile mode, and now do: (i) where there is already input
available (fport_input_waiting), (ii) where the fd is non-blocking.

Leaving and re-entering guile mode feels quite expensive to me; it's
at least locking and unlocking a mutex.  I'm surprised that doesn't
outweigh the gain of not calling select() a couple of times.  Are you
sure that it does?

Regards,
    Neil

Re: Add `scm_std_read ()'

[Ludovic Courtès]

Hi,

Neil Jerram <neil@ossau.uklinux.net> writes:

> I'm not sure.  There are two cases which previously didn't leave and
> re-enter guile mode, and now do: (i) where there is already input
> available (fport_input_waiting), (ii) where the fd is non-blocking.

Right, good point.

> Leaving and re-entering guile mode feels quite expensive to me; it's
> at least locking and unlocking a mutex.  I'm surprised that doesn't
> outweigh the gain of not calling select() a couple of times.  Are you
> sure that it does?

Looking more closely at the output of the `read.bm' benchmark, we see
this (excerpt):

  - Without `scm_std_read'

    ("read.bm: read: _IONBF" 5 total 6.69 user 1.84 system 4.84
    ("read.bm: read: _IOFBF 4096" 100 total 3.13 user 3.05 system 0.08

  - With `scm_std_read'

    ("read.bm: read: _IONBF" 5 total 4.41 user 2.22 system 2.21
    ("read.bm: read: _IOFBF 4096" 100 total 3.11 user 3.07 system 0.05

This shows that less time is spent in the kernel, especially in the
unbuffered case (as is the case with sockets), leading to a noticeable
improvement in the unbuffered case.  OTOH, part of the system overhead
is shifted to user-level in the buffered case, leading to little or no
improvement.

Conclusion: even if it's nice to improve the unbuffered case given the
current state of affairs with sockets, it probably needs some more
thought to benefit the more general case as well.

Thanks,
Ludovic.