MPS image cache

MPS image cache

[Gerd Möllmann]

I think I have one: struct terminal's image_cache is not traced. AFAICS,
the cache is malloc'd and contains struct image which is an MPS object.

WDYT?

Re: MPS image cache

[Helmut Eller]

On Fri, May 03 2024, Gerd Möllmann wrote:

> I think I have one: struct terminal's image_cache is not traced. AFAICS,
> the cache is malloc'd and contains struct image which is an MPS object.
>
> WDYT?

Two options come to mind:
  1) trace it in fix_terminal
  2) register terminal.c:terminal_list as root.

It seems to me that creating a root is more "correct".

I have a similar problem with xterm.c:x_display_list which could be
registered as root or traced e.g. fix_frame.  (Also note that struct
struct terminal contains a x_display_info, which is probably the same as
the in x_display_list.)

So maybe we should indeed register terminal_list and as part of the
scan function, also trace the display_info.

There is a bit of an issue with proper initialization of the structs
pointed to by those roots.  E.g. x_term_init uses xzalloc, puts pointers
into the struct, and only then stores the pointer to the struct in
x_display_list.  So what should be the root, the struct (registred
immediately after the call to xzalloc) or the global variable
x_display_list?

Re: MPS image cache

[Po Lu]

Helmut Eller <eller.helmut@gmail.com> writes:

> On Fri, May 03 2024, Gerd Möllmann wrote:
>
>> I think I have one: struct terminal's image_cache is not traced. AFAICS,
>> the cache is malloc'd and contains struct image which is an MPS object.
>>
>> WDYT?
>
> Two options come to mind:
>   1) trace it in fix_terminal
>   2) register terminal.c:terminal_list as root.
>
> It seems to me that creating a root is more "correct".
>
> I have a similar problem with xterm.c:x_display_list which could be
> registered as root or traced e.g. fix_frame.

It is valid for an open display not to have any frames under its name,
so they must be registered as roots.

Re: MPS image cache

[Gerd Möllmann]

Helmut Eller <eller.helmut@gmail.com> writes:

> On Fri, May 03 2024, Gerd Möllmann wrote:
>
>> I think I have one: struct terminal's image_cache is not traced. AFAICS,
>> the cache is malloc'd and contains struct image which is an MPS object.
>>
>> WDYT?
>
> Two options come to mind:
>   1) trace it in fix_terminal
>   2) register terminal.c:terminal_list as root.
>
> It seems to me that creating a root is more "correct".

I'm meanhile tending ta the third option: make iamge_cache an MPS object
like face_cache. For a reason that's proobably the reason why I made
fache_cache an MPS object:

While we scan an object, MPS gives us exclusive access to it. I think we
want that for these caches, so that scanning does not interfere iwth the
client modifying the cache?

Right?

> I have a similar problem with xterm.c:x_display_list which could be
> registered as root or traced e.g. fix_frame.  (Also note that struct
> struct terminal contains a x_display_info, which is probably the same as
> the in x_display_list.)
>
> So maybe we should indeed register terminal_list and as part of the
> scan function, also trace the display_info.
>
> There is a bit of an issue with proper initialization of the structs
> pointed to by those roots.  E.g. x_term_init uses xzalloc, puts pointers
> into the struct, and only then stores the pointer to the struct in
> x_display_list.  So what should be the root, the struct (registred
> immediately after the call to xzalloc) or the global variable
> x_display_list?

Hm.

Re: MPS image cache

[Gerd Möllmann]

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

> Helmut Eller <eller.helmut@gmail.com> writes:
>
>> On Fri, May 03 2024, Gerd Möllmann wrote:
>>
>>> I think I have one: struct terminal's image_cache is not traced. AFAICS,
>>> the cache is malloc'd and contains struct image which is an MPS object.
>>>
>>> WDYT?
>>
>> Two options come to mind:
>>   1) trace it in fix_terminal
>>   2) register terminal.c:terminal_list as root.
>>
>> It seems to me that creating a root is more "correct".
>
> I'm meanhile tending ta the third option: make iamge_cache an MPS object
> like face_cache. For a reason that's proobably the reason why I made
> fache_cache an MPS object:
>
> While we scan an object, MPS gives us exclusive access to it. I think we
> want that for these caches, so that scanning does not interfere iwth the
> client modifying the cache?
>
> Right?

I've doen that now. It is right :-)

Re: MPS image cache

[Gerd Möllmann]

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

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Helmut Eller <eller.helmut@gmail.com> writes:
>>
>>> On Fri, May 03 2024, Gerd Möllmann wrote:
>>>
>>>> I think I have one: struct terminal's image_cache is not traced. AFAICS,
>>>> the cache is malloc'd and contains struct image which is an MPS object.
>>>>
>>>> WDYT?
>>>
>>> Two options come to mind:
>>>   1) trace it in fix_terminal
>>>   2) register terminal.c:terminal_list as root.
>>>
>>> It seems to me that creating a root is more "correct".
>>
>> I'm meanhile tending ta the third option: make iamge_cache an MPS object
>> like face_cache. For a reason that's proobably the reason why I made
>> fache_cache an MPS object:
>>
>> While we scan an object, MPS gives us exclusive access to it. I think we
>> want that for these caches, so that scanning does not interfere iwth the
>> client modifying the cache?
>>
>> Right?
>
> I've doen that now. It is right :-)

The next one I get is in nsterm.m

      NSWindow *parentWindow = [FRAME_NS_VIEW (parentFrame) window];

where parentFrame is a struct frame *. That can take some time...

Re: MPS image cache

[Gerd Möllmann]

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

> The next one I get is in nsterm.m
>
>       NSWindow *parentWindow = [FRAME_NS_VIEW (parentFrame) window];
>
> where parentFrame is a struct frame *. That can take some time...

So, did someting for NS in fix_frame, and this Emacs has survived for 10
minutes already...

MPS: scroll-bars (was: MPS image cache)

[Helmut Eller]

The function xterm.c:xg_finish_scroll_bar_creation hands a pointer to a
struct scroll_bar to GTK.  GTK simply passes this along to the
callback.  How could we trace this or how could we pin scroll bars?

Re: MPS: scroll-bars

[Gerd Möllmann]

Helmut Eller <eller.helmut@gmail.com> writes:

> The function xterm.c:xg_finish_scroll_bar_creation hands a pointer to a
> struct scroll_bar to GTK.  GTK simply passes this along to the
> callback.  How could we trace this or how could we pin scroll bars?

Moin Helmut.

With only having had one coffee, could we use an additional indirection
somehow? On the basis that every problem can be solved with an additional
indirection? 

Re: MPS: scroll-bars

[Gerd Möllmann]

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

> Helmut Eller <eller.helmut@gmail.com> writes:
>
>> The function xterm.c:xg_finish_scroll_bar_creation hands a pointer to a
>> struct scroll_bar to GTK.  GTK simply passes this along to the
>> callback.  How could we trace this or how could we pin scroll bars?
>
> Moin Helmut.
>
> With only having had one coffee, could we use an additional indirection
> somehow? On the basis that every problem can be solved with an additional
> indirection?

That was too terse, sorry. I'm thinking of passing a scroll_bar **
through GTK to the callback. That would make it perhaps possible to put
the original scroll_bar * where it is better traceable, if that address
itself doesn't change... 

Re: MPS: scroll-bars

[Po Lu]

Helmut Eller <eller.helmut@gmail.com> writes:

> The function xterm.c:xg_finish_scroll_bar_creation hands a pointer to a
> struct scroll_bar to GTK.  GTK simply passes this along to the
> callback.  How could we trace this or how could we pin scroll bars?

Connect a callback that unpins the object's scroll bars to the "destroy"
signal of the object, as xg_gtk_scroll_destroy is immediately above.

Re: MPS: scroll-bars

[Helmut Eller]

On Sat, May 04 2024, Po Lu wrote:

> Helmut Eller <eller.helmut@gmail.com> writes:
>
>> The function xterm.c:xg_finish_scroll_bar_creation hands a pointer to a
>> struct scroll_bar to GTK.  GTK simply passes this along to the
>> callback.  How could we trace this or how could we pin scroll bars?
>
> Connect a callback that unpins the object's scroll bars to the "destroy"
> signal of the object, as xg_gtk_scroll_destroy is immediately above.

xg_gtk_scroll_destroy uses id_to_widget. That's a global variable.  I'd
rather avoid that.

I could allocate a single cell as ambiguous root to the scroll_bar.  That
would pin it.  Then pass that cell to a "destroy" callback to free the
cell.  Hmm.. a lot of work.

Could we perhaps use the mark-and-sweep pool for the widget-like
objects, like scroll_bar and window?  That would perhaps be easier,
because then widgets wouldn't move and tracing would be very similar to
what the old GC does.  And the widgets are hardly performance critical.

Re: MPS: scroll-bars

[Gerd Möllmann]

Helmut Eller <eller.helmut@gmail.com> writes:

> Could we perhaps use the mark-and-sweep pool for the widget-like
> objects, like scroll_bar and window?  That would perhaps be easier,
> because then widgets wouldn't move and tracing would be very similar to
> what the old GC does.  And the widgets are hardly performance critical.

We could try, certainly. They say

  AMS is not currently suitable for production use. However, it could be
  developed into a solid mark-and-sweep pool. If you have a use case
  that needs this, contact us.

so we are a bit on our own, I guess.

Would we then alloc struct scroll_bar in AMS? Or is it something else?

(BTW, this one got me today. I couldn't find a PVEC_SCROLL_BAR and then
this:

  #define XSCROLL_BAR(vec) ((struct scroll_bar *) XVECTOR (vec))

Nice, nice :-)

Re: MPS: scroll-bars

[Helmut Eller]

> Would we then alloc struct scroll_bar in AMS? Or is it something else?

Yes, that was the idea.  I hope that we can have references from blocks
in the AMC to blocks in AMS.

>
> (BTW, this one got me today. I couldn't find a PVEC_SCROLL_BAR and then
> this:
>
>   #define XSCROLL_BAR(vec) ((struct scroll_bar *) XVECTOR (vec))
>
> Nice, nice :-)

x_scroll_bar_create uses PVEC_OTHER.

Re: MPS: scroll-bars

[Gerd Möllmann]

Helmut Eller <eller.helmut@gmail.com> writes:

>> Would we then alloc struct scroll_bar in AMS? Or is it something else?
>
> Yes, that was the idea.  I hope that we can have references from blocks
> in the AMC to blocks in AMS.
>
>>
>> (BTW, this one got me today. I couldn't find a PVEC_SCROLL_BAR and then
>> this:
>>
>>   #define XSCROLL_BAR(vec) ((struct scroll_bar *) XVECTOR (vec))
>>
>> Nice, nice :-)
>
> x_scroll_bar_create uses PVEC_OTHER.

Also nice :-)

Re: MPS: scroll-bars

[Gerd Möllmann]

AFAICT, I have scroll bars on NS handled now.

But I will deny any and all knowledge of what I did on NS in the future.

Re: MPS: scroll-bars

[Gerd Möllmann]

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

> Helmut Eller <eller.helmut@gmail.com> writes:
>
>> Could we perhaps use the mark-and-sweep pool for the widget-like
>> objects, like scroll_bar and window?  That would perhaps be easier,
>> because then widgets wouldn't move and tracing would be very similar to
>> what the old GC does.  And the widgets are hardly performance critical.
>
> We could try, certainly. They say
>
>   AMS is not currently suitable for production use. However, it could be
>   developed into a solid mark-and-sweep pool. If you have a use case
>   that needs this, contact us.
>
> so we are a bit on our own, I guess.
>
> Would we then alloc struct scroll_bar in AMS? Or is it something else?
>
> (BTW, this one got me today. I couldn't find a PVEC_SCROLL_BAR and then
> this:
>
>   #define XSCROLL_BAR(vec) ((struct scroll_bar *) XVECTOR (vec))
>
> Nice, nice :-)

So, maybe this is also interesting on other platforms, who knows...

struct window has members for scroll bars, e.g.

  Lisp_Object vertical_scroll_bar;

These Lisp_Objects are either fixnums or Lisp_Misc_Ptrs (see function
xmint_ptr in lisp.h). This is a C pointer, and points, on NS, to an
EmacsScroller, which is derived from some GUI widget class.
EmacsScroller then has references to window and frame.

Re: MPS: scroll-bars

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Po Lu <luangruo@yahoo.com>,  Eli Zaretskii <eliz@gnu.org>,  Emacs Devel
>  <emacs-devel@gnu.org>
> Date: Sat, 04 May 2024 09:09:00 +0200
> 
> >   #define XSCROLL_BAR(vec) ((struct scroll_bar *) XVECTOR (vec))
> >
> > Nice, nice :-)
> 
> So, maybe this is also interesting on other platforms, who knows...
> 
> struct window has members for scroll bars, e.g.
> 
>   Lisp_Object vertical_scroll_bar;
> 
> These Lisp_Objects are either fixnums or Lisp_Misc_Ptrs (see function
> xmint_ptr in lisp.h).

