Re: [Emacs-diffs] master 81a1088: Tweak builtin symbol order for speed

Re: [Emacs-diffs] master 81a1088: Tweak builtin symbol order for speed

[Pip Cet]

On Fri, Jul 12, 2019 at 6:07 AM Paul Eggert <eggert@cs.ucla.edu> wrote:
> branch: master
> commit 81a1088ee8b833cd30a3363782195d6c4d575672
> Author: Paul Eggert <eggert@cs.ucla.edu>
> Commit: Paul Eggert <eggert@cs.ucla.edu>
>
>     Tweak builtin symbol order for speed
>
>     * lib-src/make-docfile.c (compare_globals):
>     Make symbols 1 through 4 be t, unbound, error, lambda.
>     This is in addition to symbol 0 being nil.
>     This change improved ‘make compile-always’ performance by 0.6%
>     on my platform.

Hmm. I don't understand why this would improve performance this
significantly (maybe because it allows 8-bit offsets to be used rather
than 32-bit ones?). It will make debugging a tad easier, though.
XIL(0x30) for Qt is easy to remember.

Maybe it would be a good idea to make Vdead a symbol (or a tagged NULL
pointer) and treat it similarly, if this actually improves
performance.

> --->  lib-src/make-docfile.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/lib-src/make-docfile.c b/lib-src/make-docfile.c
> index 4d25b0a..368150b 100644
> --- a/lib-src/make-docfile.c
> +++ b/lib-src/make-docfile.c
> @@ -641,13 +641,24 @@ compare_globals (const void *a, const void *b)
>      return ga->type - gb->type;
>
>    /* Consider "nil" to be the least, so that iQnil is zero.  That
> -     way, Qnil's internal representation is zero, which is a bit faster.  */
> +     way , Qnil's internal representation is zero, which is a bit faster.
> +     Similarly, consideer "t" to be the second-least, and so forth.  */

"consider"

>    if (ga->type == SYMBOL)
>      {
> -      bool a_nil = strcmp (ga->name, "Qnil") == 0;
> -      bool b_nil = strcmp (gb->name, "Qnil") == 0;
> -      if (a_nil | b_nil)
> -       return b_nil - a_nil;
> +      /* Common symbols in decreasing popularity order.  */
> +      static char const commonsym[][8]

I'm curious, why isn't this simply "static char *commonsym[]"?

[Emacs-diffs] master 81a1088: Tweak builtin symbol order for speed

[Paul Eggert]

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

On 7/12/19 12:12 AM, Pip Cet wrote:

> I don't understand why this would improve performance this
> significantly (maybe because it allows 8-bit offsets to be used rather
> than 32-bit ones?).

Yes, that's my guess too. For the record, my benchmark platform was my old work 
desktop (AMD Phenom II X4 910e, circa 2010) running Fedora 30 x86-64, with plain 
'make compile-always' (no -j). I really should get that upgraded.

> Maybe it would be a good idea to make Vdead a symbol (or a tagged NULL
> pointer) and treat it similarly, if this actually improves
> performance.

It does speed up GC a bit. I installed the attached. Thanks for the suggestion.

>> +      static char const commonsym[][8]
> 
> I'm curious, why isn't this simply "static char *commonsym[]"?

Old performance habits that don't matter here. Besides, that alternative should 
be 'static char const *const commonsym[]" which is not obviously simpler....

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Replace-Vdead-with-tagged-pointer.patch --]
[-- Type: text/x-patch; name="0001-Replace-Vdead-with-tagged-pointer.patch", Size: 7533 bytes --]

From 04cbdde94d256d9b3fbfcc67981374a55d339fcd Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 12 Jul 2019 22:29:02 -0700
Subject: [PATCH] Replace Vdead with tagged pointer
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This speeds up ‘make compile-always’ by 0.1% on my platform.
Suggested by Pip Cet in:
https://lists.gnu.org/r/emacs-devel/2019-07/msg00257.html
* src/.gdbinit (pwinx, pgx, xbuffer, xprintstr):
Output dead_object () as "DEAD".
* src/alloc.c (Vdead, DEADP): Remove.
All uses replaced by dead_object () / deadp.
(deadp): New function.
(init_alloc_once_for_pdumper): Remove no-longer-needed
initialization.
* src/lisp.h (dead_object): New function.
---
 src/.gdbinit  | 27 ++++++++++++++++++---------
 src/alloc.c   | 30 +++++++++++-------------------
 src/lisp.h    | 12 +++++++++---
 src/pdumper.c |  5 +----
 4 files changed, 39 insertions(+), 35 deletions(-)

