MPS: Crash when switching to buffer

MPS: Crash when switching to buffer

[Ihor Radchenko]

Thread 1 "emacs" hit Breakpoint 1, terminate_due_to_signal (sig=sig@entry=11, backtrace_limit=backtrace_limit@entry=40) at emacs.c:443
443	{
(gdb) bt
#0  terminate_due_to_signal (sig=sig@entry=11, backtrace_limit=backtrace_limit@entry=40) at emacs.c:443
#1  0x00005555556e446b in handle_fatal_signal (sig=sig@entry=11) at sysdep.c:1800
#2  0x00005555556e44d0 in deliver_thread_signal (sig=11, handler=0x5555556e4457 <handle_fatal_signal>) at sysdep.c:1792
#3  deliver_fatal_thread_signal (sig=sig@entry=11) at sysdep.c:1812
#4  0x00005555556e44fd in handle_sigsegv (sig=11, siginfo=<optimized out>, arg=<optimized out>) at sysdep.c:1950
#5  0x00007ffff3048050 in <signal handler called> () at /lib64/libc.so.6
#6  0x00007ffff304822b in kill () at /lib64/libc.so.6
#7  0x0000555555880af9 in sigHandle (sig=<optimized out>, info=<optimized out>, uap=<optimized out>) at /home/yantar92/Dist/mps/code/protsgix.c:114
#8  0x00007ffff3048050 in <signal handler called> () at /lib64/libc.so.6
#9  XCDR (c=XIL(0x32ec042e3)) at /home/yantar92/Git/emacs/src/lisp.h:1522
#10 plist_get (plist=<optimized out>, prop=prop@entry=XIL(0x8220)) at fns.c:2611
#11 0x000055555575a8de in Fget (symbol=symbol@entry=XIL(0x2aaa92d6f288), propname=propname@entry=XIL(0x8220)) at fns.c:2631
#12 0x0000555555664370 in resolve_face_name (face_name=XIL(0x2aaa92d6f288), signal_p=signal_p@entry=false) at xfaces.c:2004
#13 0x0000555555669e57 in get_lface_attributes
    (w=w@entry=0x7fffef8a0068, f=f@entry=0x7fffe26fd610, face_name=<optimized out>, attrs=attrs@entry=0x7fffffff5270, signal_p=signal_p@entry=false, named_merge_points=<optimized out>, named_merge_points@entry=0x0) at xfaces.c:2105
#14 0x0000555555670ea9 in lookup_derived_face
    (w=0x7fffef8a0068, f=f@entry=0x7fffe26fd610, symbol=<optimized out>, face_id=face_id@entry=45, signal_p=signal_p@entry=false) at xfaces.c:5279
#15 0x0000555555671ef4 in merge_faces (w=<optimized out>, face_name=<optimized out>, face_name@entry=XIL(0x30), face_id=<optimized out>, base_face_id=45)
    at xfaces.c:7128
#16 0x00005555555c0751 in next_element_from_display_vector (it=0x7fffffff7dc0) at xdisp.c:9045
#17 0x00005555555dc636 in next_element_from_buffer (it=0x7fffffff7dc0) at xdisp.c:9649
#18 0x00005555555da85e in get_next_display_element (it=it@entry=0x7fffffff7dc0) at xdisp.c:8213
#19 0x00005555555e42bf in display_line (it=it@entry=0x7fffffff7dc0, cursor_vpos=cursor_vpos@entry=1) at xdisp.c:25319
#20 0x00005555555e709c in try_window (window=window@entry=XIL(0x7fffef8a006d), pos=..., flags=flags@entry=1) at xdisp.c:21144
#21 0x0000555555604aab in redisplay_window (window=XIL(0x7fffef8a006d), just_this_one_p=just_this_one_p@entry=false) at xdisp.c:20538
#22 0x0000555555607233 in redisplay_window_0 (window=window@entry=XIL(0x7fffef8a006d)) at xdisp.c:18023
#23 0x0000555555748d10 in internal_condition_case_1
    (bfun=bfun@entry=0x555555607200 <redisplay_window_0>, arg=arg@entry=XIL(0x7fffef8a006d), handlers=<optimized out>, hfun=hfun@entry=0x5555555c0412 <redisplay_window_error>) at eval.c:1653
#24 0x00005555555bdeee in redisplay_windows (window=XIL(0x7fffef8a006d)) at xdisp.c:17992
#25 0x00005555555f1d79 in redisplay_internal () at xdisp.c:17391
#26 0x00005555555f26ea in redisplay () at xdisp.c:16566
#27 0x00005555556d4c8e in read_char
    (commandflag=1, map=map@entry=XIL(0x7fffa5134703), prev_event=XIL(0), used_mouse_menu=used_mouse_menu@entry=0x7fffffffd30b, end_time=end_time@entry=0x0)
    at keyboard.c:2689
#28 0x00005555556d768a in read_key_sequence
    (keybuf=keybuf@entry=0x7fffffffd470, prompt=prompt@entry=XIL(0), dont_downcase_last=dont_downcase_last@entry=false, can_return_switch_frame=can_return_switch_frame@entry=true, fix_current_buffer=fix_current_buffer@entry=true, prevent_redisplay=prevent_redisplay@entry=false, disable_text_conversion_p=false)
    at keyboard.c:10759
#29 0x00005555556da3f3 in command_loop_1 () at keyboard.c:1440
--Type <RET> for more, q to quit, c to continue without paging--c
#30 0x0000555555748c92 in internal_condition_case
    (bfun=bfun@entry=0x5555556d9208 <command_loop_1>, handlers=handlers@entry=XIL(0x90), hfun=hfun@entry=0x5555556ca60c <cmd_error>) at eval.c:1629
#31 0x00005555556c5838 in command_loop_2 (handlers=handlers@entry=XIL(0x90)) at keyboard.c:1179
#32 0x0000555555748bc4 in internal_catch (tag=tag@entry=XIL(0x12360), func=func@entry=0x5555556c5811 <command_loop_2>, arg=arg@entry=XIL(0x90))
    at eval.c:1308
#33 0x00005555556c57ee in command_loop () at keyboard.c:1157
#34 0x00005555556ca1c1 in recursive_edit_1 () at keyboard.c:765
#35 0x00005555556ca52e in Frecursive_edit () at keyboard.c:848
#36 0x00005555556c4aad in main (argc=1, argv=<optimized out>) at emacs.c:2651

Lisp Backtrace:
"redisplay_internal (C function)" (0x0)

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: MPS: Crash when switching to buffer

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Gerd Möllmann <gerd.moellmann@gmail.com>, Eli Zaretskii
>  <eliz@gnu.org>,
>  eller.helmut@gmail.com
> Date: Mon, 01 Jul 2024 09:26:25 +0000
> 
> 
> Thread 1 "emacs" hit Breakpoint 1, terminate_due_to_signal (sig=sig@entry=11, backtrace_limit=backtrace_limit@entry=40) at emacs.c:443
> 443	{
> (gdb) bt
> #0  terminate_due_to_signal (sig=sig@entry=11, backtrace_limit=backtrace_limit@entry=40) at emacs.c:443
> #1  0x00005555556e446b in handle_fatal_signal (sig=sig@entry=11) at sysdep.c:1800
> #2  0x00005555556e44d0 in deliver_thread_signal (sig=11, handler=0x5555556e4457 <handle_fatal_signal>) at sysdep.c:1792
> #3  deliver_fatal_thread_signal (sig=sig@entry=11) at sysdep.c:1812
> #4  0x00005555556e44fd in handle_sigsegv (sig=11, siginfo=<optimized out>, arg=<optimized out>) at sysdep.c:1950
> #5  0x00007ffff3048050 in <signal handler called> () at /lib64/libc.so.6
> #6  0x00007ffff304822b in kill () at /lib64/libc.so.6
> #7  0x0000555555880af9 in sigHandle (sig=<optimized out>, info=<optimized out>, uap=<optimized out>) at /home/yantar92/Dist/mps/code/protsgix.c:114
> #8  0x00007ffff3048050 in <signal handler called> () at /lib64/libc.so.6
> #9  XCDR (c=XIL(0x32ec042e3)) at /home/yantar92/Git/emacs/src/lisp.h:1522
> #10 plist_get (plist=<optimized out>, prop=prop@entry=XIL(0x8220)) at fns.c:2611
> #11 0x000055555575a8de in Fget (symbol=symbol@entry=XIL(0x2aaa92d6f288), propname=propname@entry=XIL(0x8220)) at fns.c:2631
> #12 0x0000555555664370 in resolve_face_name (face_name=XIL(0x2aaa92d6f288), signal_p=signal_p@entry=false) at xfaces.c:2004
> #13 0x0000555555669e57 in get_lface_attributes
>     (w=w@entry=0x7fffef8a0068, f=f@entry=0x7fffe26fd610, face_name=<optimized out>, attrs=attrs@entry=0x7fffffff5270, signal_p=signal_p@entry=false, named_merge_points=<optimized out>, named_merge_points@entry=0x0) at xfaces.c:2105
> #14 0x0000555555670ea9 in lookup_derived_face
>     (w=0x7fffef8a0068, f=f@entry=0x7fffe26fd610, symbol=<optimized out>, face_id=face_id@entry=45, signal_p=signal_p@entry=false) at xfaces.c:5279
> #15 0x0000555555671ef4 in merge_faces (w=<optimized out>, face_name=<optimized out>, face_name@entry=XIL(0x30), face_id=<optimized out>, base_face_id=45)
>     at xfaces.c:7128
> #16 0x00005555555c0751 in next_element_from_display_vector (it=0x7fffffff7dc0) at xdisp.c:9045

This is not a crash when switching buffers, this is a crash in
redisplay, when merging faces for display.

What exactly is the cause of segfault in plist_get?  Please look at
the data there in the debugger and see which one(s) are invalid or
corrupted.  From the backtrace it sounds like the plist of the face is
botched, but that's a guess.

Is this reproducible? I tried switching between two buffers and didn't
get any crashes here, FWIW.

Re: MPS: Crash when switching to buffer

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

> This is not a crash when switching buffers, this is a crash in
> redisplay, when merging faces for display.
>
> What exactly is the cause of segfault in plist_get?  Please look at
> the data there in the debugger and see which one(s) are invalid or
> corrupted.  From the backtrace it sounds like the plist of the face is
> botched, but that's a guess.

May you please provide more detailed instructions?

> Is this reproducible? I tried switching between two buffers and didn't
> get any crashes here, FWIW.

I've seen it multiple times after a few hours of using Emacs.
So, it is not reproducible reliably, unfortunately.
Next time, I will leave the gdb session running, so that I can check the
state in more details if necessary.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: MPS: Crash when switching to buffer

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: emacs-devel@gnu.org, gerd.moellmann@gmail.com, eller.helmut@gmail.com
> Date: Mon, 01 Jul 2024 12:13:25 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > This is not a crash when switching buffers, this is a crash in
> > redisplay, when merging faces for display.
> >
> > What exactly is the cause of segfault in plist_get?  Please look at
> > the data there in the debugger and see which one(s) are invalid or
> > corrupted.  From the backtrace it sounds like the plist of the face is
> > botched, but that's a guess.
> 
> May you please provide more detailed instructions?

Below.

> > Is this reproducible? I tried switching between two buffers and didn't
> > get any crashes here, FWIW.
> 
> I've seen it multiple times after a few hours of using Emacs.
> So, it is not reproducible reliably, unfortunately.
> Next time, I will leave the gdb session running, so that I can check the
> state in more details if necessary.

Thanks.  The commands below should be a starting point:

  (gdb) fr 10

Make sure frame #10 is indeed in plist_get, and adjust the frame
number if not.

  (gdb) p tail
  (gdb) xtype
  (gdb) p prop
  (gdb) xtype
  (gdb) xsymbol

Finally, for the brave:

  (gdb) pp tail

Re: MPS: Crash when switching to buffer

[Pip Cet]

On Monday, July 1st, 2024 at 12:04, Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Ihor Radchenko yantar92@posteo.net
> 
> > Cc: Gerd Möllmann gerd.moellmann@gmail.com, Eli Zaretskii
> > eliz@gnu.org,
> > eller.helmut@gmail.com
> > Date: Mon, 01 Jul 2024 09:26:25 +0000
> > 
> > Thread 1 "emacs" hit Breakpoint 1, terminate_due_to_signal (sig=sig@entry=11, backtrace_limit=backtrace_limit@entry=40) at emacs.c:443
> > 443 {
> > (gdb) bt
> > #0 terminate_due_to_signal (sig=sig@entry=11, backtrace_limit=backtrace_limit@entry=40) at emacs.c:443
> > #1 0x00005555556e446b in handle_fatal_signal (sig=sig@entry=11) at sysdep.c:1800
> > #2 0x00005555556e44d0 in deliver_thread_signal (sig=11, handler=0x5555556e4457 <handle_fatal_signal>) at sysdep.c:1792
> > #3 deliver_fatal_thread_signal (sig=sig@entry=11) at sysdep.c:1812
> > #4 0x00005555556e44fd in handle_sigsegv (sig=11, siginfo=<optimized out>, arg=<optimized out>) at sysdep.c:1950
> > #5 0x00007ffff3048050 in <signal handler called> () at /lib64/libc.so.6
> > #6 0x00007ffff304822b in kill () at /lib64/libc.so.6
> > #7 0x0000555555880af9 in sigHandle (sig=<optimized out>, info=<optimized out>, uap=<optimized out>) at /home/yantar92/Dist/mps/code/protsgix.c:114
> > #8 0x00007ffff3048050 in <signal handler called> () at /lib64/libc.so.6
> > #9 XCDR (c=XIL(0x32ec042e3)) at /home/yantar92/Git/emacs/src/lisp.h:1522
> > #10 plist_get (plist=<optimized out>, prop=prop@entry=XIL(0x8220)) at fns.c:2611
> > #11 0x000055555575a8de in Fget (symbol=symbol@entry=XIL(0x2aaa92d6f288), propname=propname@entry=XIL(0x8220)) at fns.c:2631
> > #12 0x0000555555664370 in resolve_face_name (face_name=XIL(0x2aaa92d6f288), signal_p=signal_p@entry=false) at xfaces.c:2004
> > #13 0x0000555555669e57 in get_lface_attributes
> > (w=w@entry=0x7fffef8a0068, f=f@entry=0x7fffe26fd610, face_name=<optimized out>, attrs=attrs@entry=0x7fffffff5270, signal_p=signal_p@entry=false, named_merge_points=<optimized out>, named_merge_points@entry=0x0) at xfaces.c:2105
> > #14 0x0000555555670ea9 in lookup_derived_face
> > (w=0x7fffef8a0068, f=f@entry=0x7fffe26fd610, symbol=<optimized out>, face_id=face_id@entry=45, signal_p=signal_p@entry=false) at xfaces.c:5279
> > #15 0x0000555555671ef4 in merge_faces (w=<optimized out>, face_name=<optimized out>, face_name@entry=XIL(0x30), face_id=<optimized out>, base_face_id=45)
> > at xfaces.c:7128
> > #16 0x00005555555c0751 in next_element_from_display_vector (it=0x7fffffff7dc0) at xdisp.c:9045

At first glance, the lface_id_to_name "vector" (just a pointer to Lisp_Object, not a Lisp_Vector) isn't allocated using igc methods, so references in it might not be traced. Is that possible, Gerd? I confess I'm not totally sure how xmalloc and friends get translated in the MPS build...

Pip

Re: MPS: Crash when switching to buffer

[Gerd Möllmann]

Pip Cet <pipcet@protonmail.com> writes:

> On Monday, July 1st, 2024 at 12:04, Eli Zaretskii <eliz@gnu.org> wrote:
>> > From: Ihor Radchenko yantar92@posteo.net
>> 
>> > Cc: Gerd Möllmann gerd.moellmann@gmail.com, Eli Zaretskii
>> > eliz@gnu.org,
>> > eller.helmut@gmail.com
>> > Date: Mon, 01 Jul 2024 09:26:25 +0000
>> > 
>> > Thread 1 "emacs" hit Breakpoint 1, terminate_due_to_signal (sig=sig@entry=11, backtrace_limit=backtrace_limit@entry=40) at emacs.c:443
>> > 443 {
>> > (gdb) bt
>> > #0 terminate_due_to_signal (sig=sig@entry=11, backtrace_limit=backtrace_limit@entry=40) at emacs.c:443
>> > #1 0x00005555556e446b in handle_fatal_signal (sig=sig@entry=11) at sysdep.c:1800
>> > #2 0x00005555556e44d0 in deliver_thread_signal (sig=11, handler=0x5555556e4457 <handle_fatal_signal>) at sysdep.c:1792
>> > #3 deliver_fatal_thread_signal (sig=sig@entry=11) at sysdep.c:1812
>> > #4 0x00005555556e44fd in handle_sigsegv (sig=11, siginfo=<optimized out>, arg=<optimized out>) at sysdep.c:1950
>> > #5 0x00007ffff3048050 in <signal handler called> () at /lib64/libc.so.6
>> > #6 0x00007ffff304822b in kill () at /lib64/libc.so.6
>> > #7 0x0000555555880af9 in sigHandle (sig=<optimized out>, info=<optimized out>, uap=<optimized out>) at /home/yantar92/Dist/mps/code/protsgix.c:114
>> > #8 0x00007ffff3048050 in <signal handler called> () at /lib64/libc.so.6
>> > #9 XCDR (c=XIL(0x32ec042e3)) at /home/yantar92/Git/emacs/src/lisp.h:1522
>> > #10 plist_get (plist=<optimized out>, prop=prop@entry=XIL(0x8220)) at fns.c:2611
>> > #11 0x000055555575a8de in Fget (symbol=symbol@entry=XIL(0x2aaa92d6f288), propname=propname@entry=XIL(0x8220)) at fns.c:2631
>> > #12 0x0000555555664370 in resolve_face_name (face_name=XIL(0x2aaa92d6f288), signal_p=signal_p@entry=false) at xfaces.c:2004
>> > #13 0x0000555555669e57 in get_lface_attributes
>> > (w=w@entry=0x7fffef8a0068, f=f@entry=0x7fffe26fd610,
>> > face_name=<optimized out>, attrs=attrs@entry=0x7fffffff5270,
>> > signal_p=signal_p@entry=false, named_merge_points=<optimized out>,
>> > named_merge_points@entry=0x0) at xfaces.c:2105
>> > #14 0x0000555555670ea9 in lookup_derived_face
>> > (w=0x7fffef8a0068, f=f@entry=0x7fffe26fd610, symbol=<optimized out>, face_id=face_id@entry=45, signal_p=signal_p@entry=false) at xfaces.c:5279
>> > #15 0x0000555555671ef4 in merge_faces (w=<optimized out>, face_name=<optimized out>, face_name@entry=XIL(0x30), face_id=<optimized out>, base_face_id=45)
>> > at xfaces.c:7128
>> > #16 0x00005555555c0751 in next_element_from_display_vector (it=0x7fffffff7dc0) at xdisp.c:9045
>
> At first glance, the lface_id_to_name "vector" (just a pointer to
> Lisp_Object, not a Lisp_Vector) isn't allocated using igc methods, so
> references in it might not be traced. Is that possible, Gerd? I
> confess I'm not totally sure how xmalloc and friends get translated in
> the MPS build...

That's very possible, and a bug :-). Good catch!

If xmalloc/xfree and friends are used to alloc memory that contains
references, we have replaced them with oen of these

  void *igc_xzalloc_ambig (size_t size);
  void *igc_realloc_ambig (void *block, size_t size);
  void igc_xfree (void *p);
  Lisp_Object *igc_xalloc_lisp_objs_exact (size_t n);

  void *igc_xpalloc_ambig (void *pa, ptrdiff_t *nitems,
                           ptrdiff_t nitems_incr_min, ptrdiff_t nitems_max,
                           ptrdiff_t item_size);
  void igc_xpalloc_exact (void **pa_cell, ptrdiff_t *nitems,
                          ptrdiff_t nitems_incr_min, ptrdiff_t nitems_max,
                          ptrdiff_t item_size, igc_scan_area_t scan);

  void *igc_xnrealloc_ambig (void *pa, ptrdiff_t nitems, ptrdiff_t item_size);

Hope I got them all. All of them create roots, the ambig variants
ambiguous roots, the exact variants exact roots. igc_xfree must be used
instead of xfree so that the roots get destroyed when the memory is freed.

Re: MPS: Crash when switching to buffer

[Pip Cet]

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

On Monday, July 1st, 2024 at 14:42, Gerd Möllmann <gerd.moellmann@gmail.com> wrote:
> > At first glance, the lface_id_to_name "vector" (just a pointer to
> > Lisp_Object, not a Lisp_Vector) isn't allocated using igc methods, so
> > references in it might not be traced. Is that possible, Gerd? I
> > confess I'm not totally sure how xmalloc and friends get translated in
> > the MPS build...
> 
> That's very possible, and a bug :-). Good catch!
> 
> If xmalloc/xfree and friends are used to alloc memory that contains
> references, we have replaced them with oen of these
> 
> void *igc_xzalloc_ambig (size_t size);
> void *igc_realloc_ambig (void *block, size_t size);
> void igc_xfree (void *p);
> Lisp_Object *igc_xalloc_lisp_objs_exact (size_t n);
> 
> void *igc_xpalloc_ambig (void *pa, ptrdiff_t *nitems,
> ptrdiff_t nitems_incr_min, ptrdiff_t nitems_max,
> ptrdiff_t item_size);
> void igc_xpalloc_exact (void **pa_cell, ptrdiff_t *nitems,
> ptrdiff_t nitems_incr_min, ptrdiff_t nitems_max,
> ptrdiff_t item_size, igc_scan_area_t scan);
> 
> void *igc_xnrealloc_ambig (void *pa, ptrdiff_t nitems, ptrdiff_t item_size);
> 
> Hope I got them all. All of them create roots, the ambig variants
> ambiguous roots, the exact variants exact roots. igc_xfree must be used
> instead of xfree so that the roots get destroyed when the memory is freed.

Thanks, that helps clarify things. So would something like the following work? lface_id_to_name is never freed, it seems...

Ihor, I'm not sure whether the bug is fully reproducible. If it is, could you please try this?

diff --git a/src/xfaces.c b/src/xfaces.c
index 2bdd2f660fd..ee26a260ed4 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -2970,9 +2970,15 @@ DEFUN ("internal-make-lisp-face", Finternal_make_lisp_face,
 	 The mapping from Lisp face to Lisp face id is given by the
 	 property `face' of the Lisp face name.  */
       if (next_lface_id == lface_id_to_name_size)
+#ifdef HAVE_MPS
+	lface_id_to_name =
+	  igc_xpalloc_ambig (lface_id_to_name, &lface_id_to_name_size, 1, MAX_FACE_ID,
+			     sizeof *lface_id_to_name);
+#else
 	lface_id_to_name =
 	  xpalloc (lface_id_to_name, &lface_id_to_name_size, 1, MAX_FACE_ID,
 		   sizeof *lface_id_to_name);
+#endif
 
       Lisp_Object face_id = make_fixnum (next_lface_id);
       lface_id_to_name[next_lface_id] = face;
@@ -7326,7 +7332,11 @@ init_xfaces (void)
 	{
 	  /* Allocate the lface_id_to_name[] array.  */
 	  lface_id_to_name_size = next_lface_id = nfaces;
+#ifdef HAVE_MPS
+	  lface_id_to_name = igc_xzalloc_ambig (next_lface_id * sizeof *lface_id_to_name);
+#else
 	  lface_id_to_name = xnmalloc (next_lface_id, sizeof *lface_id_to_name);
+#endif
 
 	  /* Store the faces.  */
 	  struct Lisp_Hash_Table* table = XHASH_TABLE (Vface_new_frame_defaults);

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-try-to-fix-face-related-crashes.patch --]
[-- Type: text/x-patch; name=0002-try-to-fix-face-related-crashes.patch, Size: 1488 bytes --]

From 7debd69b331f2068c7d0d6975263b2f8ea089fe4 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@protonmail.com>
Date: Tue, 2 Jul 2024 00:18:58 +0000
Subject: [PATCH 2/2] try to fix face-related crashes

---
 src/xfaces.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/xfaces.c b/src/xfaces.c
index 2bdd2f660fd..ee26a260ed4 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -2970,9 +2970,15 @@ DEFUN ("internal-make-lisp-face", Finternal_make_lisp_face,
 	 The mapping from Lisp face to Lisp face id is given by the
 	 property `face' of the Lisp face name.  */
       if (next_lface_id == lface_id_to_name_size)
+#ifdef HAVE_MPS
+	lface_id_to_name =
+	  igc_xpalloc_ambig (lface_id_to_name, &lface_id_to_name_size, 1, MAX_FACE_ID,
+			     sizeof *lface_id_to_name);
+#else
 	lface_id_to_name =
 	  xpalloc (lface_id_to_name, &lface_id_to_name_size, 1, MAX_FACE_ID,
 		   sizeof *lface_id_to_name);
+#endif
 
       Lisp_Object face_id = make_fixnum (next_lface_id);
       lface_id_to_name[next_lface_id] = face;
@@ -7326,7 +7332,11 @@ init_xfaces (void)
 	{
 	  /* Allocate the lface_id_to_name[] array.  */
 	  lface_id_to_name_size = next_lface_id = nfaces;
+#ifdef HAVE_MPS
+	  lface_id_to_name = igc_xzalloc_ambig (next_lface_id * sizeof *lface_id_to_name);
+#else
 	  lface_id_to_name = xnmalloc (next_lface_id, sizeof *lface_id_to_name);
+#endif
 
 	  /* Store the faces.  */
 	  struct Lisp_Hash_Table* table = XHASH_TABLE (Vface_new_frame_defaults);
-- 
2.45.2

Re: MPS: Crash when switching to buffer

[Gerd Möllmann]

Pip Cet <pipcet@protonmail.com> writes:

> Thanks, that helps clarify things. So would something like the
> following work? lface_id_to_name is never freed, it seems...

Yes, that works 👍

> Ihor, I'm not sure whether the bug is fully reproducible. If it is, could you please try this?
>
> diff --git a/src/xfaces.c b/src/xfaces.c
> index 2bdd2f660fd..ee26a260ed4 100644
> --- a/src/xfaces.c
> +++ b/src/xfaces.c
> @@ -2970,9 +2970,15 @@ DEFUN ("internal-make-lisp-face", Finternal_make_lisp_face,
>  	 The mapping from Lisp face to Lisp face id is given by the
>  	 property `face' of the Lisp face name.  */
>        if (next_lface_id == lface_id_to_name_size)
> +#ifdef HAVE_MPS
> +	lface_id_to_name =
> +	  igc_xpalloc_ambig (lface_id_to_name, &lface_id_to_name_size, 1, MAX_FACE_ID,
> +			     sizeof *lface_id_to_name);
> +#else
>  	lface_id_to_name =
>  	  xpalloc (lface_id_to_name, &lface_id_to_name_size, 1, MAX_FACE_ID,
>  		   sizeof *lface_id_to_name);
> +#endif
>  
>        Lisp_Object face_id = make_fixnum (next_lface_id);
>        lface_id_to_name[next_lface_id] = face;
> @@ -7326,7 +7332,11 @@ init_xfaces (void)
>  	{
>  	  /* Allocate the lface_id_to_name[] array.  */
>  	  lface_id_to_name_size = next_lface_id = nfaces;
> +#ifdef HAVE_MPS
> +	  lface_id_to_name = igc_xzalloc_ambig (next_lface_id * sizeof *lface_id_to_name);
> +#else
>  	  lface_id_to_name = xnmalloc (next_lface_id, sizeof *lface_id_to_name);
> +#endif
>  
>  	  /* Store the faces.  */
>  	  struct Lisp_Hash_Table* table = XHASH_TABLE (Vface_new_frame_defaults);

Alternatively, we could also use

  Lisp_Object *igc_alloc_lisp_obj_vec (size_t n)

to allocate the vector. That returns an object of type IGC_OBJ_OBJ_VEC
in MPS that eventually gets collected. We would then have to use

  void igc_root_create_exact_ptr (void *var_addr)

for the variable holding the reference to the object, where the
staticpros are done.

Just wanted to mention that this also exists, and has the advantage of
fewer ambiguous references.

Re: MPS: Crash when switching to buffer

[Ihor Radchenko]

Pip Cet <pipcet@protonmail.com> writes:

>> void *igc_xnrealloc_ambig (void *pa, ptrdiff_t nitems, ptrdiff_t item_size);
>> 
>> Hope I got them all. All of them create roots, the ambig variants
>> ambiguous roots, the exact variants exact roots. igc_xfree must be used
>> instead of xfree so that the roots get destroyed when the memory is freed.
>
> Thanks, that helps clarify things. So would something like the following work? lface_id_to_name is never freed, it seems...
>
> Ihor, I'm not sure whether the bug is fully reproducible. If it is, could you please try this?

Alas, I cannot reproduce the bug reliably. But I installed the
patch. Will see if the crash happens again. But testing may take
anywhere between hours to days. The crash was not frequent to start
with.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: MPS: Crash when switching to buffer

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

>> Ihor, I'm not sure whether the bug is fully reproducible. If it is, could you please try this?
>
> Alas, I cannot reproduce the bug reliably. But I installed the
> patch. Will see if the crash happens again. But testing may take
> anywhere between hours to days. The crash was not frequent to start
> with.

So far, no crashes.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: MPS: Crash when switching to buffer

[Gerd Möllmann]

Ihor Radchenko <yantar92@posteo.net> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>>> Ihor, I'm not sure whether the bug is fully reproducible. If it is, could you please try this?
>>
>> Alas, I cannot reproduce the bug reliably. But I installed the
>> patch. Will see if the crash happens again. But testing may take
>> anywhere between hours to days. The crash was not frequent to start
>> with.
>
> So far, no crashes.

Hi Ihor. Soemthing completely unrelated, but I think it might be
interesting to play with if you are running an igc Emacs more often:

  igc-step-interval is a variable defined in ‘C source code’.

  Its value is 0.1

  How much time is MPS allowed to spend in GC when Emacs is idle.
  The value is in seconds, and should be a non-negative number.  It can
  be either an integer or a float.  The default value is 0 which means .
  don’t do something when idle.  Negative values and values that are not numbers
  are handled as if they were the default value.

It's for interactive use.

MPS: User GC customizations (was: MPS: Crash when switching to buffer)

[Ihor Radchenko]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Hi Ihor. Soemthing completely unrelated, but I think it might be
> interesting to play with if you are running an igc Emacs more often:
>
>   igc-step-interval is a variable defined in ‘C source code’.
>
>   Its value is 0.1
>
>   How much time is MPS allowed to spend in GC when Emacs is idle.
>   The value is in seconds, and should be a non-negative number.  It can
>   be either an integer or a float.  The default value is 0 which means .
>   don’t do something when idle.  Negative values and values that are not numbers
>   are handled as if they were the default value.
>
> It's for interactive use.

IMHO, this variable makes little sense in isolation. It only makes sense
to postpone GCs when we also have the means to increase the thresholds
when to trigger them. Most of the time, GCs are triggered by
memory-intensive commands. Extra GC on idle would not help those.

For example, my recent measurement of building agendas displayed 30% of
the time spend in GC. (whatever this means in the context of our handling
of SIGPRF)

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: MPS: User GC customizations

[Gerd Möllmann]

Ihor Radchenko <yantar92@posteo.net> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Hi Ihor. Soemthing completely unrelated, but I think it might be
>> interesting to play with if you are running an igc Emacs more often:
>>
>>   igc-step-interval is a variable defined in ‘C source code’.
>>
>>   Its value is 0.1
>>
>>   How much time is MPS allowed to spend in GC when Emacs is idle.
>>   The value is in seconds, and should be a non-negative number.  It can
>>   be either an integer or a float.  The default value is 0 which means .
>>   don’t do something when idle.  Negative values and values that are not numbers
>>   are handled as if they were the default value.
>>
>> It's for interactive use.
>
> IMHO, this variable makes little sense in isolation. It only makes sense
> to postpone GCs when we also have the means to increase the thresholds
> when to trigger them. Most of the time, GCs are triggered by
> memory-intensive commands. Extra GC on idle would not help those.

Define GC, so to speak, and define what triggering the GC means in a
concurrent, incremental GC :-)

What this variable does is give MPS notice that the client is currently
idle and it might be a good time to do some work.

> For example, my recent measurement of building agendas displayed 30% of
> the time spend in GC. (whatever this means in the context of our handling
> of SIGPRF)

Exactly, whqt does it mean? And if we don't know, why is it an example
for anything?

Re: MPS: User GC customizations

[Ihor Radchenko]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

>>>   igc-step-interval is a variable defined in ‘C source code’.
>>> ...
>> IMHO, this variable makes little sense in isolation. It only makes sense
>> to postpone GCs when we also have the means to increase the thresholds
>> when to trigger them. Most of the time, GCs are triggered by
>> memory-intensive commands. Extra GC on idle would not help those.
>
> Define GC, so to speak, and define what triggering the GC means in a
> concurrent, incremental GC :-)

AFAIU, the points of interests from real-time performance perspective
are (1) when MPS has to pause the threads to do its job and how we can
tweak it; (2) how long does it take to scan the arena to allocate new
objects (synchronous process) and how this time is influenced by MPS
settings.

> What this variable does is give MPS notice that the client is currently
> idle and it might be a good time to do some work.

Without understanding what effect setting this variable has on the Emacs
responsiveness, it does not seem very useful. (Exactly because MPS is
concurrent)

>> For example, my recent measurement of building agendas displayed 30% of
>> the time spend in GC. (whatever this means in the context of our handling
>> of SIGPRF)
>
> Exactly, whqt does it mean? And if we don't know, why is it an example
> for anything?

AFAIU, on master, SIGPROF handled while our vanilla GC is running
will record it. In contrast, on scratch/igc, SIGPROF will put all the
time when igc_busy_p () is non-nil into "GC".

And igc_busy_p is not only non-nil when MPS is pausing Emacs to do its
job, but also during object allocation. So, on master, profiler "GC"
field records real GC pauses, while on scratch/igc "GC" field is GC
pauses + new object allocation.

My figure of 30% says that igc_busy_p () is for 30% of CPU time, which
is a significant number. But it is not very useful unless we get some
idea about which part of it is memory allocation and which part of it is
MPS pausing all Emacs threads.

Ideally, we should also have some way to get the allocation times on
master. Then, we can compare them directly.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: MPS: User GC customizations

[Gerd Möllmann]

Ihor Radchenko <yantar92@posteo.net> writes:

>> Define GC, so to speak, and define what triggering the GC means in a
>> concurrent, incremental GC :-)
>
> AFAIU, the points of interests from real-time performance perspective
> are (1) when MPS has to pause the threads to do its job and how we can
> tweak it; (2) how long does it take to scan the arena to allocate new
> objects (synchronous process) and how this time is influenced by MPS
> settings.

Right. Alas, we don't have something for that. (And I don't know how to
do it either.)

>
>> What this variable does is give MPS notice that the client is currently
>> idle and it might be a good time to do some work.
>
> Without understanding what effect setting this variable has on the Emacs
> responsiveness, it does not seem very useful. (Exactly because MPS is
> concurrent)

That was kind of my point. We can use idle time for using this variable
and see what effect it has, especially in interactive use. Sorry if that
wasn't clear. I'm personally trying 0.1 (100ms) here at the moment.

>>> For example, my recent measurement of building agendas displayed 30% of
>>> the time spend in GC. (whatever this means in the context of our handling
>>> of SIGPRF)
>>
>> Exactly, whqt does it mean? And if we don't know, why is it an example
>> for anything?
>
> AFAIU, on master, SIGPROF handled while our vanilla GC is running
> will record it. In contrast, on scratch/igc, SIGPROF will put all the
> time when igc_busy_p () is non-nil into "GC".

Right. And I wonder if that simply is because MPS is doing stuff in its
own thread.

> And igc_busy_p is not only non-nil when MPS is pausing Emacs to do its
> job, but also during object allocation. So, on master, profiler "GC"
> field records real GC pauses, while on scratch/igc "GC" field is GC
> pauses + new object allocation.

The docs say

 -- C Function: *note mps_bool_t: 129. mps_arena_busy (mps_arena_t
          arena)

     Return true if an *note arena: 16. is part of the way through
     execution of an operation, false otherwise.

     ‘arena’ is the arena.

          Note: This function is intended to assist with debugging fatal
          errors in the *note client program: d0.  It is not expected to
          be needed in normal use.  If you find yourself wanting to use
          this function other than in the use case described below,
          there may be a better way to meet your requirements: please
          *note contact us: d8.

What "partly through an operation" means is anyone's guess at this
point. Someone would have to consult the sources. The docs don't say
what you are suggesting, from my POV.

> My figure of 30% says that igc_busy_p () is for 30% of CPU time, which
> is a significant number. But it is not very useful unless we get some
> idea about which part of it is memory allocation and which part of it is
> MPS pausing all Emacs threads.
>
> Ideally, we should also have some way to get the allocation times on
> master. Then, we can compare them directly.

Maybe it would be interesting what the meansurements look like on macOS,
where the check for igc_busy are not needed in the SIGPROF handler.

Anyway. I'm not working on this, I just wanted to promote the variable
as a knob that one can play with.

Re: MPS: User GC customizations

[Pip Cet]

On Thursday, July 4th, 2024 at 14:45, Gerd Möllmann <gerd.moellmann@gmail.com> wrote:
> Ihor Radchenko yantar92@posteo.net writes:
> 
> > > Define GC, so to speak, and define what triggering the GC means in a
> > > concurrent, incremental GC :-)
> > 
> > AFAIU, the points of interests from real-time performance perspective
> > are (1) when MPS has to pause the threads to do its job and how we can
> > tweak it; (2) how long does it take to scan the arena to allocate new
> > objects (synchronous process) and how this time is influenced by MPS
> > settings.
> 
> Right. Alas, we don't have something for that. (And I don't know how to
> do it either.)

I think we can just set flags for "called MPS" and "in a scan function" and look at them in the SIGPROF handler to distinguish the four cases? 

My suspicion is that most problems are going to be due to large objects creating large segments which we have to scan completely, but that's a (micro-?) optimization for another day.

Would it be okay to keep track of the largest object/pvec of each type in igc.el/Figc_info? I've got a patch here which does that, and I think the number is at least as interesting as the average :-)

> > > What this variable does is give MPS notice that the client is currently
> > > idle and it might be a good time to do some work.
> > 
> > Without understanding what effect setting this variable has on the Emacs
> > responsiveness, it does not seem very useful. (Exactly because MPS is
> > concurrent)
> 
> That was kind of my point. We can use idle time for using this variable
> and see what effect it has, especially in interactive use. Sorry if that
> wasn't clear. I'm personally trying 0.1 (100ms) here at the moment.

I think it might very well have an effect, but being able to tune the MPS is important, too, both for debugging and to improve performance. And the documentation seems to be quite minimal.

> > > > For example, my recent measurement of building agendas displayed 30% of
> > > > the time spend in GC. (whatever this means in the context of our handling
> > > > of SIGPRF)
> > > 
> > > Exactly, whqt does it mean? And if we don't know, why is it an example
> > > for anything?
> > 
> > AFAIU, on master, SIGPROF handled while our vanilla GC is running
> > will record it. In contrast, on scratch/igc, SIGPROF will put all the
> > time when igc_busy_p () is non-nil into "GC".
> 
> Right. And I wonder if that simply is because MPS is doing stuff in its
> own thread.

That (or another thread calling MPS to make an allocation) would definitely show up as a false positive.

> > And igc_busy_p is not only non-nil when MPS is pausing Emacs to do its
> > job, but also during object allocation. So, on master, profiler "GC"
> > field records real GC pauses, while on scratch/igc "GC" field is GC
> > pauses + new object allocation.
> 
> The docs say
> 
> -- C Function: *note mps_bool_t: 129. mps_arena_busy (mps_arena_t
> arena)
> 
> Return true if an *note arena: 16. is part of the way through
> execution of an operation, false otherwise.
> 
> ‘arena’ is the arena.
> 
> Note: This function is intended to assist with debugging fatal
> errors in the *note client program: d0. It is not expected to
> be needed in normal use. If you find yourself wanting to use
> this function other than in the use case described below,
> there may be a better way to meet your requirements: please
> *note contact us: d8.
> 
> What "partly through an operation" means is anyone's guess at this
> point. Someone would have to consult the sources. The docs don't say
> what you are suggesting, from my POV.

IIRC it just checks whether the arena lock is held, whenever that might be.

> > My figure of 30% says that igc_busy_p () is for 30% of CPU time, which
> > is a significant number. But it is not very useful unless we get some
> > idea about which part of it is memory allocation and which part of it is
> > MPS pausing all Emacs threads.
> > 
> > Ideally, we should also have some way to get the allocation times on
> > master. Then, we can compare them directly.
> 
> Maybe it would be interesting what the meansurements look like on macOS,
> where the check for igc_busy are not needed in the SIGPROF handler.

How sure are we of that, by the way? My understanding is there are two ways signals can interfere with one another: SIGSEGV -> SIGPROF -> SIGSEGV (which wouldn't happen on macOS) and alloc -> SIGPROF -> SIGSEGV, which might.

Pip

Re: MPS: User GC customizations

[Gerd Möllmann]

Pip Cet <pipcet@protonmail.com> writes:

> I think we can just set flags for "called MPS" and "in a scan
> function" and look at them in the SIGPROF handler to distinguish the
> four cases?

Not sure. What if MPS calls these in its own thread? I guess that
wouldn't be so interesting for Ihor.

> My suspicion is that most problems are going to be due to large
> objects creating large segments which we have to scan completely, but
> that's a (micro-?) optimization for another day.
>
> Would it be okay to keep track of the largest object/pvec of each type
> in igc.el/Figc_info? I've got a patch here which does that, and I
> think the number is at least as interesting as the average :-)

Good idea! That first weill showing the number of objects and average
size in igc-stats.

>
>> > > What this variable does is give MPS notice that the client is currently
>> > > idle and it might be a good time to do some work.
>> > 
>> > Without understanding what effect setting this variable has on the Emacs
>> > responsiveness, it does not seem very useful. (Exactly because MPS is
>> > concurrent)
>> 
>> That was kind of my point. We can use idle time for using this variable
>> and see what effect it has, especially in interactive use. Sorry if that
>> wasn't clear. I'm personally trying 0.1 (100ms) here at the moment.
>
> I think it might very well have an effect, but being able to tune the
> MPS is important, too, both for debugging and to improve performance.
> And the documentation seems to be quite minimal.
>
>> > > > For example, my recent measurement of building agendas displayed 30% of
>> > > > the time spend in GC. (whatever this means in the context of our handling
>> > > > of SIGPRF)
>> > > 
>> > > Exactly, whqt does it mean? And if we don't know, why is it an example
>> > > for anything?
>> > 
>> > AFAIU, on master, SIGPROF handled while our vanilla GC is running
>> > will record it. In contrast, on scratch/igc, SIGPROF will put all the
>> > time when igc_busy_p () is non-nil into "GC".
>> 
>> Right. And I wonder if that simply is because MPS is doing stuff in its
>> own thread.
>
> That (or another thread calling MPS to make an allocation) would definitely show up as a false positive.
>
>> > And igc_busy_p is not only non-nil when MPS is pausing Emacs to do its
>> > job, but also during object allocation. So, on master, profiler "GC"
>> > field records real GC pauses, while on scratch/igc "GC" field is GC
>> > pauses + new object allocation.
>> 
>> The docs say
>> 
>> -- C Function: *note mps_bool_t: 129. mps_arena_busy (mps_arena_t
>> arena)
>> 
>> Return true if an *note arena: 16. is part of the way through
>> execution of an operation, false otherwise.
>> 
>> ‘arena’ is the arena.
>> 
>> Note: This function is intended to assist with debugging fatal
>> errors in the *note client program: d0. It is not expected to
>> be needed in normal use. If you find yourself wanting to use
>> this function other than in the use case described below,
>> there may be a better way to meet your requirements: please
>> *note contact us: d8.
>> 
>> What "partly through an operation" means is anyone's guess at this
>> point. Someone would have to consult the sources. The docs don't say
>> what you are suggesting, from my POV.
>
> IIRC it just checks whether the arena lock is held, whenever that
> might be.

Good to know, thanks! That's what I was suspecting.

>
>> > My figure of 30% says that igc_busy_p () is for 30% of CPU time, which
>> > is a significant number. But it is not very useful unless we get some
>> > idea about which part of it is memory allocation and which part of it is
>> > MPS pausing all Emacs threads.
>> > 
>> > Ideally, we should also have some way to get the allocation times on
>> > master. Then, we can compare them directly.
>> 
>> Maybe it would be interesting what the meansurements look like on macOS,
>> where the check for igc_busy are not needed in the SIGPROF handler.
>
> How sure are we of that, by the way? My understanding is there are two
> ways signals can interfere with one another: SIGSEGV -> SIGPROF ->
> SIGSEGV (which wouldn't happen on macOS) and alloc -> SIGPROF ->
> SIGSEGV, which might.

I'm not an expert and so on, so just talking about my understanding:

Darwin is a kind of Mach descendant, and as such it uses Mach exceptions
for hardware faults. Among those is EXC_BAD_ACCESS which one gets for
protection faults. Exceptions are handled by installing a "port"
which runs in its own thread and which the OS invokes when the exception
occurs.

Exception handling is "synchronous", which I think means the OS suspends
the thread which caused the exception, invokes the port for doing what
it wants, and when done let's the original thread continue, if the port
wants that.

Signals on Darwin are something different and used for non-hardware
cases.

Re: MPS: User GC customizations

[Ihor Radchenko]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Pip Cet <pipcet@protonmail.com> writes:
>
>> I think we can just set flags for "called MPS" and "in a scan
>> function" and look at them in the SIGPROF handler to distinguish the
>> four cases?
>
> Not sure. What if MPS calls these in its own thread? I guess that
> wouldn't be so interesting for Ihor.

What about `alloc_impl'? It should only be called by Emacs threads.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: MPS: User GC customizations

[Gerd Möllmann]

Ihor Radchenko <yantar92@posteo.net> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Pip Cet <pipcet@protonmail.com> writes:
>>
>>> I think we can just set flags for "called MPS" and "in a scan
>>> function" and look at them in the SIGPROF handler to distinguish the
>>> four cases?
>>
>> Not sure. What if MPS calls these in its own thread? I guess that
>> wouldn't be so interesting for Ihor.
>
> What about `alloc_impl'? It should only be called by Emacs threads.

Yes, maybe.

Re: MPS: User GC customizations

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Ihor Radchenko <yantar92@posteo.net>,  Eli Zaretskii <eliz@gnu.org>,
>   emacs-devel@gnu.org,  eller.helmut@gmail.com
> Date: Thu, 04 Jul 2024 18:07:42 +0200
> 
> Pip Cet <pipcet@protonmail.com> writes:
> 
> > I think we can just set flags for "called MPS" and "in a scan
> > function" and look at them in the SIGPROF handler to distinguish the
> > four cases?
> 
> Not sure. What if MPS calls these in its own thread? I guess that
> wouldn't be so interesting for Ihor.

AFAIU, on GNU/Linux signals are delivered to the main thread,

> Darwin is a kind of Mach descendant, and as such it uses Mach exceptions
> for hardware faults. Among those is EXC_BAD_ACCESS which one gets for
> protection faults. Exceptions are handled by installing a "port"
> which runs in its own thread and which the OS invokes when the exception
> occurs.
> 
> Exception handling is "synchronous", which I think means the OS suspends
> the thread which caused the exception, invokes the port for doing what
> it wants, and when done let's the original thread continue, if the port
> wants that.

That's very similar to what happens on Windows, at least on this high
level of description.

Re: MPS: User GC customizations

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: Ihor Radchenko <yantar92@posteo.net>,  Eli Zaretskii <eliz@gnu.org>,
>>   emacs-devel@gnu.org,  eller.helmut@gmail.com
>> Date: Thu, 04 Jul 2024 18:07:42 +0200
>> 
>> Pip Cet <pipcet@protonmail.com> writes:
>> 
>> > I think we can just set flags for "called MPS" and "in a scan
>> > function" and look at them in the SIGPROF handler to distinguish the
>> > four cases?
>> 
>> Not sure. What if MPS calls these in its own thread? I guess that
>> wouldn't be so interesting for Ihor.
>
> AFAIU, on GNU/Linux signals are delivered to the main thread,

Pip mentioned scan functions, so let's say we set a flag in_scan while
being in dflt_scan. MPS now calls dflt_scan, in the MPS thread, to do an
increment of its work. While in dflt_scan we get SIGPROF and land in the
signal handler in main thread, and the profiler sees in_scan == true.

Q: do we count that as part of GC work that the profiler should report,
although it happened in the MPS thread? I understood Ihor as saying that
he doesn't want that.

Re: MPS: User GC customizations

[Ihor Radchenko]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Pip mentioned scan functions, so let's say we set a flag in_scan while
> being in dflt_scan. MPS now calls dflt_scan, in the MPS thread, to do an
> increment of its work. While in dflt_scan we get SIGPROF and land in the
> signal handler in main thread, and the profiler sees in_scan == true.
>
> Q: do we count that as part of GC work that the profiler should report,
> although it happened in the MPS thread? I understood Ihor as saying that
> he doesn't want that.

I think that we should ideally have multiple flags like this:
1. when arena is locked
2. when our dflt_scan code is running
3. when we query MPS synchronously (e.g. memory allocation)
4. when Emacs thread is paused by MPS

Then, we can report statistics for each flag.

I think that it can give us some idea about how different aspects of MPS
affect Emacs responsiveness.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: MPS: User GC customizations

[Pip Cet]

On Thursday, July 4th, 2024 at 18:26, Ihor Radchenko <yantar92@posteo.net> wrote:
> Gerd Möllmann gerd.moellmann@gmail.com writes:
> 
> > Pip mentioned scan functions, so let's say we set a flag in_scan while
> > being in dflt_scan. MPS now calls dflt_scan, in the MPS thread, to do an
> > increment of its work. While in dflt_scan we get SIGPROF and land in the
> > signal handler in main thread, and the profiler sees in_scan == true.
> > 
> > Q: do we count that as part of GC work that the profiler should report,
> > although it happened in the MPS thread? I understood Ihor as saying that
> > he doesn't want that.
> 
> 
> I think that we should ideally have multiple flags like this:
> 1. when arena is locked
> 2. when our dflt_scan code is running
> 3. when we query MPS synchronously (e.g. memory allocation)
> 4. when Emacs thread is paused by MPS

5. when our dflt_scan code is running on the main thread

I think that's what Gerd meant to suggest, anyway?

> Then, we can report statistics for each flag.

Or each of the 32 combinations (okay, okay, 24).

> I think that it can give us some idea about how different aspects of MPS
> affect Emacs responsiveness.

I think it would help me understand what in the word MPS might be doing. AFAICT it Just Works.

Also, may I suggest we really need

6. whether Emacs is "idle"

Pip

Re: MPS: User GC customizations

[Gerd Möllmann]

Pip Cet <pipcet@protonmail.com> writes:

> On Thursday, July 4th, 2024 at 18:26, Ihor Radchenko <yantar92@posteo.net> wrote:
>> Gerd Möllmann gerd.moellmann@gmail.com writes:
>> 
>> > Pip mentioned scan functions, so let's say we set a flag in_scan while
>> > being in dflt_scan. MPS now calls dflt_scan, in the MPS thread, to do an
>> > increment of its work. While in dflt_scan we get SIGPROF and land in the
>> > signal handler in main thread, and the profiler sees in_scan == true.
>> > 
>> > Q: do we count that as part of GC work that the profiler should report,
>> > although it happened in the MPS thread? I understood Ihor as saying that
>> > he doesn't want that.
>> 
>> 
>> I think that we should ideally have multiple flags like this:
>> 1. when arena is locked
>> 2. when our dflt_scan code is running
>> 3. when we query MPS synchronously (e.g. memory allocation)
>> 4. when Emacs thread is paused by MPS
>
> 5. when our dflt_scan code is running on the main thread
>
> I think that's what Gerd meant to suggest, anyway?

I don't know. I only wanted to contribute something because I appeared
to be confused. Or maybe were confusing me. Whatever. I'm not thinking
about profiling otherwise.

Re: MPS: User GC customizations

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: pipcet@protonmail.com,  yantar92@posteo.net,  emacs-devel@gnu.org,
>   eller.helmut@gmail.com
> Date: Thu, 04 Jul 2024 20:18:31 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> >> Cc: Ihor Radchenko <yantar92@posteo.net>,  Eli Zaretskii <eliz@gnu.org>,
> >>   emacs-devel@gnu.org,  eller.helmut@gmail.com
> >> Date: Thu, 04 Jul 2024 18:07:42 +0200
> >> 
> >> Pip Cet <pipcet@protonmail.com> writes:
> >> 
> >> > I think we can just set flags for "called MPS" and "in a scan
> >> > function" and look at them in the SIGPROF handler to distinguish the
> >> > four cases?
> >> 
> >> Not sure. What if MPS calls these in its own thread? I guess that
> >> wouldn't be so interesting for Ihor.
> >
> > AFAIU, on GNU/Linux signals are delivered to the main thread,
> 
> Pip mentioned scan functions, so let's say we set a flag in_scan while
> being in dflt_scan. MPS now calls dflt_scan, in the MPS thread, to do an
> increment of its work. While in dflt_scan we get SIGPROF and land in the
> signal handler in main thread, and the profiler sees in_scan == true.
> 
> Q: do we count that as part of GC work that the profiler should report,
> although it happened in the MPS thread? I understood Ihor as saying that
> he doesn't want that.

We probably don't want that, no.  So in_scan should not be a boolean,
it should probably hold the thread ID of the thread which set the
flag.

Anyway, we are putting the wagon before the horse.  These subtle
issues of how to account for GC in a profile are very far from the
fundamental problems we have to solve at this stage.  We didn't yet
agree on the final design of handling SIGPROF (and other useful
signals), so talking about these aspects is simply a waste of time.
Ihor, please record the issue and raise it much later, when we have
the basics figured out.  It is not useful to raise this now.

Re: MPS: User GC customizations

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

> Ihor, please record the issue and raise it much later, when we have
> the basics figured out.  It is not useful to raise this now.

No problem. I only started replying because `igc-step-interval' raised
the topic of benchmarking and performance.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: MPS: User GC customizations

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Pip Cet <pipcet@protonmail.com>,  Eli Zaretskii <eliz@gnu.org>,
>   emacs-devel@gnu.org,  eller.helmut@gmail.com
> Date: Thu, 04 Jul 2024 14:51:16 +0200
> 
> >>   igc-step-interval is a variable defined in ‘C source code’.
> >>
> >>   Its value is 0.1
> >>
> >>   How much time is MPS allowed to spend in GC when Emacs is idle.
> >>   The value is in seconds, and should be a non-negative number.  It can
> >>   be either an integer or a float.  The default value is 0 which means .
> >>   don’t do something when idle.  Negative values and values that are not numbers
> >>   are handled as if they were the default value.
> >>
> >> It's for interactive use.
> > [...]
> What this variable does is give MPS notice that the client is currently
> idle and it might be a good time to do some work.

Is that really what this variable does?  My reading of the
documentation is that it tells MPS how soon to stop GC which it
started when Emacs was idle.  IOW, it's not about _triggering_ GC,
it's about _ending_ it.  And the reason, AFAIU, is to avoid making
Emacs responses slow because MPS started GC when it detected that
Emacs is idle.

If my interpretation is wrong, then the doc string definitely needs to
be amended, but I think what you say is also in contradiction with
what MPS documentation itself says:

     ‘interval’ is the time, in seconds, the MPS is permitted to take.
     It must not be negative, but may be ‘0.0’.
[...]
     mps_arena_step() allows the client program to make use
     of idle time to do some garbage collection, for example when it is
     waiting for interactive input.  The MPS makes every effort to
     return from this function within ‘interval’ seconds, but cannot
     guarantee to do so, as it may need to call your own scanning code.
     It uses ‘multiplier’ to decide whether to commence long-duration
     operations that consume CPU (such as a full collection): it will
     only start such an operation if it is expected to be completed
     within ‘multiplier * interval’ seconds.

Did I misunderstand what this step means?

Re: MPS: User GC customizations

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: Pip Cet <pipcet@protonmail.com>,  Eli Zaretskii <eliz@gnu.org>,
>>   emacs-devel@gnu.org,  eller.helmut@gmail.com
>> Date: Thu, 04 Jul 2024 14:51:16 +0200
>> 
>> >>   igc-step-interval is a variable defined in ‘C source code’.
>> >>
>> >>   Its value is 0.1
>> >>
>> >>   How much time is MPS allowed to spend in GC when Emacs is idle.
>> >>   The value is in seconds, and should be a non-negative number.  It can
>> >>   be either an integer or a float.  The default value is 0 which means .
>> >>   don’t do something when idle.  Negative values and values that are not numbers
>> >>   are handled as if they were the default value.
>> >>
>> >> It's for interactive use.
>> > [...]
>> What this variable does is give MPS notice that the client is currently
>> idle and it might be a good time to do some work.
>
> Is that really what this variable does?  My reading of the
> documentation is that it tells MPS how soon to stop GC which it
> started when Emacs was idle.  IOW, it's not about _triggering_ GC,
> it's about _ending_ it.  And the reason, AFAIU, is to avoid making
> Emacs responses slow because MPS started GC when it detected that
> Emacs is idle.

I'm taking my view from this;:

  2.4.6 Using idle time for collection
  ------------------------------------

  Some types of program have “idle time” in which they are waiting for an
  external event such as user input or network activity.  The MPS provides
  a function, *note mps_arena_step(): 19c, for making use of idle time to
  make memory management progress.
  ...
  
The name interval I've taken from 

 -- C Function: *note mps_bool_t: 129. mps_arena_step (mps_arena_t
          arena, double interval, double multiplier)

     ‘interval’ is the time, in seconds, the MPS is permitted to take.
     It must not be negative, but may be ‘0.0’.

Alas, it doesn't promise to always obey interval:

     The MPS makes every effort to
     return from this function within ‘interval’ seconds, but cannot
     guarantee to do so, as it may need to call your own scanning code.
     It uses ‘multiplier’ to decide whether to commence long-duration
     operations that consume CPU (such as a full collection): it will
     only start such an operation if it is expected to be completed
     within ‘multiplier * interval’ seconds.

(Haven't found out yet to prevent full collections with a message "client
predicts plenty of idle time" or some such yet. Please see the comment
in igc.c where mps_arena_step is called.)

Re: MPS: User GC customizations

[Eli Zaretskii]

Re: MPS: User GC customizations

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: yantar92@posteo.net,  pipcet@protonmail.com,  emacs-devel@gnu.org,
>   eller.helmut@gmail.com
> Date: Thu, 04 Jul 2024 16:30:06 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> What this variable does is give MPS notice that the client is currently
> >> idle and it might be a good time to do some work.
> >
> > Is that really what this variable does?  My reading of the
> > documentation is that it tells MPS how soon to stop GC which it
> > started when Emacs was idle.  IOW, it's not about _triggering_ GC,
> > it's about _ending_ it.  And the reason, AFAIU, is to avoid making
> > Emacs responses slow because MPS started GC when it detected that
> > Emacs is idle.
> 
> I'm taking my view from this;:

I'm not sure what you are saying.  You've cited exactly the same text
as I did, but didn't answer ny real question: does this variable work
as you said above ("give MPS notice that the client is currently
idle") or like I interpret it: tell MPS how much time it's okay to
spend in GC which it started when Emacs was idle?

> (Haven't found out yet to prevent full collections with a message "client
> predicts plenty of idle time" or some such yet. Please see the comment
> in igc.c where mps_arena_step is called.)

Sure.  But how is this relevant to the issue at hand, may I ask?

Re: MPS: User GC customizations

[Pip Cet]

On Thursday, July 4th, 2024 at 15:48, Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Gerd Möllmann gerd.moellmann@gmail.com
> 
> > Cc: yantar92@posteo.net, pipcet@protonmail.com, emacs-devel@gnu.org,
> > eller.helmut@gmail.com
> > Date: Thu, 04 Jul 2024 16:30:06 +0200
> > 
> > Eli Zaretskii eliz@gnu.org writes:
> > 
> > > > What this variable does is give MPS notice that the client is currently
> > > > idle and it might be a good time to do some work.
> > > 
> > > Is that really what this variable does? My reading of the
> > > documentation is that it tells MPS how soon to stop GC which it
> > > started when Emacs was idle. IOW, it's not about triggering GC,
> > > it's about ending it. And the reason, AFAIU, is to avoid making
> > > Emacs responses slow because MPS started GC when it detected that
> > > Emacs is idle.
> > 
> > I'm taking my view from this;:
> 
> 
> I'm not sure what you are saying. You've cited exactly the same text
> as I did, but didn't answer ny real question: does this variable work
> as you said above ("give MPS notice that the client is currently
> idle") or like I interpret it: tell MPS how much time it's okay to
> spend in GC which it started when Emacs was idle?

I'm really confused. Can you explain what you think the difference is? The variable's value is a number. If the variable is zero, we don't tell MPS we're idle when we're idle. If it's nonzero, we tell it we're idle (when we are), and how much time it's okay to spend in GC.

Pip

Re: MPS: User GC customizations

[Eli Zaretskii]

> Date: Thu, 04 Jul 2024 15:52:35 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: Gerd Möllmann <gerd.moellmann@gmail.com>, yantar92@posteo.net, emacs-devel@gnu.org, eller.helmut@gmail.com
> 
> On Thursday, July 4th, 2024 at 15:48, Eli Zaretskii <eliz@gnu.org> wrote:
> > > From: Gerd Möllmann gerd.moellmann@gmail.com
> > 
> > > Cc: yantar92@posteo.net, pipcet@protonmail.com, emacs-devel@gnu.org,
> > > eller.helmut@gmail.com
> > > Date: Thu, 04 Jul 2024 16:30:06 +0200
> > > 
> > > Eli Zaretskii eliz@gnu.org writes:
> > > 
> > > > > What this variable does is give MPS notice that the client is currently
> > > > > idle and it might be a good time to do some work.
> > > > 
> > > > Is that really what this variable does? My reading of the
> > > > documentation is that it tells MPS how soon to stop GC which it
> > > > started when Emacs was idle. IOW, it's not about triggering GC,
> > > > it's about ending it. And the reason, AFAIU, is to avoid making
> > > > Emacs responses slow because MPS started GC when it detected that
> > > > Emacs is idle.
> > > 
> > > I'm taking my view from this;:
> > 
> > 
> > I'm not sure what you are saying. You've cited exactly the same text
> > as I did, but didn't answer ny real question: does this variable work
> > as you said above ("give MPS notice that the client is currently
> > idle") or like I interpret it: tell MPS how much time it's okay to
> > spend in GC which it started when Emacs was idle?
> 
> I'm really confused. Can you explain what you think the difference is?

I just did, twice.  If that still doesn't explain it, then I guess my
explanatory talents betray me, and I don't know what else to say.

> The variable's value is a number. If the variable is zero, we don't tell MPS we're idle when we're idle. If it's nonzero, we tell it we're idle (when we are), and how much time it's okay to spend in GC.

The number can only tell one thing, not two.  AFAIU, it tells the
latter: "how much time it's okay to spend in GC" once GC started due
to idleness (and not some other reason).  Gerd said something
different: that this number "gives MPS notice" that Emacs is idle.
(How can a number "give notice" I don't understand even in principle.)

Re: MPS: User GC customizations

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Pip Cet <pipcet@protonmail.com>
>> I'm really confused. Can you explain what you think the difference
>> is?

FWIW, I'm also confused.

> I just did, twice.  If that still doesn't explain it, then I guess my
> explanatory talents betray me, and I don't know what else to say.
>
>> The variable's value is a number. If the variable is zero, we don't
>> tell MPS we're idle when we're idle. If it's nonzero, we tell it
>> we're idle (when we are), and how much time it's okay to spend in GC.
>
> The number can only tell one thing, not two.  AFAIU, it tells the
> latter: "how much time it's okay to spend in GC" once GC started due
> to idleness (and not some other reason).  Gerd said something
> different: that this number "gives MPS notice" that Emacs is idle.
> (How can a number "give notice" I don't understand even in principle.)

By saying that 0 has a special meaning.

The "once GC started due to idleness" confuses me. We don't "start" GC
due to idleness. We allow MPS to do something concurrently, say one or
more increments in the sense of its incremental GC algorithm. And "GC
runs" all the time.

Re: MPS: User GC customizations

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Pip Cet <pipcet@protonmail.com>,  yantar92@posteo.net,
>   emacs-devel@gnu.org,  eller.helmut@gmail.com
> Date: Thu, 04 Jul 2024 19:01:35 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Pip Cet <pipcet@protonmail.com>
> >> I'm really confused. Can you explain what you think the difference
> >> is?
> 
> FWIW, I'm also confused.
> 
> > I just did, twice.  If that still doesn't explain it, then I guess my
> > explanatory talents betray me, and I don't know what else to say.
> >
> >> The variable's value is a number. If the variable is zero, we don't
> >> tell MPS we're idle when we're idle. If it's nonzero, we tell it
> >> we're idle (when we are), and how much time it's okay to spend in GC.
> >
> > The number can only tell one thing, not two.  AFAIU, it tells the
> > latter: "how much time it's okay to spend in GC" once GC started due
> > to idleness (and not some other reason).  Gerd said something
> > different: that this number "gives MPS notice" that Emacs is idle.
> > (How can a number "give notice" I don't understand even in principle.)
> 
> By saying that 0 has a special meaning.
> 
> The "once GC started due to idleness" confuses me. We don't "start" GC
> due to idleness.

I didn't say "we" start GC due to idleness, I said that GC starts due
to idleness.  Not necessarily by ourself.

> We allow MPS to do something concurrently, say one or more
> increments in the sense of its incremental GC algorithm. And "GC
> runs" all the time.

You are saying that the whether Emacs is or isn't idle is not relevant
at all to the effect of this variable?  Then why does MPS
documentation talk about "allowing the client program to make use of
idle time to do some garbage collection"?  And why does the doc string
of igc-step-interval itself mention "GC when Emacs is idle"?

IOW, let me turn the table and ask what does "idleness" have to do
with this variable?

Re: MPS: User GC customizations

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> The "once GC started due to idleness" confuses me. We don't "start" GC
>> due to idleness.
>
> I didn't say "we" start GC due to idleness, I said that GC starts due
> to idleness.  Not necessarily by ourself.

I'm sorry, I find that totally confusing. Is what you mean simply "when
we call mps_arena_step? And why "start GC"?

>> We allow MPS to do something concurrently, say one or more
>> increments in the sense of its incremental GC algorithm. And "GC
>> runs" all the time.
>
> You are saying that the whether Emacs is or isn't idle is not relevant
> at all to the effect of this variable?  

Wot? Please someone help.

> Then why does MPS documentation talk about "allowing the client
> program to make use of idle time to do some garbage collection"? And
> why does the doc string of igc-step-interval itself mention "GC when
> Emacs is idle"?

I wrote that because I put mps_arena_step where it is, in igc_on_idle,
which is called when Emacs is idle.

> IOW, let me turn the table and ask what does "idleness" have to do
> with this variable?

Simple. Because this person here, a long time ago, idly put
mps_arena_step where it is, when writing igc_on_idle, and made Emacs call
that when idle.

Re: MPS: User GC customizations

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: pipcet@protonmail.com,  yantar92@posteo.net,  emacs-devel@gnu.org,
>   eller.helmut@gmail.com
> Date: Thu, 04 Jul 2024 20:28:23 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > IOW, let me turn the table and ask what does "idleness" have to do
> > with this variable?
> 
> Simple. Because this person here, a long time ago, idly put
> mps_arena_step where it is, when writing igc_on_idle, and made Emacs call
> that when idle.

Then it follows that "we" cause the to begin when Emacs is idle, and
"we" tell MPS that it is kindly requested not to spend more than
igc-step-interval seconds doing GC before it returns, when we trigger
GC via mps_arena_step.  Right?

Re: MPS: User GC customizations

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: pipcet@protonmail.com,  yantar92@posteo.net,  emacs-devel@gnu.org,
>>   eller.helmut@gmail.com
>> Date: Thu, 04 Jul 2024 20:28:23 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > IOW, let me turn the table and ask what does "idleness" have to do
>> > with this variable?
>> 
>> Simple. Because this person here, a long time ago, idly put
>> mps_arena_step where it is, when writing igc_on_idle, and made Emacs call
>> that when idle.
>
> Then it follows that "we" cause the to begin when Emacs is idle, and
> "we" tell MPS that it is kindly requested not to spend more than
> igc-step-interval seconds doing GC before it returns, when we trigger
> GC via mps_arena_step.  Right?

We give MPS the chance to to work it wants to, right. 

Re: MPS: User GC customizations

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: pipcet@protonmail.com,  yantar92@posteo.net,  emacs-devel@gnu.org,
>   eller.helmut@gmail.com
> Date: Thu, 04 Jul 2024 21:09:23 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> >> Cc: pipcet@protonmail.com,  yantar92@posteo.net,  emacs-devel@gnu.org,
> >>   eller.helmut@gmail.com
> >> Date: Thu, 04 Jul 2024 20:28:23 +0200
> >> 
> >> Eli Zaretskii <eliz@gnu.org> writes:
> >> 
> >> > IOW, let me turn the table and ask what does "idleness" have to do
> >> > with this variable?
> >> 
> >> Simple. Because this person here, a long time ago, idly put
> >> mps_arena_step where it is, when writing igc_on_idle, and made Emacs call
> >> that when idle.
> >
> > Then it follows that "we" cause the to begin when Emacs is idle, and
> > "we" tell MPS that it is kindly requested not to spend more than
> > igc-step-interval seconds doing GC before it returns, when we trigger
> > GC via mps_arena_step.  Right?
> 
> We give MPS the chance to to work it wants to, right. 

OK, in that case all the preceding discussion and confusion was due to
misunderstanding, because that's what I meant from the beginning.

Re: MPS: User GC customizations

[Pip Cet]

On Thursday, July 4th, 2024 at 13:58, Eli Zaretskii <eliz@gnu.org> wrote:
> ‘interval’ is the time, in seconds, the MPS is permitted to take.
> It must not be negative, but may be ‘0.0’.
> [...]
> mps_arena_step() allows the client program to make use
> of idle time to do some garbage collection, for example when it is
> waiting for interactive input. The MPS makes every effort to
> return from this function within ‘interval’ seconds, but cannot
> guarantee to do so, as it may need to call your own scanning code.
> It uses ‘multiplier’ to decide whether to commence long-duration
> operations that consume CPU (such as a full collection): it will
> only start such an operation if it is expected to be completed
> within ‘multiplier * interval’ seconds.

Why are we passing 0 for "multiplier", then? Should that be 1?

Pip

Re: MPS: User GC customizations

[Gerd Möllmann]

Pip Cet <pipcet@protonmail.com> writes:

> On Thursday, July 4th, 2024 at 13:58, Eli Zaretskii <eliz@gnu.org> wrote:
>> ‘interval’ is the time, in seconds, the MPS is permitted to take.
>> It must not be negative, but may be ‘0.0’.
>> [...]
>> mps_arena_step() allows the client program to make use
>> of idle time to do some garbage collection, for example when it is
>> waiting for interactive input. The MPS makes every effort to
>> return from this function within ‘interval’ seconds, but cannot
>> guarantee to do so, as it may need to call your own scanning code.
>> It uses ‘multiplier’ to decide whether to commence long-duration
>> operations that consume CPU (such as a full collection): it will
>> only start such an operation if it is expected to be completed
>> within ‘multiplier * interval’ seconds.
>
> Why are we passing 0 for "multiplier", then? Should that be 1?

It's just my interpretation of 

     ‘multiplier’ is the number of further similar calls that the client
     program expects to make during this idle period.

IOW, I don't expect that I'll call mps_arena_step again in this period.
But actually, I don't know, so I thought 0 was appropriate.