That's not what I see on w32.  Here's what I see:

  static void
  w32_set_vertical_scroll_bar (struct window *w,
			       int portion, int whole, int position)
  {
    Lisp_Object barobj;
    struct scroll_bar *bar;
    [...]
    /* Does the scroll bar exist yet?  */
    if (NILP (w->vertical_scroll_bar))
      {
	[...]
	bar = w32_scroll_bar_create (w, left, top, width, height, false);
      }
    [...]
    XSETVECTOR (barobj, bar);
    wset_vertical_scroll_bar (w, barobj);

And w32_scroll_bar_create does:

  static struct scroll_bar *
  w32_scroll_bar_create (struct window *w, int left, int top,
			 int width, int height, bool horizontal)
  {
    struct frame *f = XFRAME (WINDOW_FRAME (w));
    [...]
    struct scroll_bar *bar
      = ALLOCATE_PSEUDOVECTOR (struct scroll_bar, w32_widget_high, PVEC_OTHER);
    Lisp_Object barobj;

    block_input ();

    XSETWINDOW (bar->window, w);
    bar->top = top;
    bar->left = left;
    bar->width = width;
    bar->height = height;
    bar->start = 0;
    bar->end = 0;
    bar->dragging = 0;
    bar->horizontal = horizontal;

    /* Requires geometry to be set before call to create the real window */

    if (horizontal)
      hwnd = my_create_hscrollbar (f, bar);
    else
      hwnd = my_create_vscrollbar (f, bar);
    [...]
    /* Add bar to its frame's list of scroll bars.  */
    bar->next = FRAME_SCROLL_BARS (f);
    bar->prev = Qnil;
    XSETVECTOR (barobj, bar);
    fset_scroll_bars (f, barobj);
    if (! NILP (bar->next))
      XSETVECTOR (XSCROLL_BAR (bar->next)->prev, bar);

My conclusions:

  . scroll-bar Lisp object is a pseudovector
  . w->vertical_scroll_bar is set to the window's scroll-bar object
  . scroll-bar objects of a frame are stored in a doubly-linked list
    via the ->prev and ->next C pointers of 'struct scroll_bar'
  . frame's f->scroll_bars points to the last scroll-bar object
    created for the frame's windows, and thus effectively to the
    linked list of the frame's scroll bars
  . similarly, a frame has f->condemned_scroll_bars, which is a
    doubly-linked list of condemned scroll bars, the ones that could
    be deleted as result of redisplay (e.g., because their window were
    deleted)

Moreover (and this is w32-specific, I think), the Lisp thread sends a
GUI message to the UI thread with the pointer to 'struct scroll_bar',
via the call to my_create_hscrollbar:

    case WM_EMACS_CREATEVSCROLLBAR:
      return (LRESULT) w32_createvscrollbar ((struct frame *) wParam,
					     (struct scroll_bar *) lParam);

  static HWND
  my_create_vscrollbar (struct frame * f, struct scroll_bar * bar)
  {
    return (HWND) SendMessage (FRAME_W32_WINDOW (f),
			       WM_EMACS_CREATEVSCROLLBAR, (WPARAM) f,
			       (LPARAM) bar); <<<<<<<<<<<<<<<<<<<<<<<<
  }

the UI thread then uses the members of 'bar' like this:

  static HWND
  w32_createvscrollbar (struct frame *f, struct scroll_bar * bar)
  {
    HWND hwnd = CreateWindow ("SCROLLBAR", "",
			 /* Clip siblings so we don't draw over child
			    frames.  Apparently this is not always
			    sufficient so we also try to make bar windows
			    bottommost.  */
			 SBS_VERT | WS_CHILD | WS_VISIBLE | WS_CLIPSIBLINGS,
			 /* Position and size of scroll bar.  */
			 bar->left, bar->top, bar->width, bar->height,
			 FRAME_W32_WINDOW (f), NULL, hinst, NULL);

So, what does this mean in the MPS build?  I guess we need to make
sure scroll bars don't move, for the reference across threads to work?

I see that allocate_pseudovector does:

  struct Lisp_Vector *
  igc_alloc_pseudovector (size_t nwords_mem, size_t nwords_lisp,
			  size_t nwords_zero, enum pvec_type tag)
  {
    struct Lisp_Vector *v
      = alloc (header_size + nwords_mem * word_size, IGC_OBJ_VECTOR, tag);
    XSETPVECTYPESIZE (v, tag, nwords_lisp, nwords_mem - nwords_lisp);
    maybe_finalize (v, tag);
    return v;
  }

But still for the life of me I don't really understand what that means
for us in this case.  What does it mean for the references to its C
'struct scroll_bar' when the scroll-bar object is moved?  If any of
these references are on some thread's stack, does it mean the object
will not move?  And what about those prev and next pointers in frame's
scroll_bars lists -- do we need to do anything with them?  Likewise
with the window's vertical_scroll_bar pointers.

Re: MPS: scroll-bars

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> My conclusions:
>
>   . scroll-bar Lisp object is a pseudovector
>   . w->vertical_scroll_bar is set to the window's scroll-bar object
>   . scroll-bar objects of a frame are stored in a doubly-linked list
>     via the ->prev and ->next C pointers of 'struct scroll_bar'
>   . frame's f->scroll_bars points to the last scroll-bar object
>     created for the frame's windows, and thus effectively to the
>     linked list of the frame's scroll bars
>   . similarly, a frame has f->condemned_scroll_bars, which is a
>     doubly-linked list of condemned scroll bars, the ones that could
>     be deleted as result of redisplay (e.g., because their window were
>     deleted)

  /* List of scroll bars on this frame.
     Actually, we don't specify exactly what is stored here at all; the
     scroll bar implementation code can use it to store anything it likes.
     This field is marked by the garbage collector.  It is here
     instead of in the `device' structure so that the garbage
     collector doesn't need to look inside the window-system-dependent
     structure.  */
  Lisp_Object scroll_bars;
  Lisp_Object condemned_scroll_bars;

Oh shit :-(. I need a break.

> Moreover (and this is w32-specific, I think), the Lisp thread sends a
> GUI message to the UI thread with the pointer to 'struct scroll_bar',
> via the call to my_create_hscrollbar:
>
>     case WM_EMACS_CREATEVSCROLLBAR:
>       return (LRESULT) w32_createvscrollbar ((struct frame *) wParam,
> 					     (struct scroll_bar *) lParam);
>
>   static HWND
>   my_create_vscrollbar (struct frame * f, struct scroll_bar * bar)
>   {
>     return (HWND) SendMessage (FRAME_W32_WINDOW (f),
> 			       WM_EMACS_CREATEVSCROLLBAR, (WPARAM) f,
> 			       (LPARAM) bar); <<<<<<<<<<<<<<<<<<<<<<<<
>   }
>
> the UI thread then uses the members of 'bar' like this:
>
>   static HWND
>   w32_createvscrollbar (struct frame *f, struct scroll_bar * bar)
>   {
>     HWND hwnd = CreateWindow ("SCROLLBAR", "",
> 			 /* Clip siblings so we don't draw over child
> 			    frames.  Apparently this is not always
> 			    sufficient so we also try to make bar windows
> 			    bottommost.  */
> 			 SBS_VERT | WS_CHILD | WS_VISIBLE | WS_CLIPSIBLINGS,
> 			 /* Position and size of scroll bar.  */
> 			 bar->left, bar->top, bar->width, bar->height,
> 			 FRAME_W32_WINDOW (f), NULL, hinst, NULL);
>
> So, what does this mean in the MPS build?  I guess we need to make
> sure scroll bars don't move, for the reference across threads to work?

That would be my guess as well.

> I see that allocate_pseudovector does:
>
>   struct Lisp_Vector *
>   igc_alloc_pseudovector (size_t nwords_mem, size_t nwords_lisp,
> 			  size_t nwords_zero, enum pvec_type tag)
>   {
>     struct Lisp_Vector *v
>       = alloc (header_size + nwords_mem * word_size, IGC_OBJ_VECTOR, tag);
>     XSETPVECTYPESIZE (v, tag, nwords_lisp, nwords_mem - nwords_lisp);
>     maybe_finalize (v, tag);
>     return v;
>   }
>
> But still for the life of me I don't really understand what that means
> for us in this case.  What does it mean for the references to its C
> 'struct scroll_bar' when the scroll-bar object is moved?  If any of
> these references are on some thread's stack, does it mean the object
> will not move?  And what about those prev and next pointers in frame's
> scroll_bars lists -- do we need to do anything with them?  Likewise
> with the window's vertical_scroll_bar pointers.

I assume that the GUI thread you mention is not one the Emacs threads,
but something else, so it's not main_thread and not one of the threads
made with make-thread.

Then I think we first of all must make sure that MPS knows about that
thread. The existing threads are added with thread_add

  static struct igc_thread_list *
  thread_add (struct thread_state *ts)
  {
    mps_thr_t thr;
    mps_res_t res = mps_thread_reg (&thr, global_igc->arena);
    IGC_CHECK_RES (res);
    struct igc_thread_list *t = register_thread (global_igc, thr, ts);

  That's the root for the control stack of the thread:

    root_create_thread (t);

  These are allocation points in case the thread allocates from MPS:

    create_thread_aps (&t->d);
    return t;
  }

Maybe we should start with this, and move forward when we have it.

Re: MPS: scroll-bars

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  luangruo@yahoo.com,  emacs-devel@gnu.org
> Date: Sat, 04 May 2024 11:13:40 +0200
> 
> > So, what does this mean in the MPS build?  I guess we need to make
> > sure scroll bars don't move, for the reference across threads to work?
> 
> That would be my guess as well.

How to go about that?

> > I see that allocate_pseudovector does:
> >
> >   struct Lisp_Vector *
> >   igc_alloc_pseudovector (size_t nwords_mem, size_t nwords_lisp,
> > 			  size_t nwords_zero, enum pvec_type tag)
> >   {
> >     struct Lisp_Vector *v
> >       = alloc (header_size + nwords_mem * word_size, IGC_OBJ_VECTOR, tag);
> >     XSETPVECTYPESIZE (v, tag, nwords_lisp, nwords_mem - nwords_lisp);
> >     maybe_finalize (v, tag);
> >     return v;
> >   }
> >
> > But still for the life of me I don't really understand what that means
> > for us in this case.  What does it mean for the references to its C
> > 'struct scroll_bar' when the scroll-bar object is moved?  If any of
> > these references are on some thread's stack, does it mean the object
> > will not move?  And what about those prev and next pointers in frame's
> > scroll_bars lists -- do we need to do anything with them?  Likewise
> > with the window's vertical_scroll_bar pointers.
> 
> I assume that the GUI thread you mention is not one the Emacs threads,
> but something else, so it's not main_thread and not one of the threads
> made with make-thread.

It isn't a Lisp thread, that's true.  But it's an "Emacs thread" in
the sense that in the w32 build it is an integral part of Emacs, and
without it the GUI features and GUI I/O would not work on MS-Windows.

(You can find the overview of the thread structure of the w32 build in
a large comment that starts at line 3365 of w32fns.c.)

> Then I think we first of all must make sure that MPS knows about that
> thread.

Why do we need MPS to know about that thread (or any other non-Lisp
thread used by Emacs on MS-Windows; there are more)?  is that for MPS
to scan their C stacks and registers?

> The existing threads are added with thread_add
> 
>   static struct igc_thread_list *
>   thread_add (struct thread_state *ts)
>   {
>     mps_thr_t thr;
>     mps_res_t res = mps_thread_reg (&thr, global_igc->arena);
>     IGC_CHECK_RES (res);
>     struct igc_thread_list *t = register_thread (global_igc, thr, ts);
> 
>   That's the root for the control stack of the thread:
> 
>     root_create_thread (t);
> 
>   These are allocation points in case the thread allocates from MPS:
> 
>     create_thread_aps (&t->d);
>     return t;
>   }
> 
> Maybe we should start with this, and move forward when we have it.

I'd first like to understand the need and the purpose of registering
non-Lisp threads with MPS.

Re: MPS: scroll-bars

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> How to go about that?

Please let's first do the thread stuff. That makes things easier for the
old man here.


>> I assume that the GUI thread you mention is not one the Emacs threads,
>> but something else, so it's not main_thread and not one of the threads
>> made with make-thread.
>
> It isn't a Lisp thread, that's true.  But it's an "Emacs thread" in
> the sense that in the w32 build it is an integral part of Emacs, and
> without it the GUI features and GUI I/O would not work on MS-Windows.

Does it have a struct thread_state associated with it? That's what
igc.c currently knows how to handle.

>
> (You can find the overview of the thread structure of the w32 build in
> a large comment that starts at line 3365 of w32fns.c.)
>
>> Then I think we first of all must make sure that MPS knows about that
>> thread.
>
> Why do we need MPS to know about that thread (or any other non-Lisp
> thread used by Emacs on MS-Windows; there are more)?  is that for MPS
> to scan their C stacks and registers?

9.2. Thread registration

In order to scan a thread’s registers for references (which happens at
each flip), the MPS needs to be able to suspend that thread, and in
order to gain exclusive atomic access to memory in order to scan it, the
MPS needs to be able to suspend all threads that might access that
memory. This means that threads must be registered with the MPS by
calling mps_thread_reg() (and thread roots created; see Thread roots).

A thread must be registered with an arena if:

its control stack and registers form a root (this is enforced by
mps_root_create_thread()); or it reads or writes from a location in an
automatically managed pool in the arena. However, some automatically
managed pool classes may be more liberal than this. See the
documentation for the pool class.

Re: MPS: w32 threads

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  luangruo@yahoo.com,  emacs-devel@gnu.org
> Date: Sat, 04 May 2024 12:04:50 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > How to go about that?
> 
> Please let's first do the thread stuff. That makes things easier for the
> old man here.

OK.  I've started a new thread (pun intended) for this branch of the
discussion.

> >> I assume that the GUI thread you mention is not one the Emacs threads,
> >> but something else, so it's not main_thread and not one of the threads
> >> made with make-thread.
> >
> > It isn't a Lisp thread, that's true.  But it's an "Emacs thread" in
> > the sense that in the w32 build it is an integral part of Emacs, and
> > without it the GUI features and GUI I/O would not work on MS-Windows.
> 
> Does it have a struct thread_state associated with it? That's what
> igc.c currently knows how to handle.

No.  Only Lisp threads started with make-thread have thread_state.

> A thread must be registered with an arena if:
> 
> its control stack and registers form a root (this is enforced by
> mps_root_create_thread()); or it reads or writes from a location in an
> automatically managed pool in the arena. However, some automatically
> managed pool classes may be more liberal than this. See the
> documentation for the pool class.

Can you try translating this to Emacs-related terms, please?  What
activities in the thread should require the thread to be "registered
with an arena"?

Emacs on Windows starts separate threads for the following jobs:

  . processing Windows GUI-related messages
  . reading from sub-processes (part of pselect emulation)
  . emulation of itimers (for atimers and for M-x profiler)
  . file-change notifications

I need to understand what traits of a thread make it need to be
registered with MPS.  Each of the above will then need a separate
audit, I think.

Re: MPS: w32 threads

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> No.  Only Lisp threads started with make-thread have thread_state.

OK.

(Plus main_thread, which is a thread_state in the data segment (!) for
the main thread. Just in case someone wonders why main_thread is a
root.)

>> A thread must be registered with an arena if:
>> 
>> its control stack and registers form a root (this is enforced by
>> mps_root_create_thread()); or it reads or writes from a location in an
>> automatically managed pool in the arena. However, some automatically
>> managed pool classes may be more liberal than this. See the
>> documentation for the pool class.
>
> Can you try translating this to Emacs-related terms, please?  What
> activities in the thread should require the thread to be "registered
> with an arena"?

It basically says that if a thread can have Lisp_Objects or pointers to
them on its control stack, register it with MPS.

An arena is basically an implementation of some low-level aspects of
memory managemnt in MPS. We have only one MPS arena, and it is one using
VM functions (mmap, ...).

The hint that some pool classes don't require this or that we can
ignore. AKAIK the mostly-copying pools we use require it.

> Emacs on Windows starts separate threads for the following jobs:
>
>   . processing Windows GUI-related messages
>   . reading from sub-processes (part of pselect emulation)
>   . emulation of itimers (for atimers and for M-x profiler)
>   . file-change notifications
>
> I need to understand what traits of a thread make it need to be
> registered with MPS.  Each of the above will then need a separate
> audit, I think.

Using any Lisp_Objects (or other objects allocated from MPS) -> register
it. AFAIU, the GUI thread gets such an objects posted in a message, so
that one must be registered.

The others are hard to say for me. It would not do harm to register
them, but might mean addtional work for nothing.

Re: MPS: w32 threads

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  luangruo@yahoo.com,  emacs-devel@gnu.org
> Date: Sat, 04 May 2024 16:20:19 +0200
> 
> >> A thread must be registered with an arena if:
> >> 
> >> its control stack and registers form a root (this is enforced by
> >> mps_root_create_thread()); or it reads or writes from a location in an
> >> automatically managed pool in the arena. However, some automatically
> >> managed pool classes may be more liberal than this. See the
> >> documentation for the pool class.
> >
> > Can you try translating this to Emacs-related terms, please?  What
> > activities in the thread should require the thread to be "registered
> > with an arena"?
> 
> It basically says that if a thread can have Lisp_Objects or pointers to
> them on its control stack, register it with MPS.
> 
> An arena is basically an implementation of some low-level aspects of
> memory managemnt in MPS. We have only one MPS arena, and it is one using
> VM functions (mmap, ...).
> 
> The hint that some pool classes don't require this or that we can
> ignore. AKAIK the mostly-copying pools we use require it.
> 
> > Emacs on Windows starts separate threads for the following jobs:
> >
> >   . processing Windows GUI-related messages
> >   . reading from sub-processes (part of pselect emulation)
> >   . emulation of itimers (for atimers and for M-x profiler)
> >   . file-change notifications
> >
> > I need to understand what traits of a thread make it need to be
> > registered with MPS.  Each of the above will then need a separate
> > audit, I think.
> 
> Using any Lisp_Objects (or other objects allocated from MPS) -> register
> it. AFAIU, the GUI thread gets such an objects posted in a message, so
> that one must be registered.
> 
> The others are hard to say for me. It would not do harm to register
> them, but might mean addtional work for nothing.

So how does one go about registering a thread?

I've just got up from a full hour reading the seemingly-related parts
of igc.c and the MPS documentation (the latter, I must say, is really
badly written and organized illogically), and I still have quite a few
questions about this.

First, the MPS manual says to call mps_thread_reg from the thread
itself, to register a thread.  That function accepts the ARENA
argument.  I see we have a single global arena, but should other
threads use the same arena, or should they create their own?  The
considerations for that are not clear (are they documented
somewhere?), I only see that you used the same arena for the Lisp
threads in thread_add.  Is that arena defined in a way that could
serve additional threads, perhaps many of them?  (The w32 build starts
a separate thread for any file-notification watch, up to 64 threads
for sub-process communications, and one thread per atimer, so in
principle we could have quite a few of them, if a session is a heavy
user of these features.)

While trying to think about the need for an additional arena, I took a
look at the relevant APIs, and became confused with what we do (or
don't do) in make_arena.  All the keywords we add  with MPS_ARGS_ADD
and the magic constants there -- what do they mean and what were the
considerations to choose them and not the others?  Can some commentary
be added there to explain what we need to know, and maybe even mention
optional features we might consider in the future?

Assuming we can do with the single global arena already defined, the
MPS documentation next says that every thread's registers and stack
should be registered using mps_root_create_thread.  But the code in
igc.c uses mps_root_create_thread_scanned instead, whose documentation
is obscure, and I cannot figure out whether to copycat or do something
else.

Next, mps_root_create_thread and mps_root_create_thread_scanned
require a COLD pointer, about which the MPS documentation says:

  The MPS’s stack scanner needs to know how to find the cold end of the
  part of the stack to scan.  The cold end of the relevant part of the
  stack can be found by taking the address of a local variable in the
  function that calls the main work function of your thread.  You should
  take care to ensure that the work function is not inlined so that the
  address is definitely in the stack frame below any potential roots.

But in our case, the thread function is run directly by the MS-Windows
CreateThread API, not via an intermediate function, so the worker
function is effectively "inlined".  So is it okay to have the cold end
marker be a local variable in the same thread-worker function?  In
another place of the MPS documentation there's an example which seems
to answer in the positive:

     void *marker = &marker;
     mps_root_t stack_root;
     res = mps_root_create_thread(&stack_root, arena, thread, marker);
     if (res != MPS_RES_OK) error("Couldn't create stack root");

Am I missing something, or could the above be at the very beginning of
a thread's worker function?  Or do we have to define intermediary
functions for each thread worker?

mps_root_create_thread_scanned also takes the CLOSURE argument, which
is a pointer, and we pass a NULL pointer as its value(??).  Is this
because our scan_area function, scan_ambig, simply ignores the CLOSURE
argument?

What else needs to be done for "registering a thread"?  igc_thread_add
does a few other things; some of them are Lisp-related (so not
relevant for non-Lisp threads), but others aren't, so I'm unsure about
these:

  . igc_thread_list_push (what is it? and where defined?)
  . igc_root_list_push (where is it defined and what does it do?)
  . create_thread_aps (what is it for?)

I think some minimal commentary to what these functions do is in
order, to make the code look less like some "black magic".

TIA

Re: MPS: w32 threads

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> First, the MPS manual says to call mps_thread_reg from the thread
> itself, to register a thread.  That function accepts the ARENA
> argument.  I see we have a single global arena, but should other
> threads use the same arena, or should they create their own?  The
> considerations for that are not clear (are they documented
> somewhere?), I only see that you used the same arena for the Lisp
> threads in thread_add.  Is that arena defined in a way that could
> serve additional threads, perhaps many of them?  (The w32 build starts
> a separate thread for any file-notification watch, up to 64 threads
> for sub-process communications, and one thread per atimer, so in
> principle we could have quite a few of them, if a session is a heavy
> user of these features.)

Let's say we made a thread_state for GUI thread. If we had that, you
could simply call this:

static struct igc_thread_list *
thread_add (struct thread_state *ts)
{
  mps_thr_t thr;
  mps_res_t res = mps_thread_reg (&thr, global_igc->arena);
  IGC_CHECK_RES (res);
  struct igc_thread_list *t = register_thread (global_igc, thr, ts);
  root_create_thread (t);
  create_thread_aps (&t->d);
  return t;
}

from the GUI thread. That does everything you could possibly need. The
igc_thread_add "API" does more because other threads have their own
specpdl and byte code stack.

> While trying to think about the need for an additional arena, I took a
> look at the relevant APIs, and became confused with what we do (or
> don't do) in make_arena.  All the keywords we add  with MPS_ARGS_ADD
> and the magic constants there -- what do they mean and what were the
> considerations to choose them and not the others?  Can some commentary
> be added there to explain what we need to know, and maybe even mention
> optional features we might consider in the future?

What's currently there is simply the result of me reading the docs, and
picking what I thought would be needed. I guess there are a lot of knobs
that could be tried... That is also the case for pools, generations....

> Assuming we can do with the single global arena already defined, 

There is no need for an additional arena.

> the MPS documentation next says that every thread's registers and
> stack should be registered using mps_root_create_thread. But the code
> in igc.c uses mps_root_create_thread_scanned instead, whose
> documentation is obscure, and I cannot figure out whether to copycat
> or do something else.

The _scanned variant allows us to use our own scan method, which is
necessary because how we represent symbols in Lisp_Objects (the NULL ==
nil thing.)

> Next, mps_root_create_thread and mps_root_create_thread_scanned
> require a COLD pointer, about which the MPS documentation says:
>
>   The MPS’s stack scanner needs to know how to find the cold end of the
>   part of the stack to scan.  The cold end of the relevant part of the
>   stack can be found by taking the address of a local variable in the
>   function that calls the main work function of your thread.  You should
>   take care to ensure that the work function is not inlined so that the
>   address is definitely in the stack frame below any potential roots.
>
> But in our case, the thread function is run directly by the MS-Windows
> CreateThread API, not via an intermediate function, so the worker
> function is effectively "inlined".  So is it okay to have the cold end
> marker be a local variable in the same thread-worker function?  In
> another place of the MPS documentation there's an example which seems
> to answer in the positive:
>
>      void *marker = &marker;
>      mps_root_t stack_root;
>      res = mps_root_create_thread(&stack_root, arena, thread, marker);
>      if (res != MPS_RES_OK) error("Couldn't create stack root");
>
> Am I missing something, or could the above be at the very beginning of
> a thread's worker function?  Or do we have to define intermediary
> functions for each thread worker?

I'm not sure I have understood how that works. Is it like in pthread
that you create a thread specifying a thread_main? And when you return
from thread_main the thread is over?

> mps_root_create_thread_scanned also takes the CLOSURE argument, which
> is a pointer, and we pass a NULL pointer as its value(??).  Is this
> because our scan_area function, scan_ambig, simply ignores the CLOSURE
> argument?

The closure arg is for use by us, as we want. An example is dflt_scanx
that is also used for walking over all objects. Kind of a visitor pattern.

> What else needs to be done for "registering a thread"?  igc_thread_add
> does a few other things; some of them are Lisp-related (so not
> relevant for non-Lisp threads), but others aren't, so I'm unsure about
> these:

I think igc_thread_add does too much for the GUI thread, see above.

>   . igc_thread_list_push (what is it? and where defined?)
>   . igc_root_list_push (where is it defined and what does it do?)

It's a very ppor man's alternative to C++ std::list, implemented with
abominable macros, see IGC_DEFINE_LIS.

>   . create_thread_aps (what is it for?)

These are allocation points for allocating from MPS. Each thread needs
its own aps. (It wouldn't hurt to have aps, even if they are not used.)

> I think some minimal commentary to what these functions do is in
> order, to make the code look less like some "black magic".

I know :-).

In summary, I would really recommend, if technically possible on Window,
to set up a thread_state and use what's there in igc.c.

Re: MPS: w32 threads

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Sun, 05 May 2024 11:16:04 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> Let's say we made a thread_state for GUI thread. If we had that, you
> could simply call this:
> 
> static struct igc_thread_list *
> thread_add (struct thread_state *ts)
> {
>   mps_thr_t thr;
>   mps_res_t res = mps_thread_reg (&thr, global_igc->arena);
>   IGC_CHECK_RES (res);
>   struct igc_thread_list *t = register_thread (global_igc, thr, ts);
>   root_create_thread (t);
>   create_thread_aps (&t->d);
>   return t;
> }

thread_state is HUGE!  Almost everything there is unneeded for the
other threads, which are not Lisp threads, they run straightforward
and quite simple C code.  So I'd like to avoid that if possible, which
is why I asked all those questions.

> > While trying to think about the need for an additional arena, I took a
> > look at the relevant APIs, and became confused with what we do (or
> > don't do) in make_arena.  All the keywords we add  with MPS_ARGS_ADD
> > and the magic constants there -- what do they mean and what were the
> > considerations to choose them and not the others?  Can some commentary
> > be added there to explain what we need to know, and maybe even mention
> > optional features we might consider in the future?
> 
> What's currently there is simply the result of me reading the docs, and
> picking what I thought would be needed. I guess there are a lot of knobs
> that could be tried... That is also the case for pools, generations....

I'd appreciate if you could add at least some comments about why you
thought this stuff and not the rest.

> > Assuming we can do with the single global arena already defined, 
> 
> There is no need for an additional arena.

OK, thanks.

> > the MPS documentation next says that every thread's registers and
> > stack should be registered using mps_root_create_thread. But the code
> > in igc.c uses mps_root_create_thread_scanned instead, whose
> > documentation is obscure, and I cannot figure out whether to copycat
> > or do something else.
> 
> The _scanned variant allows us to use our own scan method, which is
> necessary because how we represent symbols in Lisp_Objects (the NULL ==
> nil thing.)

Got it, thanks.

> >   The MPS’s stack scanner needs to know how to find the cold end of the
> >   part of the stack to scan.  The cold end of the relevant part of the
> >   stack can be found by taking the address of a local variable in the
> >   function that calls the main work function of your thread.  You should
> >   take care to ensure that the work function is not inlined so that the
> >   address is definitely in the stack frame below any potential roots.
> >
> > But in our case, the thread function is run directly by the MS-Windows
> > CreateThread API, not via an intermediate function, so the worker
> > function is effectively "inlined".  So is it okay to have the cold end
> > marker be a local variable in the same thread-worker function?  In
> > another place of the MPS documentation there's an example which seems
> > to answer in the positive:
> >
> >      void *marker = &marker;
> >      mps_root_t stack_root;
> >      res = mps_root_create_thread(&stack_root, arena, thread, marker);
> >      if (res != MPS_RES_OK) error("Couldn't create stack root");
> >
> > Am I missing something, or could the above be at the very beginning of
> > a thread's worker function?  Or do we have to define intermediary
> > functions for each thread worker?
> 
> I'm not sure I have understood how that works. Is it like in pthread
> that you create a thread specifying a thread_main? And when you return
> from thread_main the thread is over?

Yes.  Example:

	  cp->thrd = CreateThread (NULL, 64 * 1024, reader_thread, cp,
				   0x00010000, &id);

where reader_thread is the thread worker function.  When the thread's
work is done, that function exits, and the thread exits as well.

My question is whether I can make the mps_root_create_thread call from
inside reader_thread and other similar worker functions, or do I need
to go through an intermediary, like so:

  static int
  worker (...)
  {
    void *marker = &marker;
    mps_root_t stack_root;
    res = mps_root_create_thread(&stack_root, arena, thread, marker);
    if (res != MPS_RES_OK) error("Couldn't create stack root");
    return real_worker ();
  }

  CreateThread (..., worker, ...);

> >   . igc_thread_list_push (what is it? and where defined?)
> >   . igc_root_list_push (where is it defined and what does it do?)
> 
> It's a very ppor man's alternative to C++ std::list, implemented with
> abominable macros, see IGC_DEFINE_LIS.

I reckon we don't need to keep the threads I'm talking about on a
list, since they don't have any specpdl pointers?

> >   . create_thread_aps (what is it for?)
> 
> These are allocation points for allocating from MPS. Each thread needs
> its own aps. (It wouldn't hurt to have aps, even if they are not used.)

OK, but what is each of those 4 aps for? are all of them needed for a
thread that doesn't run Lisp?  I see these aps are stored in struct
igc_thread, which seems to be for Lisp threads?

> In summary, I would really recommend, if technically possible on Window,
> to set up a thread_state and use what's there in igc.c.

I'd like to avoid that, for reasons explained above.

Thanks.

Re: MPS: w32 threads

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
>> Date: Sun, 05 May 2024 11:16:04 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> Let's say we made a thread_state for GUI thread. If we had that, you
>> could simply call this:
>> 
>> static struct igc_thread_list *
>> thread_add (struct thread_state *ts)
>> {
>>   mps_thr_t thr;
>>   mps_res_t res = mps_thread_reg (&thr, global_igc->arena);
>>   IGC_CHECK_RES (res);
>>   struct igc_thread_list *t = register_thread (global_igc, thr, ts);
>>   root_create_thread (t);
>>   create_thread_aps (&t->d);
>>   return t;
>> }
>
> thread_state is HUGE!  Almost everything there is unneeded for the
> other threads, which are not Lisp threads, they run straightforward
> and quite simple C code.  So I'd like to avoid that if possible, which
> is why I asked all those questions.

Understandable, but then we have more work to do in igc. main_thread
could also be an example

union aligned_thread_state main_thread
  = {{
      .header.size = PVECHEADERSIZE (PVEC_THREAD,
				     PSEUDOVECSIZE (struct thread_state,
						    event_object),
				     VECSIZE (struct thread_state)),
      .m_last_thing_searched = LISPSYM_INITIALLY (Qnil),
      .m_saved_last_thing_searched = LISPSYM_INITIALLY (Qnil),
      .name = LISPSYM_INITIALLY (Qnil),
      .function = LISPSYM_INITIALLY (Qnil),
      .result = LISPSYM_INITIALLY (Qnil),
      .error_symbol = LISPSYM_INITIALLY (Qnil),
      .error_data = LISPSYM_INITIALLY (Qnil),
      .event_object = LISPSYM_INITIALLY (Qnil),
    }};

Not sure what more we'd need except the stack start address. Plus
potentially 2 ifs that become necessary because so far all thread has a
byte code stack and specpdl.

>> > While trying to think about the need for an additional arena, I took a
>> > look at the relevant APIs, and became confused with what we do (or
>> > don't do) in make_arena.  All the keywords we add  with MPS_ARGS_ADD
>> > and the magic constants there -- what do they mean and what were the
>> > considerations to choose them and not the others?  Can some commentary
>> > be added there to explain what we need to know, and maybe even mention
>> > optional features we might consider in the future?
>> 
>> What's currently there is simply the result of me reading the docs, and
>> picking what I thought would be needed. I guess there are a lot of knobs
>> that could be tried... That is also the case for pools, generations....
>
> I'd appreciate if you could add at least some comments about why you
> thought this stuff and not the rest.

I'll see if I can remember.

>> >   The MPS’s stack scanner needs to know how to find the cold end of the
>> >   part of the stack to scan.  The cold end of the relevant part of the
>> >   stack can be found by taking the address of a local variable in the
>> >   function that calls the main work function of your thread.  You should
>> >   take care to ensure that the work function is not inlined so that the
>> >   address is definitely in the stack frame below any potential roots.
>> >
>> > But in our case, the thread function is run directly by the MS-Windows
>> > CreateThread API, not via an intermediate function, so the worker
>> > function is effectively "inlined".  So is it okay to have the cold end
>> > marker be a local variable in the same thread-worker function?  In
>> > another place of the MPS documentation there's an example which seems
>> > to answer in the positive:
>> >
>> >      void *marker = &marker;
>> >      mps_root_t stack_root;
>> >      res = mps_root_create_thread(&stack_root, arena, thread, marker);
>> >      if (res != MPS_RES_OK) error("Couldn't create stack root");
>> >
>> > Am I missing something, or could the above be at the very beginning of
>> > a thread's worker function?  Or do we have to define intermediary
>> > functions for each thread worker?
>> 
>> I'm not sure I have understood how that works. Is it like in pthread
>> that you create a thread specifying a thread_main? And when you return
>> from thread_main the thread is over?
>
> Yes.  Example:
>
> 	  cp->thrd = CreateThread (NULL, 64 * 1024, reader_thread, cp,
> 				   0x00010000, &id);
>
> where reader_thread is the thread worker function.  When the thread's
> work is done, that function exits, and the thread exits as well.

Ok, that's good.

> My question is whether I can make the mps_root_create_thread call from
> inside reader_thread and other similar worker functions, or do I need
> to go through an intermediary, like so:

No, directly. Like in thread.c run_thread which does in pseudo code

run_thread ():
  self->gc_info = igc_thread_add (self)
  run_lisp_funcion ()
  igc_thread_remove (self->gc_info)
  
where self here is a thread_state. 

>
>   static int
>   worker (...)
>   {
>     void *marker = &marker;
>     mps_root_t stack_root;
>     res = mps_root_create_thread(&stack_root, arena, thread, marker);
>     if (res != MPS_RES_OK) error("Couldn't create stack root");
>     return real_worker ();
>   }
>
>   CreateThread (..., worker, ...);
>
>> >   . igc_thread_list_push (what is it? and where defined?)
>> >   . igc_root_list_push (where is it defined and what does it do?)
>> 
>> It's a very ppor man's alternative to C++ std::list, implemented with
>> abominable macros, see IGC_DEFINE_LIS.
>
> I reckon we don't need to keep the threads I'm talking about on a
> list, since they don't have any specpdl pointers?

Igc theoretically uses the registry of MPS object sthat struct igc is
for freeing stuff when the program ends. So if the thread was registered
with MPS, then it's best if it is added to igc.

Since specpdl and bytecode stack were currently always required it
might be that we have to add some if (this-and-that != NULL).

>> >   . create_thread_aps (what is it for?)
>> 
>> These are allocation points for allocating from MPS. Each thread needs
>> its own aps. (It wouldn't hurt to have aps, even if they are not used.)
>
> OK, but what is each of those 4 aps for? are all of them needed for a
> thread that doesn't run Lisp?  I see these aps are stored in struct
> igc_thread, which seems to be for Lisp threads?

Currently igc_thread == Lisp thread because we did have others yet. Each
thread that wants to allocate from MPS needs 1 ap for each pool we are
using. Except for the weak pool, which needs 2 aps.

If a thread doesn't want to allocate, it strictly speacking doesn't need
aps but my intuition says that only complicates matters without saving
much.

Re: MPS: w32 threads

[Gerd Möllmann]

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

> Not sure what more we'd need except the stack start address. Plus
> potentially 2 ifs that become necessary because so far all thread has a
> byte code stack and specpdl.

Oops, and of course if we make that a variable like main_thread it
woiuld need to be a root like main_thread is.

Re: MPS: scroll-bars

[Po Lu]

Helmut Eller <eller.helmut@gmail.com> writes:

> xg_gtk_scroll_destroy uses id_to_widget. That's a global variable.  I'd
> rather avoid that.

xg_gtk_scroll_destroy does, but nothing forces the function that unpins
the widget to take the same approach.  The scroll bar pseudovector
structure can as easily be provided as callback data.

Re: MPS: scroll-bars

[Gerd Möllmann]

I believe the frame::condemned_scroll_bars is okay on NS as it is now.
With all caveats as usual.

Re: MPS: scroll-bars

[Helmut Eller]

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

On Sun, May 05 2024, Gerd Möllmann wrote:

> I believe the frame::condemned_scroll_bars is okay on NS as it is now.
> With all caveats as usual.

I have a hunch that those would need weak references to work as
intended.  But I'm happy to let them leak for now.

Below is my patch for the scroll-bars.  I went for the indirection layer
solution.  Though, the mark-and-sweep pool idea might be more broadly
usable; with so many different GUI toolkits it's hard to say.

I also had a problem with the tool-bar, triggered by this test:
(dotimes (i 40)
  (igc--collect)
  (message "%S" (make-list i nil))
  (redisplay))

The third patch stops allocating glyph matrices as ambiguous roots.
Seems to work for me.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-scroll-bars-for-GTK.patch --]
[-- Type: text/x-diff, Size: 5363 bytes --]

From 0e0cc32798f6dc8947e411d0e58ae4b8ea2aaced Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Sun, 5 May 2024 06:36:24 +0200
Subject: [PATCH] Fix scroll bars for GTK

Use a layer of indirection when struct scroll_bar pointers are
used in GTK callbacks.

* src/gtkutil.c (xg_finish_scroll_bar_creation): Create a cell
with a pointer to the scroll_bar and pass it to callbacks.  Also
Register a "destroy" callback to free it.
(xg_gtk_scroll_free_scroll_bar_cell): New.

* src/xterm.c (xg_scroll_callback, xg_end_scroll_callback): Load
the scroll_bar* out the cell.
(syms_of_xterm): Staticpro window_being_scrolled.
(x_unprotect_window_for_callback): Clear unused
elements in the protected_windows array to allow windows to be
collected.
(x_cache_xi_devices, xi_disable_devices, x_term_init): Add FIXMEs.
---
 src/gtkutil.c | 37 +++++++++++++++++++++++++++++++++++--
 src/xterm.c   | 21 +++++++++++++++++++++
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/src/gtkutil.c b/src/gtkutil.c
index a01b8a41d5d..342dd35da13 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -44,6 +44,9 @@ #define xp pgtk
 #include "termhooks.h"
 #include "keyboard.h"
 #include "coding.h"
+#ifdef HAVE_MPS
+#include "igc.h"
+#endif
 
 #include <gdk/gdkkeysyms.h>
 
@@ -4523,6 +4526,13 @@ xg_scroll_bar_size_allocate_cb (GtkWidget *widget,
 }
 #endif
 
+static void
+xg_gtk_scroll_free_scroll_bar_cell (GtkWidget *widget, gpointer data)
+{
+  struct scroll_bar **bar_cell = data;
+  igc_xfree (bar_cell);
+}
+
 static void
 xg_finish_scroll_bar_creation (struct frame *f,
                                GtkWidget *wscroll,
@@ -4558,14 +4568,37 @@ xg_finish_scroll_bar_creation (struct frame *f,
                     (gpointer) scroll_id);
 #endif
 
+#ifdef HAVE_MPS
+  // FIXME: can use exact reference
+  struct scroll_bar **bar_cell =
+    igc_xzalloc_ambig (sizeof (struct scroll_bar*));
+  bar_cell[0] = bar;
+#endif
   g_signal_connect (G_OBJECT (wscroll),
                     "change-value",
                     scroll_callback,
-                    (gpointer) bar);
+#ifdef HAVE_MPS
+                    (gpointer) bar_cell
+#else
+		    (gpointer) bar
+#endif
+		    );
   g_signal_connect (G_OBJECT (wscroll),
                     "button-release-event",
                     end_callback,
-                    (gpointer) bar);
+#ifdef HAVE_MPS
+                    (gpointer) bar_cell
+#else
+		    (gpointer) bar
+#endif
+		    );
+
+#ifdef HAVE_MPS
+    g_signal_connect (G_OBJECT (wscroll),
+		      "destroy",
+		      G_CALLBACK (xg_gtk_scroll_free_scroll_bar_cell),
+		      (gpointer) bar_cell);
+#endif
 
   /* The scroll bar widget does not draw on a window of its own.  Instead
      it draws on the parent window, in this case the edit widget.  So
diff --git a/src/xterm.c b/src/xterm.c
index c47c7b0534d..7acd7f9c2a5 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -5748,6 +5748,7 @@ x_cache_xi_devices (struct x_display_info *dpyinfo)
     }
 
 #ifdef HAVE_MPS
+  // FIXME: use exact references
   dpyinfo->devices = igc_xzalloc_ambig (sizeof *dpyinfo->devices * ndevices);
 #else
   dpyinfo->devices = xzalloc (sizeof *dpyinfo->devices * ndevices);
@@ -13782,6 +13783,7 @@ xi_disable_devices (struct x_display_info *dpyinfo,
 
   ndevices = 0;
 #ifdef HAVE_MPS
+  // FIXME: use exact references
   devices = igc_xzalloc_ambig (sizeof *devices * dpyinfo->num_devices);
 #else
   devices = xzalloc (sizeof *devices * dpyinfo->num_devices);
@@ -15480,6 +15482,10 @@ x_unprotect_window_for_callback (struct x_display_info *dpyinfo)
     memmove (dpyinfo->protected_windows, &dpyinfo->protected_windows[1],
 	     sizeof (Lisp_Object) * dpyinfo->n_protected_windows);
 
+#ifdef HAVE_MPS
+  dpyinfo->protected_windows[dpyinfo->n_protected_windows] = Qnil;
+#endif
+
   return window;
 }
 
@@ -15718,7 +15724,12 @@ xg_scroll_callback (GtkRange *range, GtkScrollType scroll,
 
   whole = 0;
   portion = 0;
+#ifdef HAVE_MPS
+  struct scroll_bar** bar_cell = user_data;
+  bar = *bar_cell;
+#else
   bar = user_data;
+#endif
   part = scroll_bar_nowhere;
   adj = GTK_ADJUSTMENT (gtk_range_get_adjustment (range));
   f = g_object_get_data (G_OBJECT (range), XG_FRAME_DATA);
@@ -15795,9 +15806,14 @@ xg_end_scroll_callback (GtkWidget *widget,
                         GdkEventButton *event,
                         gpointer user_data)
 {
+#ifdef HAVE_MPS
+  struct scroll_bar **bar_cell = user_data;
+  struct scroll_bar *bar = *bar_cell;
+#else
   struct scroll_bar *bar;
 
   bar = user_data;
+#endif
   bar->dragging = -1;
 
   if (WINDOWP (window_being_scrolled))
@@ -16936,6 +16952,7 @@ XTset_vertical_scroll_bar (struct window *w, int portion, int whole, int positio
 	 Redraw the scroll bar manually.  */
       x_scroll_bar_redraw (bar);
 #endif
+
     }
   else
     {
@@ -30693,6 +30710,7 @@ #define NUM_ARGV 10
   /* We have definitely succeeded.  Record the new connection.  */
 
 #ifdef HAVE_MPS
+  // FIXME: use exact references
   dpyinfo = igc_xzalloc_ambig (sizeof *dpyinfo);
 #else
   dpyinfo = xzalloc (sizeof *dpyinfo);
@@ -32498,6 +32516,9 @@ syms_of_xterm (void)
   x_dnd_unsupported_drop_data = Qnil;
   staticpro (&x_dnd_unsupported_drop_data);
 
+  window_being_scrolled = Qnil;
+  staticpro (&window_being_scrolled);
+
   /* Used by x_cr_export_frames.  */
   DEFSYM (Qconcat, "concat");
 
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Make-xg_frame_tb_info-an-ambiguous-root.patch --]
[-- Type: text/x-diff, Size: 1357 bytes --]

From fd98e52e0efdb5e7bbf37919e3e95f8e440313b0 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Sun, 5 May 2024 09:01:06 +0200
Subject: [PATCH] Make xg_frame_tb_info an ambiguous root

We could use exact references in the future.

* src/gtkutil.c (xg_create_tool_bar): Use igc_xzalloc_ambig for
allocation.
(free_frame_tool_bar): Use igc_xfree to free it.
---
 src/gtkutil.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/gtkutil.c b/src/gtkutil.c
index 342dd35da13..31e0b915000 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -5496,7 +5496,12 @@ xg_create_tool_bar (struct frame *f)
                          TB_INFO_KEY);
   if (! tbinfo)
     {
+#ifdef HAVE_MPS
+      // FIXME: use exact references
+      tbinfo = igc_xzalloc_ambig (sizeof (*tbinfo));
+#else
       tbinfo = xmalloc (sizeof (*tbinfo));
+#endif
       tbinfo->last_tool_bar = Qnil;
       tbinfo->style = Qnil;
       tbinfo->hmargin = tbinfo->vmargin = 0;
@@ -6162,7 +6167,11 @@ free_frame_tool_bar (struct frame *f)
                                   TB_INFO_KEY);
       if (tbinfo)
         {
+#ifdef HAVE_MPS
+	  igc_xfree (tbinfo);
+#else
           xfree (tbinfo);
+#endif
           g_object_set_data (G_OBJECT (FRAME_GTK_OUTER_WIDGET (f)),
                              TB_INFO_KEY,
                              NULL);
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0001-Glyph-matrices-don-t-need-to-be-ambiguous-roots.patch --]
[-- Type: text/x-diff, Size: 3120 bytes --]

From 3d3187e5c55a770cd68b1144479a70032b3c3558 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Sun, 5 May 2024 09:38:48 +0200
Subject: [PATCH] Glyph matrices don't need to be ambiguous roots

We scan them with fix_glyph_matrix as part of fix_windows.  So avoid
scanning them twice.  save_current_matrix and
restore_current_matrix create temporary matrices that aren't
seen by fix_glyph_matrix; these are still ambiguous roots.

* src/dispnew.c (adjust_glyph_matrix, realloc_glyph_pool)
(free_glyph_matrix): Use plain malloc instead of igc_xzalloc_ambig.
(restore_current_matrix): Use igc_free where needed.
---
 src/dispnew.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/src/dispnew.c b/src/dispnew.c
index 7bb78c3b214..4d7586bdc1e 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -304,11 +304,7 @@ free_glyph_matrix (struct glyph_matrix *matrix)
       /* Free glyph memory if MATRIX owns it.  */
       if (matrix->pool == NULL)
 	for (i = 0; i < matrix->rows_allocated; ++i)
-#ifdef HAVE_MPS
-	  igc_xfree (matrix->rows[i].glyphs[LEFT_MARGIN_AREA]);
-#else
 	  xfree (matrix->rows[i].glyphs[LEFT_MARGIN_AREA]);
-#endif
       /* Free row structures and the matrix itself.  */
       xfree (matrix->rows);
       xfree (matrix);
@@ -512,13 +508,8 @@ adjust_glyph_matrix (struct window *w, struct glyph_matrix *matrix, int x, int y
 	  while (row < end)
 	    {
 	      row->glyphs[LEFT_MARGIN_AREA] =
-#ifdef HAVE_MPS
-		igc_xnrealloc_ambig (row->glyphs[LEFT_MARGIN_AREA],
-			       dim.width, sizeof (struct glyph));
-#else
 		xnrealloc (row->glyphs[LEFT_MARGIN_AREA],
 			   dim.width, sizeof (struct glyph));
-#endif
 	      /* The mode line, if displayed, never has marginal areas.  */
 	      if ((row == matrix->rows + dim.height - 1
 		   && !(w && window_wants_mode_line (w)))
@@ -1374,11 +1365,7 @@ free_glyph_pool (struct glyph_pool *pool)
       --glyph_pool_count;
       eassert (glyph_pool_count >= 0);
 #endif
-#ifdef HAVE_MPS
-      igc_xfree (pool->glyphs);
-#else
       xfree (pool->glyphs);
-#endif
       xfree (pool);
     }
 }
@@ -1409,15 +1396,9 @@ realloc_glyph_pool (struct glyph_pool *pool, struct dim matrix_dim)
   if (needed > pool->nglyphs)
     {
       ptrdiff_t old_nglyphs = pool->nglyphs;
-#ifdef HAVE_MPS
-      pool->glyphs
-	= igc_xpalloc_ambig (pool->glyphs, &pool->nglyphs, needed - old_nglyphs,
-		       -1, sizeof *pool->glyphs);
-#else
       pool->glyphs
 	= xpalloc (pool->glyphs, &pool->nglyphs, needed - old_nglyphs,
 		   -1, sizeof *pool->glyphs);
-#endif
 
       memclear (pool->glyphs + old_nglyphs,
 		(pool->nglyphs - old_nglyphs) * sizeof *pool->glyphs);
@@ -2055,8 +2036,9 @@ restore_current_matrix (struct frame *f, struct glyph_matrix *saved)
 	  memcpy (to->glyphs[RIGHT_MARGIN_AREA],
 		  from->glyphs[RIGHT_MARGIN_AREA], nbytes);
 	  to->used[RIGHT_MARGIN_AREA] = from->used[RIGHT_MARGIN_AREA];
+
 #ifdef HAVE_MPS
-	  xfree (from->glyphs[RIGHT_MARGIN_AREA]);
+	  igc_xfree (from->glyphs[RIGHT_MARGIN_AREA]);
 #else
 	  xfree (from->glyphs[RIGHT_MARGIN_AREA]);
 #endif
-- 
2.39.2

Re: MPS: scroll-bars

[Gerd Möllmann]

Helmut Eller <eller.helmut@gmail.com> writes:

> On Sun, May 05 2024, Gerd Möllmann wrote:
>
>> I believe the frame::condemned_scroll_bars is okay on NS as it is now.
>> With all caveats as usual.
>
> I have a hunch that those would need weak references to work as
> intended.  But I'm happy to let them leak for now.
>
> Below is my patch for the scroll-bars.  I went for the indirection layer
> solution.  Though, the mark-and-sweep pool idea might be more broadly
> usable; with so many different GUI toolkits it's hard to say.
>
> I also had a problem with the tool-bar, triggered by this test:
> (dotimes (i 40)
>   (igc--collect)
>   (message "%S" (make-list i nil))
>   (redisplay))
>
> The third patch stops allocating glyph matrices as ambiguous roots.
> Seems to work for me.

Very cool!

I don't use tool bars with my init, so I've evaded that one :-).

And pushed. (I hope I haven't overlooked other patches you sent, like
yesterday?)

Re: MPS: scroll-bars

[Helmut Eller]

On Sun, May 05 2024, Gerd Möllmann wrote:

> And pushed.

Thanks!

> (I hope I haven't overlooked other patches you sent, like
> yesterday?)

No, you haven't; they're all there.

Re: MPS: scroll-bars

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Po Lu <luangruo@yahoo.com>,  Eli Zaretskii <eliz@gnu.org>,  Emacs Devel
>  <emacs-devel@gnu.org>
> Date: Sun, 05 May 2024 09:53:33 +0200
> 
> Below is my patch for the scroll-bars.  I went for the indirection layer
> solution.  Though, the mark-and-sweep pool idea might be more broadly
> usable; with so many different GUI toolkits it's hard to say.

Can you tell, for my own education, what does a call to
igc_xzalloc_ambig give us, as opposed to just xzalloc?

TIA

Re: MPS: scroll-bars

[Helmut Eller]

On Sun, May 05 2024, Eli Zaretskii wrote:

> Can you tell, for my own education, what does a call to
> igc_xzalloc_ambig give us, as opposed to just xzalloc?

igc_xzalloc_ambig allocates memory like xzalloc but it addition
registers it as root.  When MPS starts scanning, it begins from the
roots.  That means, we are sure that the referenced objects stay alive.
In the case of ambiguous references, the objects are also pinned
(i.e. don't move).

That pinning part is not necessary for the scroll-bar case, but
igc_xzalloc_ambig is currently the easiest to use function available in
igc.h.

Re: MPS: scroll-bars

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  luangruo@yahoo.com,  emacs-devel@gnu.org
> Date: Sun, 05 May 2024 20:02:13 +0200
> 
> On Sun, May 05 2024, Eli Zaretskii wrote:
> 
> > Can you tell, for my own education, what does a call to
> > igc_xzalloc_ambig give us, as opposed to just xzalloc?
> 
> igc_xzalloc_ambig allocates memory like xzalloc but it addition
> registers it as root.  When MPS starts scanning, it begins from the
> roots.  That means, we are sure that the referenced objects stay alive.
> In the case of ambiguous references, the objects are also pinned
> (i.e. don't move).
> 
> That pinning part is not necessary for the scroll-bar case, but
> igc_xzalloc_ambig is currently the easiest to use function available in
> igc.h.

Thanks, I think I get this.

Re: MPS: scroll-bars

[Eli Zaretskii]

> Date: Sun, 05 May 2024 21:09:53 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: gerd.moellmann@gmail.com, luangruo@yahoo.com, emacs-devel@gnu.org
> 
> > From: Helmut Eller <eller.helmut@gmail.com>
> > Cc: gerd.moellmann@gmail.com,  luangruo@yahoo.com,  emacs-devel@gnu.org
> > Date: Sun, 05 May 2024 20:02:13 +0200
> > 
> > On Sun, May 05 2024, Eli Zaretskii wrote:
> > 
> > > Can you tell, for my own education, what does a call to
> > > igc_xzalloc_ambig give us, as opposed to just xzalloc?
> > 
> > igc_xzalloc_ambig allocates memory like xzalloc but it addition
> > registers it as root.  When MPS starts scanning, it begins from the
> > roots.  That means, we are sure that the referenced objects stay alive.
> > In the case of ambiguous references, the objects are also pinned
> > (i.e. don't move).
> > 
> > That pinning part is not necessary for the scroll-bar case, but
> > igc_xzalloc_ambig is currently the easiest to use function available in
> > igc.h.
> 
> Thanks, I think I get this.

Btw, did we decide what to do with these chains:

  struct frame
  {
  [...]
  /* List of scroll bars on this frame.
     Actually, we don't specify exactly what is stored here at all; the
     scroll bar implementation code can use it to store anything it likes.
     This field is marked by the garbage collector.  It is here
     instead of in the `device' structure so that the garbage
     collector doesn't need to look inside the window-system-dependent
     structure.  */
  Lisp_Object scroll_bars;
  Lisp_Object condemned_scroll_bars;

They are doubly-linked lists via the ->next and ->prev fields in the C
'struct scroll_bar', in X as well as w32, see XSCROLL_BAR.  The
Lisp_Object's above are the last scroll bar on the frame's windows,
and the rest are reachable via the ->next and ->prev pointers.  Do we
need to do anything about those ->next and ->prev pointers?

Re: MPS: scroll-bars

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sun, 05 May 2024 21:09:53 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: gerd.moellmann@gmail.com, luangruo@yahoo.com, emacs-devel@gnu.org
>> 
>> > From: Helmut Eller <eller.helmut@gmail.com>
>> > Cc: gerd.moellmann@gmail.com,  luangruo@yahoo.com,  emacs-devel@gnu.org
>> > Date: Sun, 05 May 2024 20:02:13 +0200
>> > 
>> > On Sun, May 05 2024, Eli Zaretskii wrote:
>> > 
>> > > Can you tell, for my own education, what does a call to
>> > > igc_xzalloc_ambig give us, as opposed to just xzalloc?
>> > 
>> > igc_xzalloc_ambig allocates memory like xzalloc but it addition
>> > registers it as root.  When MPS starts scanning, it begins from the
>> > roots.  That means, we are sure that the referenced objects stay alive.
>> > In the case of ambiguous references, the objects are also pinned
>> > (i.e. don't move).
>> > 
>> > That pinning part is not necessary for the scroll-bar case, but
>> > igc_xzalloc_ambig is currently the easiest to use function available in
>> > igc.h.
>> 
>> Thanks, I think I get this.
>
> Btw, did we decide what to do with these chains:
>
>   struct frame
>   {
>   [...]
>   /* List of scroll bars on this frame.
>      Actually, we don't specify exactly what is stored here at all; the
>      scroll bar implementation code can use it to store anything it likes.
>      This field is marked by the garbage collector.  It is here
>      instead of in the `device' structure so that the garbage
>      collector doesn't need to look inside the window-system-dependent
>      structure.  */
>   Lisp_Object scroll_bars;
>   Lisp_Object condemned_scroll_bars;
>
> They are doubly-linked lists via the ->next and ->prev fields in the C
> 'struct scroll_bar', in X as well as w32, see XSCROLL_BAR.  The
> Lisp_Object's above are the last scroll bar on the frame's windows,
> and the rest are reachable via the ->next and ->prev pointers.  Do we
> need to do anything about those ->next and ->prev pointers?

For NS, and I must always add that I don't know NS at all, the
condemned_scroll_bars are fine I think. NS sets a flag in an NS
EmacsScroller object to condemn/redeem it, and doesn't use the frame's
field, says clangd.

The scroll_bars field was something strange. It didn't contain struct
scroll_bars but someting like optimized Lisp_Misc_Ptrs (see
xmint_pointer in lisp.h) which point to EmacsScrollers.

In summary, I came for NS to the conclusion that we should be good with
what we currently have. Famous last words.

Re: MPS: scroll-bars

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Mon, 06 May 2024 17:53:40 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Btw, did we decide what to do with these chains:
> >
> >   struct frame
> >   {
> >   [...]
> >   /* List of scroll bars on this frame.
> >      Actually, we don't specify exactly what is stored here at all; the
> >      scroll bar implementation code can use it to store anything it likes.
> >      This field is marked by the garbage collector.  It is here
> >      instead of in the `device' structure so that the garbage
> >      collector doesn't need to look inside the window-system-dependent
> >      structure.  */
> >   Lisp_Object scroll_bars;
> >   Lisp_Object condemned_scroll_bars;
> >
> > They are doubly-linked lists via the ->next and ->prev fields in the C
> > 'struct scroll_bar', in X as well as w32, see XSCROLL_BAR.  The
> > Lisp_Object's above are the last scroll bar on the frame's windows,
> > and the rest are reachable via the ->next and ->prev pointers.  Do we
> > need to do anything about those ->next and ->prev pointers?
> 
> For NS, and I must always add that I don't know NS at all, the
> condemned_scroll_bars are fine I think. NS sets a flag in an NS
> EmacsScroller object to condemn/redeem it, and doesn't use the frame's
> field, says clangd.
> 
> The scroll_bars field was something strange. It didn't contain struct
> scroll_bars but someting like optimized Lisp_Misc_Ptrs (see
> xmint_pointer in lisp.h) which point to EmacsScrollers.
> 
> In summary, I came for NS to the conclusion that we should be good with
> what we currently have. Famous last words.

Helmut, WDYT about the scroll bars on X (and by extension on w32)?

Re: MPS: scroll-bars

[Helmut Eller]

On Mon, May 06 2024, Eli Zaretskii wrote:

>> > Btw, did we decide what to do with these chains:
>> >
>> >   struct frame
>> >   {
>> >   [...]
>> >   /* List of scroll bars on this frame.
>> >      Actually, we don't specify exactly what is stored here at all; the
>> >      scroll bar implementation code can use it to store anything it likes.
>> >      This field is marked by the garbage collector.  It is here
>> >      instead of in the `device' structure so that the garbage
>> >      collector doesn't need to look inside the window-system-dependent
>> >      structure.  */
>> >   Lisp_Object scroll_bars;
>> >   Lisp_Object condemned_scroll_bars;
>> >
>> > They are doubly-linked lists via the ->next and ->prev fields in the C
>> > 'struct scroll_bar', in X as well as w32, see XSCROLL_BAR.  The
>> > Lisp_Object's above are the last scroll bar on the frame's windows,
>> > and the rest are reachable via the ->next and ->prev pointers.  Do we
>> > need to do anything about those ->next and ->prev pointers?
[...]
> Helmut, WDYT about the scroll bars on X (and by extension on w32)?

On X, the struct scroll_bar is allocated as a PVEC_OTHER pseudovector.
And I think the next/prev links are traced because of that.
xterm.c:mark_xterm doesn't do anything special for scrollbars, so I
think it's all covered.  The only complication so far was that a struct
scroll_bar* was handed to GTK that was not traced; after I turned that
into a root with a pointer to the scroll_bar* it is traced.

Re: MPS: scroll-bars

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Gerd Möllmann <gerd.moellmann@gmail.com>,
>   emacs-devel@gnu.org
> Date: Tue, 07 May 2024 08:07:04 +0200
> 
> On Mon, May 06 2024, Eli Zaretskii wrote:
> 
> >> > Btw, did we decide what to do with these chains:
> >> >
> >> >   struct frame
> >> >   {
> >> >   [...]
> >> >   /* List of scroll bars on this frame.
> >> >      Actually, we don't specify exactly what is stored here at all; the
> >> >      scroll bar implementation code can use it to store anything it likes.
> >> >      This field is marked by the garbage collector.  It is here
> >> >      instead of in the `device' structure so that the garbage
> >> >      collector doesn't need to look inside the window-system-dependent
> >> >      structure.  */
> >> >   Lisp_Object scroll_bars;
> >> >   Lisp_Object condemned_scroll_bars;
> >> >
> >> > They are doubly-linked lists via the ->next and ->prev fields in the C
> >> > 'struct scroll_bar', in X as well as w32, see XSCROLL_BAR.  The
> >> > Lisp_Object's above are the last scroll bar on the frame's windows,
> >> > and the rest are reachable via the ->next and ->prev pointers.  Do we
> >> > need to do anything about those ->next and ->prev pointers?
> [...]
> > Helmut, WDYT about the scroll bars on X (and by extension on w32)?
> 
> On X, the struct scroll_bar is allocated as a PVEC_OTHER pseudovector.

Likewise on w32.

> And I think the next/prev links are traced because of that.
> xterm.c:mark_xterm doesn't do anything special for scrollbars, so I
> think it's all covered.

So any C pointers within a Lisp object allocated via MPS are
automatically traced and fixed when the object is moved?

> The only complication so far was that a struct
> scroll_bar* was handed to GTK that was not traced; after I turned that
> into a root with a pointer to the scroll_bar* it is traced.

Is this the only/best way of making a structure pinned?  If there are
others, what are they?  I wonder if there's a more elegant technique
than to use indirection.  E.g., can't I tell MPS somehow that a Lisp
object allocated as PVEC_OTHER pseudovector is pinned?

See, on w32 the pointer to 'struct scroll_bar' is handed to the UI
thread (via a message with parameters), which uses it to tell the
MS-Windows GUI system how to create the scroll bar (which is just a
special kind of window, as far as MS-Windows is concerned).  So I
think the 'struct scroll_bar' must not be allowed to be moved on w32,
because if it does, the UI thread will dereference invalid pointers.

Re: MPS: scroll-bars

[Helmut Eller]

On Tue, May 07 2024, Eli Zaretskii wrote:

>> And I think the next/prev links are traced because of that.
>> xterm.c:mark_xterm doesn't do anything special for scrollbars, so I
>> think it's all covered.
>
> So any C pointers within a Lisp object allocated via MPS are
> automatically traced and fixed when the object is moved?

Automatically is perhaps an overstatement.  For pseudovectors, the
function fix_vectorlike does most of the work.

>> The only complication so far was that a struct
>> scroll_bar* was handed to GTK that was not traced; after I turned that
>> into a root with a pointer to the scroll_bar* it is traced.
>
> Is this the only/best way of making a structure pinned?  If there are
> others, what are they?  I wonder if there's a more elegant technique
> than to use indirection.  E.g., can't I tell MPS somehow that a Lisp
> object allocated as PVEC_OTHER pseudovector is pinned?

One other way would be to allocate objects in the mark-and-sweep pool.
But we haven't decided yet whether that's worth the trouble.

> See, on w32 the pointer to 'struct scroll_bar' is handed to the UI
> thread (via a message with parameters), which uses it to tell the
> MS-Windows GUI system how to create the scroll bar (which is just a
> special kind of window, as far as MS-Windows is concerned).  So I
> think the 'struct scroll_bar' must not be allowed to be moved on w32,
> because if it does, the UI thread will dereference invalid pointers.

This doesn't sound all that different from the situation I had with GTK.

Re: MPS image cache

[Helmut Eller]

On Fri, May 03 2024, Gerd Möllmann wrote:
> While we scan an object, MPS gives us exclusive access to it. I think we
> want that for these caches, so that scanning does not interfere iwth the
> client modifying the cache?

Access is exclusive to the image_cache struct, but not the buckets, I
think.

Re: MPS image cache

[Gerd Möllmann]

Helmut Eller <eller.helmut@gmail.com> writes:

> On Fri, May 03 2024, Gerd Möllmann wrote:
>> While we scan an object, MPS gives us exclusive access to it. I think we
>> want that for these caches, so that scanning does not interfere iwth the
>> client modifying the cache?
>
> Access is exclusive to the image_cache struct, but not the buckets, I
> think.

Right. Hm.

Re: MPS image cache

[Gerd Möllmann]

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

> Helmut Eller <eller.helmut@gmail.com> writes:
>
>> On Fri, May 03 2024, Gerd Möllmann wrote:
>>> While we scan an object, MPS gives us exclusive access to it. I think we
>>> want that for these caches, so that scanning does not interfere iwth the
>>> client modifying the cache?
>>
>> Access is exclusive to the image_cache struct, but not the buckets, I
>> think.
>
> Right. Hm.

I'm plannign to do the following if nobody stops me:

Both face an image cache are hash tables containing 2 arrays of pointers
to MPS objects (face, image).

I want to introduce a new MPS object type representing such an array of
pointers, IGC_OBJ_PTR_VEC. The igc_header gives us the size of the
array, and being an MPS object, we get exclusive access to its contents.

WDYT

Re: MPS image cache

[Gerd Möllmann]

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

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Helmut Eller <eller.helmut@gmail.com> writes:
>>
>>> On Fri, May 03 2024, Gerd Möllmann wrote:
>>>> While we scan an object, MPS gives us exclusive access to it. I think we
>>>> want that for these caches, so that scanning does not interfere iwth the
>>>> client modifying the cache?
>>>
>>> Access is exclusive to the image_cache struct, but not the buckets, I
>>> think.
>>
>> Right. Hm.
>
> I'm plannign to do the following if nobody stops me:
>
> Both face an image cache are hash tables containing 2 arrays of pointers
> to MPS objects (face, image).
>
> I want to introduce a new MPS object type representing such an array of
> pointers, IGC_OBJ_PTR_VEC. The igc_header gives us the size of the
> array, and being an MPS object, we get exclusive access to its contents.
>
> WDYT

Pushed that, but not yet using it. We can always remove that later...

Re: MPS image cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Emacs Devel <emacs-devel@gnu.org>
> Date: Sun, 05 May 2024 09:02:17 +0200
> 
> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> 
> > I'm plannign to do the following if nobody stops me:
> >
> > Both face an image cache are hash tables containing 2 arrays of pointers
> > to MPS objects (face, image).
> >
> > I want to introduce a new MPS object type representing such an array of
> > pointers, IGC_OBJ_PTR_VEC. The igc_header gives us the size of the
> > array, and being an MPS object, we get exclusive access to its contents.
> >
> > WDYT
> 
> Pushed that, but not yet using it. We can always remove that later...

It breaks the 32-bit build:

  In file included from lisp.h:42,
		   from igc.c:32:
  igc.c: In function 'fix_ptr_vec':
  ../lib/verify.h:213:41: error: static assertion failed: "verify (sizeof *h == sizeof *v)"
    213 | # define _GL_VERIFY(R, DIAGNOSTIC, ...) _Static_assert (R, DIAGNOSTIC)
	|                                         ^~~~~~~~~~~~~~
  ../lib/verify.h:330:20: note: in expansion of macro '_GL_VERIFY'
    330 | # define verify(R) _GL_VERIFY (R, "verify (" #R ")", -)
	|                    ^~~~~~~~~~
  igc.c:107:30: note: in expansion of macro 'verify'
    107 | #define igc_static_assert(x) verify (x)
	|                              ^~~~~~
  igc.c:1085:5: note: in expansion of macro 'igc_static_assert'
   1085 |     igc_static_assert (sizeof *h == sizeof *v);
	|     ^~~~~~~~~~~~~~~~~
  Makefile:453: recipe for target `igc.o' failed

And I truly don't understand how it was supposed to work:

  static mps_res_t
  fix_ptr_vec (mps_ss_t ss, void *client)
  {
    MPS_SCAN_BEGIN (ss)
    {
      struct igc_header *h = client_to_base (client);
      void **v = client;
      igc_static_assert (sizeof *h == sizeof *v);  <<<<<<<<<<<<<<<<<

Here '*v' is a pointer, whereas '*h' is 'struct igc_header', which is
a structure whose size is larger than 32 bits:

  enum
  {
    IGC_TYPE_BITS = 5,
    IGC_PVEC_BITS = 6,
    IGC_HASH_BITS = 21,
    IGC_SIZE_BITS = 32
  };

  struct igc_header
  {
    enum igc_obj_type obj_type : IGC_TYPE_BITS;
    enum pvec_type pvec_type : IGC_PVEC_BITS;
    mps_word_t hash : IGC_HASH_BITS;
    mps_word_t nwords : IGC_SIZE_BITS;
  };

So this can only work on a 64-bit architecture, and even then only if
the compiler packs the structure (which is entirely not guaranteed
AFAIK).  Or what am I missing here?

And wasn't it supposed to say

      void **v = &client;

instead?

IMNSHO, this kind of code really needs comments to explain what it
does and why.

Re: MPS image cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> Pushed that, but not yet using it. We can always remove that later...
>
> It breaks the 32-bit build:
>
>   In file included from lisp.h:42,
> 		   from igc.c:32:
>   igc.c: In function 'fix_ptr_vec':
>   ../lib/verify.h:213:41: error: static assertion failed: "verify (sizeof *h == sizeof *v)"
>     213 | # define _GL_VERIFY(R, DIAGNOSTIC, ...) _Static_assert (R, DIAGNOSTIC)
> 	|                                         ^~~~~~~~~~~~~~
>   ../lib/verify.h:330:20: note: in expansion of macro '_GL_VERIFY'
>     330 | # define verify(R) _GL_VERIFY (R, "verify (" #R ")", -)
> 	|                    ^~~~~~~~~~
>   igc.c:107:30: note: in expansion of macro 'verify'
>     107 | #define igc_static_assert(x) verify (x)
> 	|                              ^~~~~~
>   igc.c:1085:5: note: in expansion of macro 'igc_static_assert'
>    1085 |     igc_static_assert (sizeof *h == sizeof *v);
> 	|     ^~~~~~~~~~~~~~~~~
>   Makefile:453: recipe for target `igc.o' failed
>
> And I truly don't understand how it was supposed to work:
>
>   static mps_res_t
>   fix_ptr_vec (mps_ss_t ss, void *client)
>   {
>     MPS_SCAN_BEGIN (ss)
>     {
>       struct igc_header *h = client_to_base (client);
>       void **v = client;
>       igc_static_assert (sizeof *h == sizeof *v);  <<<<<<<<<<<<<<<<<
>
> Here '*v' is a pointer, whereas '*h' is 'struct igc_header', which is
> a structure whose size is larger than 32 bits:

I guess this comes from igc_header being 1 word on 64 bit, and 2 on
32-bit. Thing is we need to know how large the vector is to scan it.
We have the info the igc_header::nwords.

>   enum
>   {
>     IGC_TYPE_BITS = 5,
>     IGC_PVEC_BITS = 6,
>     IGC_HASH_BITS = 21,
>     IGC_SIZE_BITS = 32
>   };
>
>   struct igc_header
>   {
>     enum igc_obj_type obj_type : IGC_TYPE_BITS;
>     enum pvec_type pvec_type : IGC_PVEC_BITS;
>     mps_word_t hash : IGC_HASH_BITS;
>     mps_word_t nwords : IGC_SIZE_BITS;
>   };
>
> So this can only work on a 64-bit architecture, and even then only if
> the compiler packs the structure (which is entirely not guaranteed
> AFAIK).  Or what am I missing here?

Apparently it only works as is on 64-bits and so on. What do you expect
me to do? A thing I promise not to do is to develop and check on any
platform except mine.

> And wasn't it supposed to say
>
>       void **v = &client;
>
> instead?

The address of a local variable? I think it's correct as is.

Re: MPS image cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Sun, 05 May 2024 11:31:23 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >   static mps_res_t
> >   fix_ptr_vec (mps_ss_t ss, void *client)
> >   {
> >     MPS_SCAN_BEGIN (ss)
> >     {
> >       struct igc_header *h = client_to_base (client);
> >       void **v = client;
> >       igc_static_assert (sizeof *h == sizeof *v);  <<<<<<<<<<<<<<<<<
> >
> > Here '*v' is a pointer, whereas '*h' is 'struct igc_header', which is
> > a structure whose size is larger than 32 bits:
> 
> I guess this comes from igc_header being 1 word on 64 bit, and 2 on
> 32-bit. Thing is we need to know how large the vector is to scan it.
> We have the info the igc_header::nwords.
> 
> >   enum
> >   {
> >     IGC_TYPE_BITS = 5,
> >     IGC_PVEC_BITS = 6,
> >     IGC_HASH_BITS = 21,
> >     IGC_SIZE_BITS = 32
> >   };
> >
> >   struct igc_header
> >   {
> >     enum igc_obj_type obj_type : IGC_TYPE_BITS;
> >     enum pvec_type pvec_type : IGC_PVEC_BITS;
> >     mps_word_t hash : IGC_HASH_BITS;
> >     mps_word_t nwords : IGC_SIZE_BITS;
> >   };
> >
> > So this can only work on a 64-bit architecture, and even then only if
> > the compiler packs the structure (which is entirely not guaranteed
> > AFAIK).  Or what am I missing here?
> 
> Apparently it only works as is on 64-bits and so on.

How can you be sure the compiler packs the struct fields, even on a
64-bit platform?  If the fields are not packed, the struct could take
more than 1 word on a 64-bit platform as well.  Compilers are known to
align struct members for faster access.

> What do you expect me to do?

Can you explain what the code is supposed to do, and why you need that
assertion?  "we need to know how large the vector is to scan it"
didn't explain it enough for me to understand.

> A thing I promise not to do is to develop and check on any platform
> except mine.

I thought I was helping to test on a platform where you don't, but I
need help in understanding the code, so I could think about possible
ways of fixing it.  We are not abandoning 32-bit platforms any time
soon, so a solution for this will be needed.

TIA

Re: MPS image cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> Apparently it only works as is on 64-bits and so on.
>
> How can you be sure the compiler packs the struct fields, even on a
> 64-bit platform?  If the fields are not packed, the struct could take
> more than 1 word on a 64-bit platform as well.  Compilers are known to
> align struct members for faster access.

It is on my platform.

>> What do you expect me to do?
>
> Can you explain what the code is supposed to do, and why you need that
> assertion?  

I made the compiler my check my assumption at compile time. I find that
pretty cool of me.

> "we need to know how large the vector is to scan it" didn't explain it
> enough for me to understand.

We allocate a vector of N pointers. The N is found in the header when we
scan it. It's the size of the MPS object we scan minus the header
divided by how large a pointer is.
>
>> A thing I promise not to do is to develop and check on any platform
>> except mine.
>
> I thought I was helping to test on a platform where you don't, but I
> need help in understanding the code, so I could think about possible
> ways of fixing it.  We are not abandoning 32-bit platforms any time
> soon, so a solution for this will be needed.

Yeah, I know, sorry. My sciatica is killing me, and I'm probably a bit
hard to handle these days :-(

Re: MPS image cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Sun, 05 May 2024 12:36:21 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Apparently it only works as is on 64-bits and so on.
> >
> > How can you be sure the compiler packs the struct fields, even on a
> > 64-bit platform?  If the fields are not packed, the struct could take
> > more than 1 word on a 64-bit platform as well.  Compilers are known to
> > align struct members for faster access.
> 
> It is on my platform.

IME, this cannot be relied upon, unless we explicitly tell the
compiler to pack (using some pragma or somesuch).

> >> What do you expect me to do?
> >
> > Can you explain what the code is supposed to do, and why you need that
> > assertion?  
> 
> I made the compiler my check my assumption at compile time. I find that
> pretty cool of me.

That's not what I meant.  I meant to ask why we need the struct to be
of the same size as a pointer?

> > "we need to know how large the vector is to scan it" didn't explain it
> > enough for me to understand.
> 
> We allocate a vector of N pointers. The N is found in the header when we
> scan it. It's the size of the MPS object we scan minus the header
> divided by how large a pointer is.

OK, but why does the struct have to fit in a single word?

Re: MPS image cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> It is on my platform.
>
> IME, this cannot be relied upon, unless we explicitly tell the
> compiler to pack (using some pragma or somesuch).

And the static_assert will fire, right. You're telling me nothing new,
BTW. I'm pretty well acquainged with all this.

>
>> >> What do you expect me to do?
>> >
>> > Can you explain what the code is supposed to do, and why you need that
>> > assertion?  
>> 
>> I made the compiler my check my assumption at compile time. I find that
>> pretty cool of me.
>
> That's not what I meant.  I meant to ask why we need the struct to be
> of the same size as a pointer?
>
>> > "we need to know how large the vector is to scan it" didn't explain it
>> > enough for me to understand.
>> 
>> We allocate a vector of N pointers. The N is found in the header when we
>> scan it. It's the size of the MPS object we scan minus the header
>> divided by how large a pointer is.
>
> OK, but why does the struct have to fit in a single word?

It doesn't, it the other way around. I'm using nwords - 1 for the size
of the vector, and the static_assert is intended to fire if nwords - 1
is not right.

Re: MPS image cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Sun, 05 May 2024 14:55:04 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> It is on my platform.
> >
> > IME, this cannot be relied upon, unless we explicitly tell the
> > compiler to pack (using some pragma or somesuch).
> 
> And the static_assert will fire, right. You're telling me nothing new,
> BTW. I'm pretty well acquainged with all this.

I imagined, it just took me by surprise that you would nonetheless
write code that could so easily fail to compile.

> >> We allocate a vector of N pointers. The N is found in the header when we
> >> scan it. It's the size of the MPS object we scan minus the header
> >> divided by how large a pointer is.
> >
> > OK, but why does the struct have to fit in a single word?
> 
> It doesn't, it the other way around. I'm using nwords - 1 for the size
> of the vector, and the static_assert is intended to fire if nwords - 1
> is not right.

OK, then my question now is "why does it have to be nwords-1?"

Re: MPS image cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
>> Date: Sun, 05 May 2024 14:55:04 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> It is on my platform.
>> >
>> > IME, this cannot be relied upon, unless we explicitly tell the
>> > compiler to pack (using some pragma or somesuch).
>> 
>> And the static_assert will fire, right. You're telling me nothing new,
>> BTW. I'm pretty well acquainged with all this.
>
> I imagined, it just took me by surprise that you would nonetheless
> write code that could so easily fail to compile.

I think it's a matter of personal priorities. It guess it can't hurt to
say explicity what these are.

1. Make progress fast, so that I can assess for myself if it's worth
continuing. With an Emacs up time of ca. 8h now, I tend to think so.
When it would reach say 5 days or so, we'd be in the realm of what
master survives for me on macOS.

2. Make it work for me in my personal fork (CL packages) which is macOS
only anyway, and is basically for me alone only.

3. GNU. And I'm neither a maintainer of anything, nor want to maintain
anything in the GNU sense.

>> >> We allocate a vector of N pointers. The N is found in the header when we
>> >> scan it. It's the size of the MPS object we scan minus the header
>> >> divided by how large a pointer is.
>> >
>> > OK, but why does the struct have to fit in a single word?
>> 
>> It doesn't, it the other way around. I'm using nwords - 1 for the size
>> of the vector, and the static_assert is intended to fire if nwords - 1
>> is not right.
>
> OK, then my question now is "why does it have to be nwords-1?"

I suspect that I'll answer something you don't ask again: We allocate a
vector of N elements. We don't store N directly somewhere, but we have
an igc_header from which we can compute N. So I compute it as nwords -
1. If that's not right, I static_assert, and leave it to the interested
party to do something.

Re: MPS image cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Sun, 05 May 2024 16:32:02 +0200
> 
> I think it's a matter of personal priorities. It guess it can't hurt to
> say explicity what these are.
> 
> 1. Make progress fast, so that I can assess for myself if it's worth
> continuing. With an Emacs up time of ca. 8h now, I tend to think so.
> When it would reach say 5 days or so, we'd be in the realm of what
> master survives for me on macOS.
> 
> 2. Make it work for me in my personal fork (CL packages) which is macOS
> only anyway, and is basically for me alone only.
> 
> 3. GNU. And I'm neither a maintainer of anything, nor want to maintain
> anything in the GNU sense.

And my impression was that we are trying to advance on 3 most
important Emacs platforms in parallel, with the purpose of adding this
to Emacs at some point.  Apologies for my misunderstanding.

> >> >> We allocate a vector of N pointers. The N is found in the header when we
> >> >> scan it. It's the size of the MPS object we scan minus the header
> >> >> divided by how large a pointer is.
> >> >
> >> > OK, but why does the struct have to fit in a single word?
> >> 
> >> It doesn't, it the other way around. I'm using nwords - 1 for the size
> >> of the vector, and the static_assert is intended to fire if nwords - 1
> >> is not right.
> >
> > OK, then my question now is "why does it have to be nwords-1?"
> 
> I suspect that I'll answer something you don't ask again: We allocate a
> vector of N elements. We don't store N directly somewhere, but we have
> an igc_header from which we can compute N. So I compute it as nwords -
> 1. If that's not right, I static_assert, and leave it to the interested
> party to do something.

I fixed the code according to my understanding, please take a look and
tell if I missed something (since the new function does not seem to be
used yet for the two caches you mentioned, it is hard to know whether
I violated some assumptions).

Thanks.

Re: MPS image cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> And my impression was that we are trying to advance on 3 most
> important Emacs platforms in parallel, with the purpose of adding this
> to Emacs at some point.  Apologies for my misunderstanding.

My apologies for not helping with the advance!

Re: MPS image cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> I fixed the code according to my understanding, please take a look and
> tell if I missed something (since the new function does not seem to be
> used yet for the two caches you mentioned, it is hard to know whether
> I violated some assumptions).

This hunk is wrong:

@@ -2978,7 +2977,8 @@ igc_make_face_cache (void)
 void *
 igc_make_ptr_vec (size_t n)
 {
-  return alloc (n * sizeof (void *), IGC_OBJ_PTR_VEC, PVEC_FREE);
+  return alloc (sizeof (struct igc_header) + (n - 1) * sizeof (void *),
+		IGC_OBJ_PTR_VEC, PVEC_FREE);
 }
 
what you removed was correct because alloc takes the size in bytes to
allocate from the POV of the client.

Alloc computes the size needed from MPS

  size = obj_size (size);

which includes the header among other things.

Re: MPS image cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Sun, 05 May 2024 19:45:03 +0200
> 
> @@ -2978,7 +2977,8 @@ igc_make_face_cache (void)
>  void *
>  igc_make_ptr_vec (size_t n)
>  {
> -  return alloc (n * sizeof (void *), IGC_OBJ_PTR_VEC, PVEC_FREE);
> +  return alloc (sizeof (struct igc_header) + (n - 1) * sizeof (void *),
> +		IGC_OBJ_PTR_VEC, PVEC_FREE);
>  }
>  
> what you removed was correct because alloc takes the size in bytes to
> allocate from the POV of the client.
> 
> Alloc computes the size needed from MPS
> 
>   size = obj_size (size);
> 
> which includes the header among other things.

Thanks, I tried to fix that now.

Re: MPS image cache

[Eli Zaretskii]

> > Alloc computes the size needed from MPS
> > 
> >   size = obj_size (size);
> > 
> > which includes the header among other things.
> 
> Thanks, I tried to fix that now.

Or maybe it's preferable to use

      size_t imax = h->nwords - to_words (sizeof (struct igc_header));

instead (assuming that mps_word_t and 'void *' must be of the same
size)?

Re: MPS image cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> > Alloc computes the size needed from MPS
>> > 
>> >   size = obj_size (size);
>> > 
>> > which includes the header among other things.
>> 
>> Thanks, I tried to fix that now.
>
> Or maybe it's preferable to use
>
>       size_t imax = h->nwords - to_words (sizeof (struct igc_header));
>
> instead (assuming that mps_word_t and 'void *' must be of the same
> size)?

Yes, I think that looks best.

Re: MPS image cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Sun, 05 May 2024 20:35:48 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Or maybe it's preferable to use
> >
> >       size_t imax = h->nwords - to_words (sizeof (struct igc_header));
> >
> > instead (assuming that mps_word_t and 'void *' must be of the same
> > size)?
> 
> Yes, I think that looks best.

Done.

Re: MPS image cache

[Gerd Möllmann]

Thanks
Sent from my iPhone

> On 5. May 2024, at 21:19, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> 
>> 
>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
>> Date: Sun, 05 May 2024 20:35:48 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>>> Or maybe it's preferable to use
>>> 
>>>      size_t imax = h->nwords - to_words (sizeof (struct igc_header));
>>> 
>>> instead (assuming that mps_word_t and 'void *' must be of the same
>>> size)?
>> 
>> Yes, I think that looks best.
> 
> Done.

Re: MPS image cache

[Helmut Eller]

On Sun, May 05 2024, Gerd Möllmann wrote:

> I'm plannign to do the following if nobody stops me:
>
> Both face an image cache are hash tables containing 2 arrays of pointers
> to MPS objects (face, image).
>
> I want to introduce a new MPS object type representing such an array of
> pointers, IGC_OBJ_PTR_VEC. The igc_header gives us the size of the
> array, and being an MPS object, we get exclusive access to its contents.
>
> WDYT

Would something like this also be needed for glyph matrices?  Or in
general, can we scan anything other than the block that dflt_scan was
called with?  Is these related to "remote references" in the MPS manual,
which I don't quite understand what they mean.

Re: MPS image cache

[Gerd Möllmann]

Helmut Eller <eller.helmut@gmail.com> writes:

> On Sun, May 05 2024, Gerd Möllmann wrote:
>
>> I'm plannign to do the following if nobody stops me:
>>
>> Both face an image cache are hash tables containing 2 arrays of pointers
>> to MPS objects (face, image).
>>
>> I want to introduce a new MPS object type representing such an array of
>> pointers, IGC_OBJ_PTR_VEC. The igc_header gives us the size of the
>> array, and being an MPS object, we get exclusive access to its contents.
>>
>> WDYT
>
> Would something like this also be needed for glyph matrices?  

Probably. Either we make the matrices roots, or objects, I guess. For
correctness. We can't easily realloc when they are objects, but I don't
think that's a big problem.

> Or in general, can we scan anything other than the block that
> dflt_scan was called with? 

Let's say we shouldn't :-). MPS lets us scan memory it doesn't manage,
but with multi-threading it's not really safe.

> Is these related to "remote references" in the MPS manual, which I
> don't quite understand what they mean.

That's this

  A reference that logically belongs to a formatted object and so must be
  fixed when the object is scanned, but which is not stored within the
  block containing the object. (For example, in an auxiliary table of some
  sort.)

  The MPS does not generally support remote references because those
  references may be protected and so if scan method attempts to fix them
  this will hit a barrier (1) and cause a re-entrant call to the MPS.

My interpretation of this is that we may only fix12 references withing
in an object that is currently scanned (is in the address range for
which MPS called us). We may not extend the scan to other MPS objects.

Let's say we are scanning a symbol S. The reference to S->name can/must
be scanned, but S->name->intervals or something is taboo because S->name
may be behind a barrier.

Made sense to me.

Re: MPS image cache

[Helmut Eller]

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

On Sun, May 05 2024, Gerd Möllmann wrote:

>> Would something like this also be needed for glyph matrices?  
>
> Probably. Either we make the matrices roots, or objects, I guess. For
> correctness. We can't easily realloc when they are objects, but I don't
> think that's a big problem.
>
>> Or in general, can we scan anything other than the block that
>> dflt_scan was called with? 
>
> Let's say we shouldn't :-). MPS lets us scan memory it doesn't manage,
> but with multi-threading it's not really safe.

So fix_glyph_matrix currently doesn't things exactly by the book.  Hm.

Anyway, I have a patch that prevents my Emacs from sporadically crashing
at startup.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Zero-out-terminal-keyboard_coding.patch --]
[-- Type: text/x-diff, Size: 1342 bytes --]

From eda36223e8062337cd40c97471749c519fcc7b9b Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Mon, 6 May 2024 10:07:14 +0200
Subject: [PATCH] Zero out terminal->keyboard_coding

This avoids hard to reproduce crashes in fix_coding.

* src/terminal.c (create_terminal): Use xzalloc instead of
xmalloc.
---
 src/terminal.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/terminal.c b/src/terminal.c
index 23a5582d4d9..33f8623c214 100644
--- a/src/terminal.c
+++ b/src/terminal.c
@@ -24,6 +24,9 @@
 #include "termchar.h"
 #include "termhooks.h"
 #include "keyboard.h"
+#ifdef HAVE_MPS
+#include "igc.h"
+#endif
 
 #if HAVE_STRUCT_UNIPAIR_UNICODE
 # include <errno.h>
@@ -281,8 +284,13 @@ create_terminal (enum output_method type, struct redisplay_interface *rif)
   terminal->rif = rif;
   terminal->id = next_terminal_id++;
 
+#ifdef HAVE_MPS
+  terminal->keyboard_coding = xzalloc (sizeof (struct coding_system));
+  terminal->terminal_coding = xzalloc (sizeof (struct coding_system));
+#else
   terminal->keyboard_coding = xmalloc (sizeof (struct coding_system));
   terminal->terminal_coding = xmalloc (sizeof (struct coding_system));
+#endif
 
   /* If default coding systems for the terminal and the keyboard are
      already defined, use them in preference to the defaults.  This is
-- 
2.39.2

Re: MPS image cache

[Gerd Möllmann]

Helmut Eller <eller.helmut@gmail.com> writes:

> Anyway, I have a patch that prevents my Emacs from sporadically crashing
> at startup.

Pushed. Thanks Helmut!

Re: MPS image cache

[Helmut Eller]

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

>> There is a bit of an issue with proper initialization of the structs
>> pointed to by those roots.  E.g. x_term_init uses xzalloc, puts pointers
>> into the struct, and only then stores the pointer to the struct in
>> x_display_list.  So what should be the root, the struct (registred
>> immediately after the call to xzalloc) or the global variable
>> x_display_list?
>
> Hm.

For my case, the troublesome field was xi_device_t.name.  I decided to
make x_display_info and xi_device_t amigous roots, because there is
usually only one x_display_info and dozen of xi_device_t.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-x_display_info-and-xi_device_t-amig-roots.patch --]
[-- Type: text/x-diff, Size: 4558 bytes --]

From 497b5c900e1a4746c214af7be6242d0bc456cdc7 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Fri, 3 May 2024 16:47:51 +0200
Subject: [PATCH] Make x_display_info and xi_device_t amig roots

* src/xterm.c (x_cache_xi_devices, xi_disable_devices): Use
  igc_xzalloc_ambig for xi_disable_t.
  (handle_one_xevent): For the XI_HierarchyChanged event
  igc_realloc_ambig if needed.
  (x_term_init): Use igc_xzalloc_ambig for dpyinfo.
* src/igc.h (igc_realloc_ambig): New.
* src/igc.c (igc_realloc_ambig): Not implemented yet;
  scetch of an implementation as comment.
---
 src/igc.c   | 24 ++++++++++++++++++++++--
 src/igc.h   |  1 +
 src/xterm.c | 29 +++++++++++++++++++++++++----
 3 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/src/igc.c b/src/igc.c
index 648f2bbc44d..6d553bc316d 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -1389,10 +1389,10 @@ fix_frame (mps_ss_t ss, struct frame *f)
 	struct font **font_ptr = &FRAME_FONT (f);
 	if (*font_ptr)
 	  IGC_FIX12_RAW (ss, font_ptr);
-	Lisp_Object *nle = &FRAME_DISPLAY_INFO(f)->name_list_element;
+	Lisp_Object *nle = &FRAME_DISPLAY_INFO (f)->name_list_element;
 	IGC_FIX12_OBJ (ss, nle);
       }
-#endif
+#endif // HAVE_WINDOW_SYSTEM
   }
   MPS_SCAN_END (ss);
   return MPS_RES_OK;
@@ -2157,6 +2157,26 @@ igc_xzalloc_ambig (size_t size)
   return p;
 }
 
+void *
+igc_realloc_ambig (void *block, size_t size)
+{
+#if 0 // non tested code:
+  struct igc_root_list *r = root_find (block);
+  igc_assert (r != NULL);
+  destroy_root (&r);
+
+  /* Can't make a root that has zero length. Want one to be able to
+     detect calling igc_free on something not having a root. */
+  size_t new_size = (size == 0 ? IGC_ALIGN_DFLT : size);
+  void *p = xrealloc (block, new_size);
+  void *end = (char *)p + new_size;
+  root_create_ambig (global_igc, p, end);
+  return p;
+#endif
+  emacs_abort ();
+}
+
+
 void
 igc_xfree (void *p)
 {
diff --git a/src/igc.h b/src/igc.h
index 84e4ff8f13a..b1d09700a78 100644
--- a/src/igc.h
+++ b/src/igc.h
@@ -51,6 +51,7 @@ #define EMACS_IGC_H
 
 struct Lisp_Buffer_Local_Value *igc_alloc_blv (void);
 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,
diff --git a/src/xterm.c b/src/xterm.c
index 36f83a33244..c47c7b0534d 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -787,6 +787,9 @@ #define XtNinitialState "initialState"
 #ifdef HAVE_XKB
 #include <X11/XKBlib.h>
 #endif
+#ifdef HAVE_MPS
+#include "igc.h"
+#endif
 
 /* Although X11/Xlib.h commonly defines the types XErrorHandler and
    XIOErrorHandler, they are not in the Xlib spec, so for portability
@@ -5744,7 +5747,11 @@ x_cache_xi_devices (struct x_display_info *dpyinfo)
       return;
     }
 
+#ifdef HAVE_MPS
+  dpyinfo->devices = igc_xzalloc_ambig (sizeof *dpyinfo->devices * ndevices);
+#else
   dpyinfo->devices = xzalloc (sizeof *dpyinfo->devices * ndevices);
+#endif
 
   for (i = 0; i < ndevices; ++i)
     {
@@ -13774,7 +13781,11 @@ xi_disable_devices (struct x_display_info *dpyinfo,
     return;
 
   ndevices = 0;
+#ifdef HAVE_MPS
+  devices = igc_xzalloc_ambig (sizeof *devices * dpyinfo->num_devices);
+#else
   devices = xzalloc (sizeof *devices * dpyinfo->num_devices);
+#endif
 
   /* Loop through every device currently in DPYINFO, and copy it to
      DEVICES if it is not in TO_DISABLE.  Note that this function
@@ -24600,10 +24611,15 @@ handle_one_xevent (struct x_display_info *dpyinfo,
 
 			  if (info && info->enabled)
 			    {
-			      dpyinfo->devices
-				= xrealloc (dpyinfo->devices,
-					    (sizeof *dpyinfo->devices
-					     * ++dpyinfo->num_devices));
+			      size_t new_size = (sizeof *dpyinfo->devices
+						 * ++dpyinfo->num_devices);
+#ifdef HAVE_MPS
+			      dpyinfo->devices =
+				igc_realloc_ambig (dpyinfo->devices, new_size);
+#else
+			      dpyinfo->devices =
+				xrealloc (dpyinfo->devices, new_size);
+#endif
 			      memset (dpyinfo->devices + dpyinfo->num_devices - 1,
 				      0, sizeof *dpyinfo->devices);
 			      device = &dpyinfo->devices[dpyinfo->num_devices - 1];
@@ -30676,7 +30692,12 @@ #define NUM_ARGV 10
 
   /* We have definitely succeeded.  Record the new connection.  */
 
+#ifdef HAVE_MPS
+  dpyinfo = igc_xzalloc_ambig (sizeof *dpyinfo);
+#else
   dpyinfo = xzalloc (sizeof *dpyinfo);
+#endif
+
   terminal = x_create_terminal (dpyinfo);
 
   if (!NILP (Vx_detect_server_trust))
-- 
2.39.2

Re: MPS image cache

[Gerd Möllmann]

Helmut Eller <eller.helmut@gmail.com> writes:

>>> There is a bit of an issue with proper initialization of the structs
>>> pointed to by those roots.  E.g. x_term_init uses xzalloc, puts pointers
>>> into the struct, and only then stores the pointer to the struct in
>>> x_display_list.  So what should be the root, the struct (registred
>>> immediately after the call to xzalloc) or the global variable
>>> x_display_list?
>>
>> Hm.
>
> For my case, the troublesome field was xi_device_t.name.  I decided to
> make x_display_info and xi_device_t amigous roots, because there is
> usually only one x_display_info and dozen of xi_device_t.

Pushed. Sorry for the delay I somehow overlooked your patch.

(And the amigos are the reason I write "ambig" :-).)

Re: MPS image cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Emacs Devel <emacs-devel@gnu.org>
> Date: Fri, 03 May 2024 10:53:50 +0200
> 
> I think I have one: struct terminal's image_cache is not traced. AFAICS,
> the cache is malloc'd and contains struct image which is an MPS object.
> 
> WDYT?

Is this related to Vterminal_frame, or is this a separate issue?

Re: MPS image cache

[Gerd Möllmann]

Not related to terminal frame. It’s from running emacs interactively. 

Sent from my iPhone

> On 3. May 2024, at 13:05, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> 
>> 
>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: Emacs Devel <emacs-devel@gnu.org>
>> Date: Fri, 03 May 2024 10:53:50 +0200
>> 
>> I think I have one: struct terminal's image_cache is not traced. AFAICS,
>> the cache is malloc'd and contains struct image which is an MPS object.
>> 
>> WDYT?
> 
> Is this related to Vterminal_frame, or is this a separate issue?