bug#37006: 27.0.50; garbage collection not happening after 26de2d42

[Paul Eggert <eggert@cs.ucla.edu>]

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

Eli Zaretskii wrote:

> We must
> have something in maybe_gc to notice the change and recompute the
> threshold.

I don't see why the threshold needs to be recomputed on each maybe_gc call. I 
suppose we could add a new builtin variable type, so that the threshold could be 
recomputed whenever GC-related builtin variables are changed; that should do the 
trick without slowing down maybe_gc. But is it that important to recalculate the 
GC threshold immediately? Variables like gc-cons-threshold aren't intended for 
fine-grained control over exactly when the GC is called; they're merely advice.

> We must also notice the memory-full condition there.

memory_full already does that, no? It sets consing_until_gc. Or are you thinking 
of some other memory-full condition?

>    if (!NILP (Vmemory_full))
>      consing_until_gc = memory_full_cons_threshold;
>    else
>      {
>        intptr_t threshold = min (max (GC_DEFAULT_THRESHOLD,
> 				     gc_cons_threshold >> 3),
> 				OBJECT_CT_MAX);
>        if (FLOATP (Vgc_cons_percentage))
> 	{
> 	  double tot = (XFLOAT_DATA (Vgc_cons_percentage)
> 			* total_bytes_of_live_objects ());
> 	  if (threshold < tot)
> 	    {
> 	      if (tot < OBJECT_CT_MAX)
> 		threshold = tot;
> 	      else
> 		threshold = OBJECT_CT_MAX;
> 	    }
> 	}
>        consing_until_gc = threshold;
>      }
> 
> First, gc_cons_threshold is an EMACS_INT, so putting its value into
> intptr_t is wrong in 32-bit builds --with-wide-int, right?

That's not a problem, since the above code does min (..., OBJECT_CT_MAX) on the 
result before storing it into intptr_t.

> using intptr_t for OBJECT_CT_MAX is wrong in such a build.

I don't see why it's wrong. The idea is to count the total number of object 
bytes in use. This cannot exceed the number of bytes addressable by pointers 
regardless of the width of EMACS_INT, so intptr_t is appropriate for such counts.
> And second, why does the code divide gc_cons_threshold by 8?  If the
> value of gc_cons_threshold is most-positive-fixnum, that is wrong, I
> think.  Did you mean to divide GC_DEFAULT_THRESHOLD instead?

Right you are; that's a typo. Thanks. I fixed that in master in the attached patch.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-GC-threshold-typo.patch --]
[-- Type: text/x-patch; name="0001-Fix-GC-threshold-typo.patch", Size: 1138 bytes --]

From 1e5bda273a67f960fb834007f4653a630cd67ce9 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 13 Aug 2019 10:03:41 -0700
Subject: [PATCH] Fix GC threshold typo
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by Eli Zaretskii (Bug#37006#25).
* src/alloc.c (garbage_collect_1): Fix typo in threshold calc.
Go back to dividing by 10 since the numerator’s a constant now.
Problem introduced in 2019-07-21T02:40:03Z!eggert@cs.ucla.edu.
---
 src/alloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 39833f8dec..c7419e2fa5 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -5932,8 +5932,8 @@ garbage_collect_1 (struct gcstat *gcst)
     consing_until_gc = memory_full_cons_threshold;
   else
     {
-      intptr_t threshold = min (max (GC_DEFAULT_THRESHOLD,
-				     gc_cons_threshold >> 3),
+      intptr_t threshold = min (max (GC_DEFAULT_THRESHOLD / 10,
+				     gc_cons_threshold),
 				OBJECT_CT_MAX);
       if (FLOATP (Vgc_cons_percentage))
 	{
-- 
2.17.1