Re: argv_ref patch 25: allow NUL in quote and comment delimiters

[Eric Blake]

Eric Blake <ebb9 <at> byu.net> writes:

> > Phooey - I was timing the wrong build.  In looking at this closer, I'm 
seeing 
> > some noticeable degredation in speed, on the order of 6%; I'm trying to see 
> if 
> > there is anything obvious that could be causing this slowdown.  My first 
> guess 
> > is that the MATCH macro in input.c might be the culprit, since it is on a 
hot 
> > path (finding the close-quote to quoted strings).
> 
> Good guess, but wrong.  It turns out that I was able to localize the bulk of 
> the slowdown to just these four hunks, all of which deal with the replacement 
> of hand-rolled obstack output with obstack_printf during stage25.  
> obstack_printf is still useful for complex things not on the hot path, but 
> looks like it has too much overhead to be used in len, eval, index, and other 
> builtins that are frequently called.  I'll be checking in a reversion patch 
> based on these four hunks.

After more analysis, here's what I'm committing.  Two of the four hunks 
identified above were on cold paths, so it isn't worth reverting them.  It 
turns out that there is some (slight) benefit to improving MATCH.  Also, more 
of the speedup resulted from fixing shipout_int (shared among len, $#, 
incr, ...) than eval, but both places showed an impact.  With this patch, speed 
is once again roughly the same as before stage25.


From: Eric Blake <address@hidden>
Date: Wed, 18 Jun 2008 17:09:48 -0600
Subject: [PATCH] Revert speed regression from previous patch.

* src/builtin.c (shipout_int, m4_eval): Avoid obstack_printf in
hot paths.
* src/input.c (MATCH): Use fewer conditionals, and factor
adjustment of S...
(match_input): ...here for smaller code size.
(input_init, set_quotes, set_comment): Supply trailing NUL to
delimiters, to meet contract of faster MATCH.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog     |   11 +++++++++-
 src/builtin.c |   15 ++++++++++---
 src/input.c   |   61 ++++++++++++++++++++++++++++++++------------------------
 3 files changed, 56 insertions(+), 31 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 60a5a9e..794d6d7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,12 +1,21 @@
 2008-06-18  Eric Blake  <address@hidden>
 
+       Revert speed regression from previous patch.
+       * src/builtin.c (shipout_int, m4_eval): Avoid obstack_printf in
+       hot paths.
+       * src/input.c (MATCH): Use fewer conditionals, and factor
+       adjustment of S...
+       (match_input): ...here for smaller code size.
+       (input_init, set_quotes, set_comment): Supply trailing NUL to
+       delimiters, to meet contract of faster MATCH.
+
        Stage 25: Handle embedded NUL in changequote and changecom.
        Track quote and comment delimiters by length, to allow embedded
        NUL.  Convert macro tracing and other locations to use
        obstack_printf rather than hand-rolled equivalents.  Ensure that
        embedded NUL in trace output does not truncate the trace string.
        Memory impact: none.
-       Speed impact: none noticed.
+       Speed impact: noticeable penalty, from obstack_printf overhead.
        * m4/gnulib-cache.m4: Import obstack-printf-posix module.
        * src/m4.h (ntoa): Remove declaration.
        (DEBUG_PRINT1, DEBUG_PRINT3, MESSAGE, DEBUG_MESSAGE1)
diff --git a/src/builtin.c b/src/builtin.c
index 6b107ae..d64b567 100644
--- a/src/builtin.c
+++ b/src/builtin.c
@@ -629,7 +629,10 @@ ntoa (int32_t value, int radix)
 static void
 shipout_int (struct obstack *obs, int val)
 {
-  obstack_printf (obs, "%d", val);
+  const char *s;
+
+  s = ntoa ((int32_t) val, 10);
+  obstack_grow (obs, s, strlen (s));
 }
 
 
@@ -1227,9 +1230,13 @@ m4_eval (struct obstack *obs, int argc, macro_arguments 
*argv)
       s++;
     }
   len = strlen (s);
-  if (min < len)
-    min = len;
-  obstack_printf (obs, "%.*d%s", min - len, 0, s);
+  if (len < min)
+    {
+      min -= len;
+      obstack_blank (obs, min);
+      memset (obstack_next_free (obs) - min, '0', min);
+    }
+  obstack_grow (obs, s, len);
 }
 
 static void
diff --git a/src/input.c b/src/input.c
index 589fbb1..18271cd 100644
--- a/src/input.c
+++ b/src/input.c
@@ -1197,11 +1197,12 @@ init_argv_token (struct obstack *obs, token_data *td)
 
 
 /*------------------------------------------------------------------.
-| This function is for matching a string against a prefix of the    |
-| input stream.  If the string S of length SLEN matches the input   |
-| and CONSUME is true, the input is discarded; otherwise any        |
-| characters read are pushed back again.  The function is used only |
-| when multicharacter quotes or comment delimiters are used.        |
+| If the string S of length SLEN matches the next characters of the |
+| input stream, return true.  If CONSUME, the first character has   |
+| already been matched.  If a match is found and CONSUME is true,   |
+| the input is discarded; otherwise any characters read are pushed  |
+| back again.  The function is used only when multicharacter quotes |
+| or comment delimiters are used.                                   |
 `------------------------------------------------------------------*/
 
 static bool
