Re: [PATCH] Move slow path out of 'scm_get_byte_or_eof' et al

[Mark H Weaver]

Mike Gran <address@hidden> writes:

> I can't imagine any possible way that this patch could make
> performance worse, but, it is in the very core of the ports.
>  
> I don't think you can get away without at least a bit of
> profiling.

Fair enough.  First of all, this patch reduces the size of the built
libguile.so by about 42 kilobytes, and libguile.a by about 96 kilobytes.

As for performance: the updated patches (attached below) slows things
down by about one quarter of one percent on my machine.  The specific
benchmark I did was to call 'read-string' on an 11 megabyte ASCII text
file, with the port-encoding set to UTF-8.  In this case, all the reads
are done using 'scm_get_byte_or_eof' (called from 'get_utf8_codepoint').

So I think this is an acceptable cost for such a great reduction in code
size.  What do you think?

Thanks for nudging me to do the measurement.  To be honest, the original
patch I posted slowed things down by 4.5%, which I found extremely
surprising.  I fixed it by adding an internal 'static' version of
'scm_fill_input'.  So for those who doubt the cost of function calls
though the shared library PLT (which, within a shared library, is
*every* function call to a non-static function), let this be a lesson.
That's the only thing that changed here, and it made more than a 4%
difference, even though the procedure call in question was done only
once per 4 kilobyte buffer!

Here's the raw data:

before:
   12767060  libguile-2.0.a
    6999271  libguile-2.0.so.22.6.0

after:
   12671162  libguile-2.0.a          (96 kbytes smaller)
    6956989  libguile-2.0.so.22.6.0  (42 kbytes smaller)

test file (the contents of 'psyntax.scm' repeated many times):
-rw-r--r-- 1 mhw mhw 11503780 Apr  2 12:46 /home/mhw/BIG-FILE

before:
scheme@(guile-user)> ,time (define s (call-with-input-file "/home/mhw/BIG-FILE" 
read-string))
;; 2.953214s real time, 2.951423s run time.  0.027767s spent in GC.
scheme@(guile-user)> ,time (define s (call-with-input-file "/home/mhw/BIG-FILE" 
read-string))
;; 2.942170s real time, 2.940581s run time.  0.011873s spent in GC.
scheme@(guile-user)> ,time (define s (call-with-input-file "/home/mhw/BIG-FILE" 
read-string))
;; 2.954309s real time, 2.950164s run time.  0.011952s spent in GC.
avg: 2.9473893333333336

after:
scheme@(guile-user)> ,time (define s (call-with-input-file "/home/mhw/BIG-FILE" 
read-string))
;; 2.964777s real time, 2.957597s run time.  0.012116s spent in GC.
scheme@(guile-user)> ,time (define s (call-with-input-file "/home/mhw/BIG-FILE" 
read-string))
;; 2.971099s real time, 2.968656s run time.  0.012020s spent in GC.
scheme@(guile-user)> ,time (define s (call-with-input-file "/home/mhw/BIG-FILE" 
read-string))
;; 2.942377s real time, 2.940015s run time.  0.012085s spent in GC.
avg: 2.9554226666666663

0.27% slower

I've attached the new patches.  Okay to push now?

   Thanks,
     Mark


>From 187fa0b9e7ff9b2d6204517a9daa9009245c7511 Mon Sep 17 00:00:00 2001
From: Mark H Weaver <address@hidden>
Date: Tue, 2 Apr 2013 13:33:14 -0400
Subject: [PATCH 1/2] Add a static version of 'scm_fill_input' to ports.c.

* libguile/ports.c (scm_i_fill_input): New static function, containing
  the code that was previously in 'scm_fill_input'.
  (scm_fill_input): Simply call 'scm_i_fill_input'.
  (scm_c_read): Use 'scm_i_fill_input'.
---
 libguile/ports.c |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/libguile/ports.c b/libguile/ports.c
index becdbed..13a993e 100644
--- a/libguile/ports.c
+++ b/libguile/ports.c
@@ -1419,8 +1419,8 @@ scm_getc (SCM port)
 /* this should only be called when the read buffer is empty.  it
    tries to refill the read buffer.  it returns the first char from
    the port, which is either EOF or *(pt->read_pos).  */
