bug#65700: time when gcs-done is updated needs to be clarified

bug#65700: time when gcs-done is updated needs to be clarified

[Shynur Xie]

I wrote a function which runs each time Emacs collects garbage, so I
added it to `post-gc-hook'.

That function cares about the value of `gcs-done' and the value of
`gc-elapsed'.
But it seems that `gcs-done' is updated after `post-gc-hook' is
executed?

Is this an intended behavior?  If so, I think the docstring of these
variables (such as `gcs-done', `gc-elapsed', ...) need to clarify when
the variable is updated.


v29.1

bug#65700: time when gcs-done is updated needs to be clarified

[Eli Zaretskii]

> From: Shynur Xie <one.last.kiss@outlook.com>
> Date: Sat, 2 Sep 2023 13:00:59 +0000
> msip_labels: 
> 
> I wrote a function which runs each time Emacs collects garbage, so I
> added it to `post-gc-hook'.
> 
> That function cares about the value of `gcs-done' and the value of
> `gc-elapsed'.
> But it seems that `gcs-done' is updated after `post-gc-hook' is
> executed?
> 
> Is this an intended behavior?  If so, I think the docstring of these
> variables (such as `gcs-done', `gc-elapsed', ...) need to clarify when
> the variable is updated.

Stefan, any comments?

bug#65700: time when gcs-done is updated needs to be clarified

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I wrote a function which runs each time Emacs collects garbage, so I
>> added it to `post-gc-hook'.
>> 
>> That function cares about the value of `gcs-done' and the value of
>> `gc-elapsed'.  But it seems that `gcs-done' is updated after
>> `post-gc-hook' is executed?

Indeed, from what I see in the code, `gc-elapsed` and `gcs-done` are
updated *after* running `post-gc-hook`.
This means for example that the time to run `post-gc-hook` is included
in the `gc-elapsed` time.

>> Is this an intended behavior?

Good question.  AFAICT this was added by the following commit:

    commit 2c5bd60800ce4f2702f263eda2145be342208ffe
    Author: Dave Love <fx@gnu.org>
    Date:   Thu Jan 30 14:15:58 2003 +0000
    
        (Vgc_elapsed, gcs_done): New variables.
        (Fgarbage_collect): Use them.
        (init_alloc, syms_of_alloc): Set them up.
    
    diff --git a/src/alloc.c b/src/alloc.c
    --- a/src/alloc.c
    +++ b/src/alloc.c
    @@ -4367,7 +4373,15 @@
       if (!NILP (Vpost_gc_hook))
         {
           int count = inhibit_garbage_collection ();
           safe_run_hooks (Qpost_gc_hook);
           unbind_to (count, Qnil);
         }
    -  
    +
    +  /* Accumulate statistics.  */
    +  EMACS_GET_TIME (t2);
    +  EMACS_SUB_TIME (t3, t2, t1);
    +  if (FLOATP (Vgc_elapsed))
    +    XSETFLOAT (Vgc_elapsed, make_float (XFLOAT_DATA (Vgc_elapsed) +
    +					EMACS_SECS (t3) +
    +					EMACS_USECS (t3) * 1.0e-6));
    +  gcs_done++;

The only related commit I see around that time is the associated NEWS
message:

    ** New variables `gc-elapsed' and `gcs-done' provide extra information
    on garbage collection.

So I strongly suspect that it was largely accidental.  "Philosophically",
both choices make sense (either consider `post-gc-hook` as being part of
the GC or consider it as external to the GC).

I'd be curious to know how it affects your code.


        Stefan

bug#65700: time when gcs-done is updated needs to be clarified

[Shynur Xie]

> Stefan:
> I'd be curious to know how it affects your code.

My Emacs displays its runtime information like this:
  <github.com/shynur/.emacs.d/issues/1#issuecomment-1671815270>
Since it's mainly related to GC, I have it updated after each GC.

I believe I'm not the first person to do this; why others didn't
report the bug may be that they don't care whether the number of GCs
is exactly the same as they expect or one less, so they didn't really
check when `gcs-done' is updated.
IOW, if the update time of `gcs-done' is changed, I think most people
won't notice this change at all.

> Stefan:
> "Philosophically", both choices make sense (either consider
> `post-gc-hook` as being part of the GC or consider it as external to
> the GC).

I'm not a native English speaker.  Does the POST in `post-gc-hook'
mean the functions will run when GC is DONE?

> shynur:
> Is this an intended behavior?  If so, I think the docstring of these
> variables (such as `gcs-done', `gc-elapsed', ...) need to clarify
> when the variable is updated.

And `garbage-collection-messages'.

bug#65700: time when gcs-done is updated needs to be clarified

[Eli Zaretskii]

> From: Shynur Xie <one.last.kiss@outlook.com>
> CC: "65700@debbugs.gnu.org" <65700@debbugs.gnu.org>
> Date: Thu, 7 Sep 2023 14:35:32 +0000
> msip_labels:
> 
> > Stefan:
> > I'd be curious to know how it affects your code.
> 
> My Emacs displays its runtime information like this:
>   <github.com/shynur/.emacs.d/issues/1#issuecomment-1671815270>
> Since it's mainly related to GC, I have it updated after each GC.

That's just causes off-by-one count of GC cycles, no?  Easy enough to
fix.

> > Stefan:
> > "Philosophically", both choices make sense (either consider
> > `post-gc-hook` as being part of the GC or consider it as external to
> > the GC).
> 
> I'm not a native English speaker.  Does the POST in `post-gc-hook'
> mean the functions will run when GC is DONE?

Its being "done" doesn't necessarily have to include updating the
count of GCs.  The main job of GC is already done by the time the hook
is called.

This is what Stefan meant when he said that philosophically it is not
clear-cut whether updating the count should be done before or after
calling the hook.

> > shynur:
> > Is this an intended behavior?  If so, I think the docstring of these
> > variables (such as `gcs-done', `gc-elapsed', ...) need to clarify
> > when the variable is updated.
> 
> And `garbage-collection-messages'.

What about it?

bug#65700: time when gcs-done is updated needs to be clarified

[Shynur Xie]

> Eli:
>
> > I'm not a native English speaker.  Does the POST in `post-gc-hook'
> > mean the functions will run when GC is DONE?
>
> Its being "done" doesn't necessarily have to include updating the
> count of GCs.  The main job of GC is already done by the time the
> hook is called.

Thanks for explaining.  Yes, "Its being 'done' doesn't necessarily
have to include updating the count of GCs."

My point is, IF "the functions will run when GC is DONE" THEN when
these functions are being executed, the `gcs-DONE' should have been
updated.

> Eli:
>
> > > Is this an intended behavior?  If so, I think the docstring of
> > > these variables (such as `gcs-done', `gc-elapsed', ...) need to
> > > clarify when the variable is updated.
> 
> > And `garbage-collection-messages'.
>
> What about it?

Mentioning it here is just for reference.  Maybe you want clarify when
the variable/message is updated/generated in the docstring.


> Stefan:
> Hmm... that doesn't really explain in which way this affects your
> code.

It does.

> `gcs-done` will be always smaller by 1

That's it.

bug#65700: time when gcs-done is updated needs to be clarified

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Hmm... that doesn't really explain in which way this affects your
>> code.
> It does.

At least not in a way that I understand, then :-(

>> `gcs-done` will be always smaller by 1
> That's it.

Why do you care if it starts at 4 rather than at 5?
Maybe we should make it start at some random number instead, to make it
clear that 0 is meaningless :-)


        Stefan

bug#65700: time when gcs-done is updated needs to be clarified

[Shynur Xie]

> Stefan:
> Why do you care if it starts at 4 rather than at 5?

To be honest, I don't really care; it's quite easy for me to fix this
in my function (although I'm a little freaked out that the behavior
doesn't match the docstring: "accumulated number of garbage
collections done.").  Even if I just leave it there, it's only one
less than it actually is.

But at least I should let Emacs developers know this.  If this is not
an intended behavior, they can fix it; if it is, they may make it
clear in docstring.

So the question is:  do you care?  or, do you think users care?

If the answer is no, then there's no need to change it, just clarify
this behavior in the docstring of `post-gc-hook'.

bug#65700: time when gcs-done is updated needs to be clarified

[Shynur Xie]

> shynur:
> My Emacs displays its runtime information like this:
>   <github.com/shynur/.emacs.d/issues/1#issuecomment-1671815270>
> Since it's mainly related to GC, I have it updated after each GC.

> Eli:
> That's just causes off-by-one count of GC cycles, no?  Easy enough
> to fix.

> Stefan:
> the only difference is that `gcs-done` will be always smaller by 1

We forgot the `gc-elapsed'.  It's a little hard to fix (dirty work is
needed).

bug#65700: time when gcs-done is updated needs to be clarified

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> My Emacs displays its runtime information like this:
>>   <github.com/shynur/.emacs.d/issues/1#issuecomment-1671815270>
>> Since it's mainly related to GC, I have it updated after each GC.
>
>> Eli:
>> That's just causes off-by-one count of GC cycles, no?  Easy enough
>> to fix.
>
>> Stefan:
>> the only difference is that `gcs-done` will be always smaller by 1
>
> We forgot the `gc-elapsed'.  It's a little hard to fix (dirty work is
> needed).

Yup, for `gcs-done`, it doesn't matter very much, but with the current
code `post-gc-hook` simply can't know how much time the last GC took,
which can be a useful information there.
I'll push my patch to `master` soon, I just need to clean it up first.


        Stefan

bug#65700: time when gcs-done is updated needs to be clarified

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I'll push my patch to `master` soon, I just need to clean it up first.

Done, closing,


        Stefan

bug#65700: time when gcs-done is updated needs to be clarified

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I'd be curious to know how it affects your code.
> My Emacs displays its runtime information like this:
>   <github.com/shynur/.emacs.d/issues/1#issuecomment-1671815270>
> Since it's mainly related to GC, I have it updated after each GC.

Hmm... that doesn't really explain in which way this affects your code.
If you only ever run that from `post-gc-hook` the only difference is
that `gcs-done` will be always smaller by 1 and since it doesn't start
at 0 anyway it's not really relevant: it will still increase by 1 every
time `post-gc-hook` is run.

FWIW, I tend to agree that it would be more useful to run `post-gc-hook`
after those values have been updated.  So that `post-gc-hook` can know
the exact duration of the GC that we just finished, rather than the one
before that.

Any objection to the patch below?


        Stefan


diff --git a/src/alloc.c b/src/alloc.c
index c77bdc6372d..cee43783387 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -6595,13 +6595,6 @@ garbage_collect (void)
   image_prune_animation_caches (false);
 #endif
 
-  if (!NILP (Vpost_gc_hook))
-    {
-      specpdl_ref gc_count = inhibit_garbage_collection ();
-      safe_run_hooks (Qpost_gc_hook);
-      unbind_to (gc_count, Qnil);
-    }
-
   /* Accumulate statistics.  */
   if (FLOATP (Vgc_elapsed))
     {
@@ -6620,6 +6613,13 @@ garbage_collect (void)
       if (tot_after < tot_before)
 	malloc_probe (min (tot_before - tot_after, SIZE_MAX));
     }
+
+  if (!NILP (Vpost_gc_hook))
+    {
+      specpdl_ref gc_count = inhibit_garbage_collection ();
+      safe_run_hooks (Qpost_gc_hook);
+      unbind_to (gc_count, Qnil);
+    }
 }
 
 DEFUN ("garbage-collect", Fgarbage_collect, Sgarbage_collect, 0, 0, "",

bug#65700: time when gcs-done is updated needs to be clarified

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  "65700@debbugs.gnu.org"
>  <65700@debbugs.gnu.org>
> Date: Thu, 07 Sep 2023 10:58:46 -0400
> 
> FWIW, I tend to agree that it would be more useful to run `post-gc-hook`
> after those values have been updated.  So that `post-gc-hook` can know
> the exact duration of the GC that we just finished, rather than the one
> before that.
> 
> Any objection to the patch below?

None here, but please call out this incompatible change in NEWS.