From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Mark H Weaver Newsgroups: gmane.lisp.guile.devel Subject: Re: [PATCH] Move slow path out of 'scm_get_byte_or_eof' et al Date: Tue, 02 Apr 2013 13:54:02 -0400 Message-ID: <874nfo24zp.fsf@tines.lan> References: <87a9ph1cde.fsf@tines.lan> <1364906601.26753.YahooMailNeo@web120403.mail.ne1.yahoo.com> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: ger.gmane.org 1364925278 16665 80.91.229.3 (2 Apr 2013 17:54:38 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 2 Apr 2013 17:54:38 +0000 (UTC) Cc: "guile-devel@gnu.org" To: Mike Gran Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Tue Apr 02 19:55:05 2013 Return-path: Envelope-to: guile-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1UN5QF-0000Xh-PD for guile-devel@m.gmane.org; Tue, 02 Apr 2013 19:55:03 +0200 Original-Received: from localhost ([::1]:35494 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UN5Pq-0001K9-RO for guile-devel@m.gmane.org; Tue, 02 Apr 2013 13:54:38 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:60801) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UN5Pd-00019e-Kc for guile-devel@gnu.org; Tue, 02 Apr 2013 13:54:31 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UN5Pa-0001JJ-Fu for guile-devel@gnu.org; Tue, 02 Apr 2013 13:54:25 -0400 Original-Received: from world.peace.net ([96.39.62.75]:58398) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UN5Pa-0001J6-8m for guile-devel@gnu.org; Tue, 02 Apr 2013 13:54:22 -0400 Original-Received: from 209-6-91-212.c3-0.smr-ubr1.sbo-smr.ma.cable.rcn.com ([209.6.91.212] helo=tines.lan) by world.peace.net with esmtpsa (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.72) (envelope-from ) id 1UN5PO-0007wX-5F; Tue, 02 Apr 2013 13:54:10 -0400 In-Reply-To: <1364906601.26753.YahooMailNeo@web120403.mail.ne1.yahoo.com> (Mike Gran's message of "Tue, 2 Apr 2013 05:43:21 -0700 (PDT)") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6.x X-Received-From: 96.39.62.75 X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Original-Sender: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.devel:16111 Archived-At: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Mike Gran writes: > I can't=C2=A0imagine any possible way that this patch could make > performance worse, but, it is in the very core of the ports. > =C2=A0 > 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-F= ILE" 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-F= ILE" 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-F= ILE" 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-F= ILE" 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-F= ILE" 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-F= ILE" 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 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0001-Add-a-static-version-of-scm_fill_input-to-ports.c.patch Content-Description: [PATCH 1/2] Add a static version of 'scm_fill_input' to ports.c >From 187fa0b9e7ff9b2d6204517a9daa9009245c7511 Mon Sep 17 00:00:00 2001 From: Mark H Weaver 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 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0002-Move-slow-path-out-of-scm_get_byte_or_eof-et-al.patch Content-Description: [PATCH 2/2] Move slow path out of 'scm_get_byte_or_eof' et al >From 8a2b596579185cd0f4d35da478f447e529d81a80 Mon Sep 17 00:00:00 2001 From: Mark H Weaver 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 --=-=-=--