Re: Increase FD_SETSIZE in w32.h to allow more than 32 (actually 29) subprocesses

[Eli Zaretskii]

> From: "Yue Yi" <include_yy@qq.com>
> Cc: "emacs-devel" <emacs-devel@gnu.org>
> Date: Fri, 24 Jan 2025 03:51:59 +0800
> 
> > Date: Tue, 21 Jan 2025 18:41:11 +0200
> > From: Eli Zaretskii 
> > To: "Yue Yi" 
> > Cc: emacs-devel@gnu.org
> 
> I have completed an initial version. Please see the attached file.I
> will add necessary comments and some error handling code.

Thanks.  Please add comments, as various details of the code are hard
to understand and think about without them.

I've added Cecilio to the discussion, in the hope that he could also
review the patch and comment on it.

> The overall working principle of the code can be simply described as
> creating multiple threads that concurrently call
> WaitForMultipleObjects to wait for more than 64 objects. When a result
> occurs or a timeout happens, the main thread notifies the other
> threads to terminate via an event. Due to the quantity issue, some
> constant macros had to be redefined.

I understand the general idea, but not its finer details.  Adding
comments to explain the details will be very useful.

> > In case you didn't know, there's a large comment starting around line
> > 890 of the file w32proc.c in the Emacs source tree, which provides an
> > overview of how sub-processes are supported in Emacs on MS-Windows.
> 
> Please tell me how unexpected errors are generally handled in Emacs. I
> need to add checks in my code for certain calls that shouldn't fail,
> such as creating threads or events.

Failure to create events or threads that are needed to support
waiting for sub-processes in sys_select should cause sys_select to set
errno to EINTR, as we do currently if waiting yields WAIT_FAILED.

> > > > Starting from the code of the person who submitted bug#71628 is
> > > > possible, but that person will still need to sign the copyright
> > > > assignment, as he will be one of the authors of the code.  Is that
> > > > possible?
> > >
> > > I will notify him and ask him to reply to this list.
> > >
> > > Of course, if he disagrees (which is almost impossible), I will
> > > consider implementing it myself without relying on his code :p
> 
> > Thanks.  In that case, it is best if someone else reads his code and
> > describes the main ideas, and you then write the code based on that
> > description.
> 
> He told me that he found the FSF's reply in the spam folder of his
> email, so I believe he will submit the signed document soon.

Thanks, that's good news.

Some specific comments below.

>  /* MSVC runtime library has limit of 64 descriptors by default */
> -#define FD_SETSIZE  64
> +#define FD_SETSIZE (MAXIMUM_WAIT_OBJECTS * MAXIMUM_WAIT_OBJECTS / 2)

What is the reason for dividing by 2?  A comment should explain that.

> +#define WAIT_TIMEOUT_2 (WAIT_TIMEOUT | 0x4000)
> +#define WAIT_ABANDONED_0_2 (WAIT_ABANDONED_0 | 0x4000)
> +#define WAIT_GROUP_SIZE (MAXIMUM_WAIT_OBJECTS - 1)
> +#define MAXIMUM_WAIT_OBJECTS_2 FD_SETSIZE

I'd prefer to define our own macros whose names do not resemble the
ones MS uses.  This is to avoid confusion, so people don't try to look
up MS documentation or Windows API headers for these symbols.

Also, the above assumes that the values returned by
WaitForMultipleObjects never exceed 0x3fff.  But note that WAIT_FAILED
violates this assumption, so I don't think we can safely use this
trick.  We need some safer solution.

> +typedef struct {
> +  HANDLE* lpHandles;
> +  int nCount;
> +  BOOL bWaitAll;
> +  DWORD dwMilliseconds;
> +  int nIndex;
> +} WaitForMultipleObjectsParams;

Please add comments describing each member of the struct.

The bWaitAll member seems to be unnecessary: we always set it to FALSE
when calling WaitForMultipleObjects end MsgWaitForMultipleObjects.

Also, our conventions are not to use MS-style camel-style symbol names
for symbols we introduce.  So I would suggest to rename the above to
something like wait_objects_params.  And similarly for other
structures and functions in the patch.

> +typedef struct
> +{
> +  HANDLE *hObjects;
> +  WaitForMultipleObjectsParams *pParams;
> +  HANDLE *pHandles;
> +  HANDLE hEndEvent;
> +  int nObject;
> +  int nThread;
> +  int nHandle;
> +} WaitForMultipleObjectsInfo;

Same here: please add comments describing the members.