-int
-scm_fill_input (SCM port)
+static int
+scm_i_fill_input (SCM port)
 {
   scm_t_port *pt = SCM_PTAB_ENTRY (port);
 
@@ -1439,6 +1439,12 @@ scm_fill_input (SCM port)
   return scm_ptobs[SCM_PTOBNUM (port)].fill_input (port);
 }
 
+int
+scm_fill_input (SCM port)
+{
+  return scm_i_fill_input (port);
+}
+
 
 /* scm_lfwrite
  *
@@ -1547,8 +1553,8 @@ scm_c_read (SCM port, void *buffer, size_t size)
   if (size == 0)
     return n_read;
 
-  /* Now we will call scm_fill_input repeatedly until we have read the
-     requested number of bytes.  (Note that a single scm_fill_input
+  /* Now we will call scm_i_fill_input repeatedly until we have read the
+     requested number of bytes.  (Note that a single scm_i_fill_input
      call does not guarantee to fill the whole of the port's read
      buffer.) */
   if (pt->read_buf_size <= 1 && pt->encoding == NULL)
@@ -1556,12 +1562,12 @@ scm_c_read (SCM port, void *buffer, size_t size)
       /* The port that we are reading from is unbuffered - i.e. does
         not have its own persistent buffer - but we have a buffer,
         provided by our caller, that is the right size for the data
-        that is wanted.  For the following scm_fill_input calls,
+        that is wanted.  For the following scm_i_fill_input calls,
         therefore, we use the buffer in hand as the port's read
         buffer.
 
         We need to make sure that the port's normal (1 byte) buffer
-        is reinstated in case one of the scm_fill_input () calls
+        is reinstated in case one of the scm_i_fill_input () calls
         throws an exception; we use the scm_dynwind_* API to achieve
         that. 
 
@@ -1578,9 +1584,9 @@ scm_c_read (SCM port, void *buffer, size_t size)
       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,
+      /* Call scm_i_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))
+      while (pt->read_buf_size && (scm_i_fill_input (port) != EOF))
        {
          pt->read_buf_size -= (pt->read_end - pt->read_pos);
          pt->read_pos = pt->read_buf = pt->read_end;
@@ -1604,7 +1610,7 @@ scm_c_read (SCM port, void *buffer, size_t size)
         that a custom port implementation's entry points (in
         particular, fill_input) can rely on the buffer always being
         the same as they first set up. */