diff --git a/src/.gdbinit b/src/.gdbinit
index c0cf639359..e9ba5267ec 100644
--- a/src/.gdbinit
+++ b/src/.gdbinit
@@ -382,7 +382,7 @@ define pwinx
   xgetptr $w->contents
   set $tem = (struct buffer *) $ptr
   xgetptr $tem->name_
-  printf "%s", ((struct Lisp_String *) $ptr)->u.s.data
+  printf "%s", $ptr ? (char *) ((struct Lisp_String *) $ptr)->u.s.data : "DEAD"
   printf "\n"
   xgetptr $w->start
   set $tem = (struct Lisp_Marker *) $ptr
@@ -508,7 +508,12 @@ define pgx
   xgettype ($g.object)
   if ($type == Lisp_String)
     xgetptr $g.object
-    printf " str=0x%x[%d]", ((struct Lisp_String *)$ptr)->u.s.data, $g.charpos
+    if ($ptr)
+      printf " str=0x%x", ((struct Lisp_String *)$ptr)->u.s.data
+    else
+      printf " str=DEAD"
+    end
+    printf "[%d]", $g.charpos
   else
     printf " pos=%d", $g.charpos
   end
@@ -879,7 +884,7 @@ define xbuffer
   xgetptr $
   print (struct buffer *) $ptr
   xgetptr $->name_
-  output ((struct Lisp_String *) $ptr)->u.s.data
+  output $ptr ? (char *) ((struct Lisp_String *) $ptr)->u.s.data : "DEAD"
   echo \n
 end
 document xbuffer
@@ -1046,13 +1051,17 @@ Print $ as a lisp object of any type.
 end
 
 define xprintstr
-  set $data = (char *) $arg0->u.s.data
-  set $strsize = ($arg0->u.s.size_byte < 0) ? ($arg0->u.s.size & ~ARRAY_MARK_FLAG) : $arg0->u.s.size_byte
-  # GDB doesn't like zero repetition counts
-  if $strsize == 0
-    output ""
+  if (! $arg0)
+    output "DEAD"
   else
-    output ($arg0->u.s.size > 1000) ? 0 : ($data[0])@($strsize)
+    set $data = (char *) $arg0->u.s.data
+    set $strsize = ($arg0->u.s.size_byte < 0) ? ($arg0->u.s.size & ~ARRAY_MARK_FLAG) : $arg0->u.s.size_byte
+    # GDB doesn't like zero repetition counts
+    if $strsize == 0
+      output ""
+    else
+      output ($arg0->u.s.size > 1000) ? 0 : ($data[0])@($strsize)
+    end
   end
 end
 
diff --git a/src/alloc.c b/src/alloc.c
index 833176d4e9..7a0611dd3e 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -420,14 +420,11 @@ enum mem_type
   MEM_TYPE_SPARE
 };
 
-/* A unique object in pure space used to make some Lisp objects
-   on free lists recognizable in O(1).  */
-
-#ifndef ENABLE_CHECKING
-static
-#endif
-Lisp_Object Vdead;
-#define DEADP(x) EQ (x, Vdead)
+static bool
+deadp (Lisp_Object x)
+{
+  return EQ (x, dead_object ());
+}
 
 #ifdef GC_MALLOC_CHECK
 
@@ -499,10 +496,6 @@ static void mem_delete (struct mem_node *);
 static void mem_delete_fixup (struct mem_node *);
 static struct mem_node *mem_find (void *);
 
-#ifndef DEADP
-# define DEADP(x) 0
-#endif
-
 /* Addresses of staticpro'd variables.  Initialize it to a nonzero
    value if we might unexec; otherwise some compilers put it into
    BSS.  */
@@ -2548,7 +2541,7 @@ void
 free_cons (struct Lisp_Cons *ptr)
 {
   ptr->u.s.u.chain = cons_free_list;
-  ptr->u.s.car = Vdead;
+  ptr->u.s.car = dead_object ();
   cons_free_list = ptr;
   consing_since_gc -= sizeof *ptr;
   gcstat.total_free_conses++;
@@ -4374,7 +4367,7 @@ live_cons_holding (struct mem_node *m, void *p)
 	{
 	  cp = ptr_bounds_copy (cp, b);
 	  struct Lisp_Cons *s = p = cp -= offset % sizeof b->conses[0];
-	  if (!EQ (s->u.s.car, Vdead))
+	  if (!deadp (s->u.s.car))
 	    return make_lisp_ptr (s, Lisp_Cons);
 	}
     }