> +static void
> +InitializeWaitForMultipleObjectsInfo (WaitForMultipleObjectsInfo *p,
> +                                   DWORD nCount,
> +                                   CONST HANDLE *lpHandles,
> +                                   BOOL bWaitAll,
> +                                   DWORD dwMilliseconds)
> +{
> +  p->nThread = 1 + (nCount - 1 - MAXIMUM_WAIT_OBJECTS / 2)
> +    / WAIT_GROUP_SIZE;

Please add here comments explaining the various constants you use,
like 1, 2, etc.

> +  p->hObjects = xmalloc (sizeof (HANDLE) * p->nObject);
> +  if (p->nObject != p->nThread)
> +    memcpy (p->hObjects + p->nThread,
> +         lpHandles + p->nThread * WAIT_GROUP_SIZE,
> +         sizeof (HANDLE) * (nCount - p->nThread * WAIT_GROUP_SIZE));
> +  p->pParams = xmalloc (sizeof (WaitForMultipleObjectsParams) * p->nThread);
> +  p->pHandles = xmalloc (sizeof (HANDLE) * p->nThread * 
> MAXIMUM_WAIT_OBJECTS);

Since the maximum size of these is known in advance, and fixed by the
implementation, I think it would be better to avoid the constant
allocation and deallocation of these arrays, and instead have static
arrays of their maximum size.  In normal Emacs usage, sys_select is
called quite often, and each call will need to xmalloc/xfree these
variables.  Their maximum size seems to take less than 10KB (am I
right?), which is a small price to pay for avoiding constant heap
allocations.

> +  p->hEndEvent = CreateEvent (NULL, TRUE, FALSE, NULL);

I wonder if it would be better to give the event a non-NULL name which
includes the number of processes.  This could help in debugging, I
think.

> +      p->hObjects[i] = CreateThread (NULL, 0, WaitForMultipleObjectsWrapped,
> +                                  p->pParams + i, 0, NULL);

The 2nd and 5th arguments zero might cause trouble, at least in the
32-bit builds of Emacs.  See the comments around line 1100 of
w32proc.c, which explain why and how to request smaller stack size for
the helper threads.

> +static void
> +CleanupWaitForMultipleObjectsInfo (WaitForMultipleObjectsInfo *p)
> +{
> +  SetEvent (p->hEndEvent);
> +  WaitForMultipleObjects(p->nThread, p->hObjects, TRUE, INFINITE);

Instead of waiting forever for the threads to exit, wouldn't it be
better to forcibly terminate them?  When the cleanup function is
called, we know that at least one thread exited, so we could terminate
the rest.  Or at least do that after waiting some reasonable amount of
time, like 100 msec, for them to exit due to SetEvent.  I'm worried
that we might become stuck in this WaitForMultipleObjects forever if
some thread fails to exit for some reason.

> +static DWORD
> +WaitForMultipleObjectsThreaded (DWORD nCount,
> +                             HANDLE *lpHandles,
> +                             BOOL bWaitAll,
> +                             DWORD dwMilliseconds)
> +{
> +  if (nCount <= MAXIMUM_WAIT_OBJECTS)
> +    {
> +      DWORD result = WaitForMultipleObjects (nCount, lpHandles, bWaitAll,
> +                                          dwMilliseconds);
> +      if (result == WAIT_TIMEOUT)
> +     result = WAIT_TIMEOUT_2;
> +      else if (result >= WAIT_ABANDONED_0
> +            && result < WAIT_ABANDONED_0 + nCount)
> +     result += WAIT_ABANDONED_0_2 - WAIT_ABANDONED_0;
> +      return result;
> +    }
> +  WaitForMultipleObjectsInfo info;
> +  InitializeWaitForMultipleObjectsInfo (&info, nCount, lpHandles,
> +                                    bWaitAll, dwMilliseconds);
> +  DWORD result = WaitForMultipleObjects (info.nObject, info.hObjects,
> +                                      bWaitAll, dwMilliseconds);
> +  result = ExtractWaitResult (&info, result);
> +  CleanupWaitForMultipleObjectsInfo (&info);
> +  return result;
> +}

IIUC, this will cause us starting 2 threads when we need to start 65
processes.  Wouldn't it be better to use just one thread, and wait for
the other processes as we do now using the rest of available slots in
the wait_hnd[] array?

Finally, is it known how long it takes on a modern Windows system to
start a thread?  This could be important for assessing the impact of
this design on the performance of sub-processes on MS-Windows.

Thanks again for working on this.