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.