@@ -1212,6 +1213,11 @@ match_input (const char *s, size_t slen, bool consume)
   const char *t;
   bool result = false;
 
+  if (consume)
+    {
+      s++;
+      slen--;
+    }
   assert (slen);
   ch = peek_input (false);
   if (ch != to_uchar (*s))
@@ -1245,21 +1251,22 @@ match_input (const char *s, size_t slen, bool consume)
   return result;
 }
 
-/*---------------------------------------------------------------.
-| The macro MATCH() is used to match a string S of length SLEN   |
-| against the input.  The first character is handled inline, for |
-| speed.  Hopefully, this will not hurt efficiency too much when |
-| single character quotes and comment delimiters are used.  If   |
-| CONSUME, then CH is the result of next_char, and a successful  |
-| match will discard the matched string.  Otherwise, CH is the   |
-| result of peek_input, and the input stream is effectively      |
-| unchanged.                                                     |
-`---------------------------------------------------------------*/
+/*--------------------------------------------------------------------.
+| The macro MATCH() is used to match a string S of length SLEN        |
+| against the input.  The first character is handled inline for       |
+| speed, and S[SLEN] must be safe to dereference (it is faster to do  |
+| character comparison prior to length checks).  This improves        |
+| efficiency for the common case of single character quotes and       |
+| comment delimiters, while being safe for disabled delimiters as     |
+| well as longer delimiters.  If CONSUME, then CH is the result of    |
+| next_char, and a successful match will discard the matched string.  |
+| Otherwise, CH is the result of peek_input, and the input stream is  |
+| effectively unchanged.                                              |
+`--------------------------------------------------------------------*/
 
 #define MATCH(ch, s, slen, consume)                                    \
-  ((slen) && to_uchar ((s)[0]) == (ch)                                 \
-   && ((slen) == 1                                                     \
-       || (match_input ((s) + (consume), (slen) - (consume), consume))))
+  (to_uchar ((s)[0]) == (ch)                                           \
+   && ((slen) >> 1 ? match_input (s, slen, consume) : (slen)))
 
 
 /*----------------------------------------------------------.
@@ -1291,13 +1298,13 @@ input_init (void)
 
   start_of_input_line = false;
 
-  curr_quote.str1 = xmemdup (DEF_LQUOTE, 1);
+  curr_quote.str1 = xmemdup0 (DEF_LQUOTE, 1);
   curr_quote.len1 = 1;
-  curr_quote.str2 = xmemdup (DEF_RQUOTE, 1);
+  curr_quote.str2 = xmemdup0 (DEF_RQUOTE, 1);
   curr_quote.len2 = 1;
-  curr_comm.str1 = xmemdup (DEF_BCOMM, 1);
+  curr_comm.str1 = xmemdup0 (DEF_BCOMM, 1);
   curr_comm.len1 = 1;
-  curr_comm.str2 = xmemdup (DEF_ECOMM, 1);
+  curr_comm.str2 = xmemdup0 (DEF_ECOMM, 1);
   curr_comm.len2 = 1;
 
 #ifdef ENABLE_CHANGEWORD
@@ -1345,9 +1352,10 @@ set_quotes (const char *lq, size_t lq_len, const char 
*rq, size_t rq_len)
 
   free (curr_quote.str1);
   free (curr_quote.str2);
-  curr_quote.str1 = xmemdup (lq, lq_len);
+  /* The use of xmemdup0 is essential for MATCH() to work.  */
+  curr_quote.str1 = xmemdup0 (lq, lq_len);
   curr_quote.len1 = lq_len;
-  curr_quote.str2 = xmemdup (rq, rq_len);
+  curr_quote.str2 = xmemdup0 (rq, rq_len);
   curr_quote.len2 = rq_len;
   set_quote_age ();
 }
@@ -1387,9 +1395,10 @@ set_comment (const char *bc, size_t bc_len, const char 
*ec, size_t ec_len)
 
   free (curr_comm.str1);
   free (curr_comm.str2);
-  curr_comm.str1 = xmemdup (bc, bc_len);
+  /* The use of xmemdup0 is essential for MATCH() to work.  */
+  curr_comm.str1 = xmemdup0 (bc, bc_len);
   curr_comm.len1 = bc_len;
-  curr_comm.str2 = xmemdup (ec, ec_len);
+  curr_comm.str2 = xmemdup0 (ec, ec_len);
   curr_comm.len2 = ec_len;
   set_quote_age ();
 }
-- 
1.5.5.1