Re: [Emacs-diffs] master e828765: DEFVAR_INT variables are now intmax_t

Re: [Emacs-diffs] master e828765: DEFVAR_INT variables are now intmax_t

[Eli Zaretskii]

> branch: master
> commit e828765d01313acddcf17279b6b43ae9f777f2a4
> Author: Paul Eggert <eggert@cs.ucla.edu>
> Commit: Paul Eggert <eggert@cs.ucla.edu>
> 
>     DEFVAR_INT variables are now intmax_t
>     
>     Formerly they were fixnums, which led to problems when dealing
>     with values that might not fit on 32-bit platforms, such as
>     string-chars-consed or floats_consed.  64-bit counters should
>     be good enough for these (for a while, anyway...).

Doesn't this slow down 32-bit builds?  We have quite a few of
DEFVAR_INTs in Emacs.

Re: [Emacs-diffs] master e828765: DEFVAR_INT variables are now intmax_t

[Paul Eggert]

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

On 2/27/19 8:13 AM, Eli Zaretskii wrote:
> Doesn't this slow down 32-bit builds?  We have quite a few of
> DEFVAR_INTs in Emacs.

Emacs has 55 DEFVAR_INTs. This patch affects their width only on 32-bit
builds sans --with-wide-int. On my platform (Fedora 29 AMD Phenom II X4
910e compiled with gcc -m32 -march=native), narrowing DEFVAR_INTs back
from 64 to 32 bits (see attached patch) shrinks Emacs's bss space (as
computed by 'size') from 379144 to 378984 bytes, a savings of 0.04%. I
tried to measure how much it affects CPU performance but couldn't get
consistent results; it seems to be below the level of significance,
though if I tried harder I might be able to tease it out.

My impression is that it's worth the small extra cost to get correct
answers rather than junk, for the affected variables. If performance
issues are of significant concern, I could split the DEFVAR_INTs into
two categories: one where even 32-bit fixnums will easily serve (e.g.,
column counts), and one where 64-bit counters are needed (e.g.,
garbage-collection byte counts). This would complicate the internals a
bit, though.