-      while (size && (scm_fill_input (port) != EOF))
+      while (size && (scm_i_fill_input (port) != EOF))
        {
          n_available = min (size, pt->read_end - pt->read_pos);
          memcpy (buffer, pt->read_pos, n_available);
-- 
1.7.10.4

>From 8a2b596579185cd0f4d35da478f447e529d81a80 Mon Sep 17 00:00:00 2001
From: Mark H Weaver <address@hidden>
Date: Tue, 2 Apr 2013 05:33:24 -0400
Subject: [PATCH 2/2] Move slow path out of 'scm_get_byte_or_eof' et al.

Suggested by Andy Wingo.

* libguile/inline.h (scm_get_byte_or_eof, scm_peek_byte_or_eof): Keep
  only the fast path here, with fallback to 'scm_i_get_byte_or_eof' and
  'scm_i_peek_byte_or_eof'.

* libguile/ports.c (scm_i_get_byte_or_eof, scm_i_peek_byte_or_eof):
  New internal functions.

* libguile/ports.h (scm_i_get_byte_or_eof, scm_i_peek_byte_or_eof): Add
  prototypes.
---
 libguile/inline.h |   44 ++++++++++----------------------------------
 libguile/ports.c  |   42 ++++++++++++++++++++++++++++++++++++++++++
 libguile/ports.h  |    2 ++
 3 files changed, 54 insertions(+), 34 deletions(-)

diff --git a/libguile/inline.h b/libguile/inline.h
index 88ba7f7..17d8a0c 100644
--- a/libguile/inline.h
+++ b/libguile/inline.h
@@ -96,50 +96,26 @@ scm_is_string (SCM x)
 SCM_INLINE_IMPLEMENTATION int
 scm_get_byte_or_eof (SCM port)
 {
-  int c;
   scm_t_port *pt = SCM_PTAB_ENTRY (port);
 
-  if (pt->rw_active == SCM_PORT_WRITE)
-    /* may be marginally faster than calling scm_flush.  */
-    scm_ptobs[SCM_PTOBNUM (port)].flush (port);
-
-  if (pt->rw_random)
-    pt->rw_active = SCM_PORT_READ;
-
-  if (pt->read_pos >= pt->read_end)
-    {
-      if (SCM_UNLIKELY (scm_fill_input (port) == EOF))
-       return EOF;
-    }
-
-  c = *(pt->read_pos++);
-
-  return c;
+  if (SCM_LIKELY ((pt->rw_active == SCM_PORT_READ || !pt->rw_random)
+                  && pt->read_pos < pt->read_end))
+    return *pt->read_pos++;
+  else
+    return scm_i_get_byte_or_eof (port);
 }
 
 /* Like `scm_get_byte_or_eof' but does not change PORT's `read_pos'.  */
 SCM_INLINE_IMPLEMENTATION int
 scm_peek_byte_or_eof (SCM port)
 {
-  int c;
   scm_t_port *pt = SCM_PTAB_ENTRY (port);
 
-  if (pt->rw_active == SCM_PORT_WRITE)
-    /* may be marginally faster than calling scm_flush.  */
-    scm_ptobs[SCM_PTOBNUM (port)].flush (port);
-
-  if (pt->rw_random)
-    pt->rw_active = SCM_PORT_READ;
-
-  if (pt->read_pos >= pt->read_end)
-    {
-      if (SCM_UNLIKELY (scm_fill_input (port) == EOF))
-       return EOF;
-    }
-
-  c = *pt->read_pos;
-
-  return c;
+  if (SCM_LIKELY ((pt->rw_active == SCM_PORT_READ || !pt->rw_random)
+                  && pt->read_pos < pt->read_end))
+    return *pt->read_pos;
+  else
+    return scm_i_peek_byte_or_eof (port);
 }
 
 SCM_INLINE_IMPLEMENTATION void
diff --git a/libguile/ports.c b/libguile/ports.c
index 13a993e..ee14ca5 100644
--- a/libguile/ports.c
+++ b/libguile/ports.c
@@ -1445,6 +1445,48 @@ scm_fill_input (SCM port)
   return scm_i_fill_input (port);
 }
 
+/* Slow-path fallback for 'scm_get_byte_or_eof' in inline.h */
+int
+scm_i_get_byte_or_eof (SCM port)
+{
+  scm_t_port *pt = SCM_PTAB_ENTRY (port);
+
+  if (pt->rw_active == SCM_PORT_WRITE)
+    scm_flush (port);
+
+  if (pt->rw_random)
+    pt->rw_active = SCM_PORT_READ;
+
+  if (pt->read_pos >= pt->read_end)
+    {
+      if (SCM_UNLIKELY (scm_i_fill_input (port) == EOF))
+       return EOF;
+    }
+
+  return *pt->read_pos++;
+}
+
+/* Slow-path fallback for 'scm_peek_byte_or_eof' in inline.h */
+int
+scm_i_peek_byte_or_eof (SCM port)
+{
+  scm_t_port *pt = SCM_PTAB_ENTRY (port);
+
+  if (pt->rw_active == SCM_PORT_WRITE)
+    scm_flush (port);
+
+  if (pt->rw_random)
+    pt->rw_active = SCM_PORT_READ;
+
+  if (pt->read_pos >= pt->read_end)
+    {
+      if (SCM_UNLIKELY (scm_i_fill_input (port) == EOF))
+       return EOF;
+    }
+
+  return *pt->read_pos;
+}
+
 
 /* scm_lfwrite
  *
diff --git a/libguile/ports.h b/libguile/ports.h
index 53d5081..54bf595 100644
--- a/libguile/ports.h
+++ b/libguile/ports.h
@@ -328,6 +328,8 @@ scm_i_default_port_conversion_handler (void);
 /* Use HANDLER as the default conversion strategy for future ports.  */
 SCM_INTERNAL void
 scm_i_set_default_port_conversion_handler 
(scm_t_string_failed_conversion_handler);
+SCM_INTERNAL int scm_i_get_byte_or_eof (SCM port);
+SCM_INTERNAL int scm_i_peek_byte_or_eof (SCM port);
 
 SCM_API SCM scm_port_conversion_strategy (SCM port);
 SCM_API SCM scm_set_port_conversion_strategy_x (SCM port, SCM behavior);
-- 
1.7.10.4