@@ -4410,7 +4403,7 @@ live_symbol_holding (struct mem_node *m, void *p)
 	{
 	  cp = ptr_bounds_copy (cp, b);
 	  struct Lisp_Symbol *s = p = cp -= offset % sizeof b->symbols[0];
-	  if (!EQ (s->u.s.function, Vdead))
+	  if (!deadp (s->u.s.function))
 	    return make_lisp_symbol (s);
 	}
     }
@@ -6717,7 +6710,7 @@ sweep_conses (void)
                       this_free++;
                       cblk->conses[pos].u.s.u.chain = cons_free_list;
                       cons_free_list = &cblk->conses[pos];
-                      cons_free_list->u.s.car = Vdead;
+                      cons_free_list->u.s.car = dead_object ();
                     }
                   else
                     {
@@ -6883,7 +6876,7 @@ sweep_symbols (void)
                 }
               sym->u.s.next = symbol_free_list;
               symbol_free_list = sym;
-              symbol_free_list->u.s.function = Vdead;
+              symbol_free_list->u.s.function = dead_object ();
               ++this_free;
             }
           else
@@ -7072,7 +7065,7 @@ which_symbols (Lisp_Object obj, EMACS_INT find_max)
    ptrdiff_t gc_count = inhibit_garbage_collection ();
    Lisp_Object found = Qnil;
 
-   if (! DEADP (obj))
+   if (! deadp (obj))
      {
        for (int i = 0; i < ARRAYELTS (lispsym); i++)
 	 {
@@ -7251,7 +7244,6 @@ init_alloc_once_for_pdumper (void)
   purebeg = PUREBEG;
   pure_size = PURESIZE;
   mem_init ();
-  Vdead = make_pure_string ("DEAD", 4, 4, 0);
 
 #ifdef DOUG_LEA_MALLOC
   mallopt (M_TRIM_THRESHOLD, 128 * 1024); /* Trim threshold.  */
diff --git a/src/lisp.h b/src/lisp.h
index 7641b2aab4..e93a219625 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -1247,6 +1247,15 @@ make_lisp_ptr (void *ptr, enum Lisp_Type type)
 #define XSETSYMBOL(a, b) ((a) = make_lisp_symbol (b))
 #define XSETFLOAT(a, b) ((a) = make_lisp_ptr (b, Lisp_Float))
 
+/* Return a Lisp_Object value that does not correspond to any object.
+   This can make some Lisp objects on free lists recognizable in O(1).  */
+
+INLINE Lisp_Object
+dead_object (void)
+{
+  return make_lisp_ptr (NULL, Lisp_String);
+}
+
 /* Pseudovector types.  */
 
 #define XSETPVECTYPE(v, code)						\
@@ -3759,9 +3768,6 @@ extern byte_ct const memory_full_cons_threshold;
 #ifdef HAVE_PDUMPER
 extern int number_finalizers_run;
 #endif
-#ifdef ENABLE_CHECKING
-extern Lisp_Object Vdead;
-#endif
 extern Lisp_Object list1 (Lisp_Object);
 extern Lisp_Object list2 (Lisp_Object, Lisp_Object);
 extern Lisp_Object list3 (Lisp_Object, Lisp_Object, Lisp_Object);
diff --git a/src/pdumper.c b/src/pdumper.c
index 3d8531c6a4..b80757c207 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -3061,10 +3061,7 @@ dump_object (struct dump_context *ctx, Lisp_Object object)
 #if CHECK_STRUCTS && !defined (HASH_Lisp_Type_E2AD97D3F7)
 # error "Lisp_Type changed. See CHECK_STRUCTS comment in config.h."
 #endif
-#ifdef ENABLE_CHECKING
-  /* Vdead is extern only when ENABLE_CHECKING.  */
-  eassert (!EQ (object, Vdead));
-#endif
+  eassert (!EQ (object, dead_object ()));
 
   dump_off offset = dump_recall_object (ctx, object);
   if (offset > 0)
-- 
2.17.1