[-- Attachment #2: narrow-DEFVAR_INT.diff --]
[-- Type: text/x-patch, Size: 12096 bytes --]

diff --git a/lib-src/make-docfile.c b/lib-src/make-docfile.c
index 05a08473c3..c68173eb66 100644
--- a/lib-src/make-docfile.c
+++ b/lib-src/make-docfile.c
@@ -700,7 +700,7 @@ write_globals (void)
       switch (globals[i].type)
 	{
 	case EMACS_INTEGER:
-	  type = "intmax_t";
+	  type = "EMACS_INT";
 	  break;
 	case BOOLEAN:
 	  type = "bool";
diff --git a/src/data.c b/src/data.c
index 15b6106cfe..9718e2f5b4 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1079,7 +1079,8 @@ store_symval_forwarding (union Lisp_Fwd *valcontents, register Lisp_Object newva
       {
 	intmax_t i;
 	CHECK_INTEGER (newval);
-	if (! integer_to_intmax (newval, &i))
+	if (! (integer_to_intmax (newval, &i)
+	       && TYPE_MINIMUM (EMACS_INT) <= i && i <= EMACS_INT_MAX))
 	  xsignal1 (Qoverflow_error, newval);
 	*XFIXNUMFWD (valcontents)->intvar = i;
       }
diff --git a/src/eval.c b/src/eval.c
index bf16a709b1..aa4ee3abc9 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -63,7 +63,7 @@ Lisp_Object Vrun_hooks;
    signal the error instead of entering an infinite loop of debugger
    invocations.  */
 
-static intmax_t when_entered_debugger;
+static EMACS_INT when_entered_debugger;
 
 /* The function from which the last `signal' was called.  Set in
    Fsignal.  */
@@ -269,9 +269,9 @@ init_eval (void)
    value otherwise.  */
 
 static void
-max_ensure_room (intmax_t *m, intmax_t a, intmax_t b)
+max_ensure_room (EMACS_INT *m, intmax_t a, int b)
 {
-  intmax_t sum = INT_ADD_WRAPV (a, b, &sum) ? INTMAX_MAX : sum;
+  EMACS_INT sum = INT_ADD_WRAPV (a, b, &sum) ? EMACS_INT_MAX : sum;
   *m = max (*m, sum);
 }
 
@@ -280,8 +280,9 @@ max_ensure_room (intmax_t *m, intmax_t a, intmax_t b)
 static void
 restore_stack_limits (Lisp_Object data)
 {
-  integer_to_intmax (XCAR (data), &max_specpdl_size);
-  integer_to_intmax (XCDR (data), &max_lisp_eval_depth);
+  intmax_t i;
+  integer_to_intmax (XCAR (data), &i); max_specpdl_size = i;
+  integer_to_intmax (XCDR (data), &i); max_lisp_eval_depth = i;
 }
 
 static void grow_specpdl (void);
@@ -294,9 +295,9 @@ call_debugger (Lisp_Object arg)
   bool debug_while_redisplaying;
   ptrdiff_t count = SPECPDL_INDEX ();
   Lisp_Object val;
-  intmax_t old_depth = max_lisp_eval_depth;
+  EMACS_INT old_depth = max_lisp_eval_depth;
   /* Do not allow max_specpdl_size less than actual depth (Bug#16603).  */
-  intmax_t old_max = max (max_specpdl_size, count);
+  EMACS_INT old_max = max (max_specpdl_size, count);
 
   /* The previous value of 40 is too small now that the debugger
      prints using cl-prin1 instead of prin1.  Printing lists nested 8
diff --git a/src/fileio.c b/src/fileio.c
index cac8ed0aee..0eb7058ddd 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -5706,7 +5706,7 @@ A non-nil CURRENT-ONLY argument means save only current buffer.  */)
   bool old_message_p = 0;
   struct auto_save_unwind auto_save_unwind;
 
-  intmax_t sum = INT_ADD_WRAPV (specpdl_size, 40, &sum) ? INTMAX_MAX : sum;
+  EMACS_INT sum = INT_ADD_WRAPV (specpdl_size, 40, &sum) ? EMACS_INT_MAX : sum;
   if (max_specpdl_size < sum)
     max_specpdl_size = sum;
 
diff --git a/src/frame.h b/src/frame.h
index 544e0bef17..f53a3bb65a 100644
--- a/src/frame.h
+++ b/src/frame.h
@@ -580,7 +580,7 @@ struct frame
   int config_scroll_bar_lines;
 
   /* The baud rate that was used to calculate costs for this frame.  */
-  intmax_t cost_calculation_baud_rate;
+  EMACS_INT cost_calculation_baud_rate;
 
   /* Frame opacity
      alpha[0]: alpha transparency of the active frame
diff --git a/src/keyboard.c b/src/keyboard.c
index 3af487cf07..38a84d6c4c 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -208,7 +208,7 @@ struct buffer *buffer_before_last_command_or_undo;
 
 /* Value of num_nonmacro_input_events as of last auto save.  */
 
-static intmax_t last_auto_save;
+static EMACS_INT last_auto_save;
 
 /* The value of point when the last command was started. */
 static ptrdiff_t last_point_position;
@@ -1966,7 +1966,7 @@ void
 bind_polling_period (int n)
 {
 #ifdef POLL_FOR_INPUT
-  intmax_t new = polling_period;
+  EMACS_INT new = polling_period;
 
   if (n > new)
     new = n;
@@ -5585,7 +5585,7 @@ make_lispy_event (struct input_event *event)
 	     double-click-fuzz as is.  On other frames, interpret it
 	     as a multiple of 1/8 characters.  */
 	  struct frame *f;
-	  intmax_t fuzz;
+	  EMACS_INT fuzz;
 
 	  if (WINDOWP (event->frame_or_window))
 	    f = XFRAME (XWINDOW (event->frame_or_window)->frame);
@@ -5651,7 +5651,7 @@ make_lispy_event (struct input_event *event)
 	    else
 	      {
 		Lisp_Object new_down, down;
-		intmax_t xdiff = double_click_fuzz, ydiff = double_click_fuzz;
+		EMACS_INT xdiff = double_click_fuzz, ydiff = double_click_fuzz;
 
 		/* The third element of every position
 		   should be the (x,y) pair.  */
@@ -5744,7 +5744,7 @@ make_lispy_event (struct input_event *event)
 	     double-click-fuzz as is.  On other frames, interpret it
 	     as a multiple of 1/8 characters.  */
 	  struct frame *fr;
-	  intmax_t fuzz;
+	  EMACS_INT fuzz;
 	  int symbol_num;
 	  bool is_double;
 
diff --git a/src/lisp.h b/src/lisp.h
index 388cd04163..0e25ecc6ad 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -2664,7 +2664,7 @@ make_uint (uintmax_t n)
 struct Lisp_Intfwd
   {
     enum Lisp_Fwd_Type type;	/* = Lisp_Fwd_Int */
-    intmax_t *intvar;
+    EMACS_INT *intvar;
   };
 
 /* Boolean forwarding pointer to an int variable.
@@ -3099,7 +3099,7 @@ enum maxargs
 extern void defvar_lisp (struct Lisp_Objfwd *, const char *, Lisp_Object *);
 extern void defvar_lisp_nopro (struct Lisp_Objfwd *, const char *, Lisp_Object *);
 extern void defvar_bool (struct Lisp_Boolfwd *, const char *, bool *);
-extern void defvar_int (struct Lisp_Intfwd *, const char *, intmax_t *);
+extern void defvar_int (struct Lisp_Intfwd *, const char *, EMACS_INT *);
 extern void defvar_kboard (struct Lisp_Kboard_Objfwd *, const char *, int);
 
 /* Macros we use to define forwarded Lisp variables.
diff --git a/src/lread.c b/src/lread.c
index 8b0d693daf..6389e3ed48 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -4422,11 +4422,11 @@ defalias (struct Lisp_Subr *sname, char *string)
 #endif /* NOTDEF */
 
 /* Define an "integer variable"; a symbol whose value is forwarded to a
-   C variable of type intmax_t.  Sample call (with "xx" to fool make-docfile):
+   C variable of type EMACS_INT.  Sample call (with "xx" to fool make-docfile):
    DEFxxVAR_INT ("emacs-priority", &emacs_priority, "Documentation");  */
 void
 defvar_int (struct Lisp_Intfwd *i_fwd,
-	    const char *namestring, intmax_t *address)
+	    const char *namestring, EMACS_INT *address)
 {
   Lisp_Object sym;
   sym = intern_c_string (namestring);
diff --git a/src/macros.c b/src/macros.c
index 2d927ffc40..9f6b4ade94 100644
--- a/src/macros.c
+++ b/src/macros.c
@@ -267,7 +267,9 @@ pop_kbd_macro (Lisp_Object info)
   Lisp_Object tem;
   Vexecuting_kbd_macro = XCAR (info);
   tem = XCDR (info);
-  integer_to_intmax (XCAR (tem), &executing_kbd_macro_index);
+  intmax_t i;
+  integer_to_intmax (XCAR (tem), &i);
+  executing_kbd_macro_index = i;
   Vreal_this_command = XCDR (tem);
   run_hook (Qkbd_macro_termination_hook);
 }
diff --git a/src/pdumper.c b/src/pdumper.c
index 2f5c719803..af526d2a80 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -1615,7 +1615,7 @@ dump_emacs_reloc_immediate (struct dump_context *ctx,
 
 DEFINE_EMACS_IMMEDIATE_FN (dump_emacs_reloc_immediate_lv, Lisp_Object);
 DEFINE_EMACS_IMMEDIATE_FN (dump_emacs_reloc_immediate_ptrdiff_t, ptrdiff_t);
-DEFINE_EMACS_IMMEDIATE_FN (dump_emacs_reloc_immediate_intmax_t, intmax_t);
+DEFINE_EMACS_IMMEDIATE_FN (dump_emacs_reloc_immediate_emacs_int, EMACS_INT);
 DEFINE_EMACS_IMMEDIATE_FN (dump_emacs_reloc_immediate_int, int);
 DEFINE_EMACS_IMMEDIATE_FN (dump_emacs_reloc_immediate_bool, bool);
 
@@ -2286,10 +2286,10 @@ dump_float (struct dump_context *ctx, const struct Lisp_Float *lfloat)
 static dump_off
 dump_fwd_int (struct dump_context *ctx, const struct Lisp_Intfwd *intfwd)
 {
-#if CHECK_STRUCTS && !defined HASH_Lisp_Intfwd_4D887A7387
+#if CHECK_STRUCTS && !defined HASH_Lisp_Intfwd_1225FA32CC
 # error "Lisp_Intfwd changed. See CHECK_STRUCTS comment."
 #endif
-  dump_emacs_reloc_immediate_intmax_t (ctx, intfwd->intvar, *intfwd->intvar);
+  dump_emacs_reloc_immediate_emacs_int (ctx, intfwd->intvar, *intfwd->intvar);
   struct Lisp_Intfwd out;
   dump_object_start (ctx, &out, sizeof (out));
   DUMP_FIELD_COPY (&out, intfwd, type);
diff --git a/src/thread.h b/src/thread.h
index e46545baf2..5e003761e8 100644
--- a/src/thread.h
+++ b/src/thread.h
@@ -104,7 +104,7 @@ struct thread_state
 #define specpdl_ptr (current_thread->m_specpdl_ptr)
 
   /* Depth in Lisp evaluations and function calls.  */
-  intmax_t m_lisp_eval_depth;
+  EMACS_INT m_lisp_eval_depth;
 #define lisp_eval_depth (current_thread->m_lisp_eval_depth)
 
   /* This points to the current buffer.  */
diff --git a/src/undo.c b/src/undo.c
index 3c1251dae6..27238e6e93 100644
--- a/src/undo.c
+++ b/src/undo.c
@@ -291,7 +291,7 @@ truncate_undo_list (struct buffer *b)
 {
   Lisp_Object list;
   Lisp_Object prev, next, last_boundary;
-  intmax_t size_so_far = 0;
+  EMACS_INT size_so_far = 0;
 
   /* Make sure that calling undo-outer-limit-function
      won't cause another GC.  */
diff --git a/src/xdisp.c b/src/xdisp.c
index 760c31c676..8d16d3891c 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -15767,7 +15767,7 @@ enum
 
 static int
 try_scrolling (Lisp_Object window, bool just_this_one_p,
-	       intmax_t arg_scroll_conservatively, intmax_t scroll_step,
+	       EMACS_INT arg_scroll_conservatively, EMACS_INT scroll_step,
 	       bool temp_scroll_step, bool last_line_misfit)
 {
   struct window *w = XWINDOW (window);
@@ -15803,7 +15803,7 @@ try_scrolling (Lisp_Object window, bool just_this_one_p,
     /* Compute how much we should try to scroll maximally to bring
        point into view.  */
     {
-      intmax_t scroll_lines_max
+      EMACS_INT scroll_lines_max
 	= max (scroll_step, max (arg_scroll_conservatively, temp_scroll_step));
       int scroll_lines = clip_to_bounds (0, scroll_lines_max, 1000000);
       scroll_max = scroll_lines * frame_line_height;
diff --git a/src/xselect.c b/src/xselect.c
index 5f0bb44cc9..24d5595af5 100644
--- a/src/xselect.c
+++ b/src/xselect.c
@@ -1085,10 +1085,10 @@ wait_for_property_change (struct prop_location *location)
      property_change_reply, because property_change_reply_object says so.  */
   if (! location->arrived)
     {
-      intmax_t timeout = max (0, x_selection_timeout);
-      intmax_t secs = timeout / 1000;
+      EMACS_INT timeout = max (0, x_selection_timeout);
+      EMACS_INT secs = timeout / 1000;
       int nsecs = (timeout % 1000) * 1000000;
-      TRACE2 ("  Waiting %"PRIdMAX" secs, %d nsecs", secs, nsecs);
+      TRACE2 ("  Waiting %"pI" secs, %d nsecs", secs, nsecs);
       wait_reading_process_output (secs, nsecs, 0, false,
 				   property_change_reply, NULL, 0);
 
@@ -1193,10 +1193,10 @@ x_get_foreign_selection (Lisp_Object selection_symbol, Lisp_Object target_type,
   unblock_input ();
 
   /* This allows quits.  Also, don't wait forever.  */
-  intmax_t timeout = max (0, x_selection_timeout);
-  intmax_t secs = timeout / 1000;
+  EMACS_INT timeout = max (0, x_selection_timeout);
+  EMACS_INT secs = timeout / 1000;
   int nsecs = (timeout % 1000) * 1000000;
-  TRACE1 ("  Start waiting %"PRIdMAX" secs for SelectionNotify", secs);
+  TRACE1 ("  Start waiting %"pI" secs for SelectionNotify", secs);
   wait_reading_process_output (secs, nsecs, 0, false,
 			       reading_selection_reply, NULL, 0);
   TRACE1 ("  Got event = %d", !NILP (XCAR (reading_selection_reply)));
diff --git a/src/xterm.c b/src/xterm.c
index 453669f6e0..c072d17553 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -4886,7 +4886,7 @@ x_x_to_emacs_modifiers (struct x_display_info *dpyinfo, int state)
 }
 
 static int
-x_emacs_to_x_modifiers (struct x_display_info *dpyinfo, intmax_t state)
+x_emacs_to_x_modifiers (struct x_display_info *dpyinfo, EMACS_INT state)
 {
   EMACS_INT mod_ctrl = ctrl_modifier;
   EMACS_INT mod_meta = meta_modifier;

Re: [Emacs-diffs] master e828765: DEFVAR_INT variables are now intmax_t

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 27 Feb 2019 17:50:10 -0800
> 
> On 2/27/19 8:13 AM, Eli Zaretskii wrote:
> > Doesn't this slow down 32-bit builds?  We have quite a few of
> > DEFVAR_INTs in Emacs.
> 
> Emacs has 55 DEFVAR_INTs. This patch affects their width only on 32-bit
> builds sans --with-wide-int. On my platform (Fedora 29 AMD Phenom II X4
> 910e compiled with gcc -m32 -march=native), narrowing DEFVAR_INTs back
> from 64 to 32 bits (see attached patch) shrinks Emacs's bss space (as
> computed by 'size') from 379144 to 378984 bytes, a savings of 0.04%. I
> tried to measure how much it affects CPU performance but couldn't get
> consistent results; it seems to be below the level of significance,
> though if I tried harder I might be able to tease it out.

Depends on what code you ran to test that, I presume.

Looking at the list of DEFVAR_INTs, I see that about 40% of them are
in the display engine, where speed certainly matters, although perhaps
not in "emacs -Q".  the ones in keyboard.c are also of some
importance.

So on balance, I guess I wonder why this minor aesthetic or maybe
convenience issue is worth slowing down Emacs on those hosts.  We have
lived with this limitation for a long time, so why not leave it alone?
Just because we _can_ make them bignums doesn't necessarily mean we
should.

Re: [Emacs-diffs] master e828765: DEFVAR_INT variables are now intmax_t

[Paul Eggert]

On 2/28/19 10:09 AM, Eli Zaretskii wrote:
> We have
> lived with this limitation for a long time, so why not leave it alone?
> Just because we _can_ make them bignums doesn't necessarily mean we
> should.

I'm mostly worried about garbage-collection statistics that can overflow
fixnums on 32-bit platform. However, it's a valid point that most of
these DEFVAR_INTs can be fixnums. I'll look into changing them back,
while keeping GC stats reliable.

Re: [Emacs-diffs] master e828765: DEFVAR_INT variables are now intmax_t

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 1 Mar 2019 08:59:25 -0800
> 
> On 2/28/19 10:09 AM, Eli Zaretskii wrote:
> > We have
> > lived with this limitation for a long time, so why not leave it alone?
> > Just because we _can_ make them bignums doesn't necessarily mean we
> > should.
> 
> I'm mostly worried about garbage-collection statistics that can overflow
> fixnums on 32-bit platform.

Why is that a problem?  They are just for display to the user, no?

> However, it's a valid point that most of these DEFVAR_INTs can be
> fixnums. I'll look into changing them back, while keeping GC stats
> reliable.

Thanks.

Re: [Emacs-diffs] master e828765: DEFVAR_INT variables are now intmax_t

[Paul Eggert]

On 3/1/19 11:26 AM, Eli Zaretskii wrote:
> Why is that a problem?  They are just for display to the user, no?

No, as some code computes with garbage-collection statistics, by taking
before-and-after stats and subtracting. This doesn't work with fixnums
after they overflow.