[PATCH] Cygwin: Interim malloc speedup

[Mark Geisert]

Hi Corinna,
Happy New Year back at you!  I'm very glad to see you posting again!

Corinna Vinschen via Cygwin-patches wrote:
> Hi Mark,
> 
> Happy New Year!
> 
> On Dec 21 20:53, Mark Geisert wrote:
>> Replaces function-level lock with data-level lock provided by existing
>> dlmalloc.  Sets up to enable dlmalloc's MSPACES, but does not yet enable
>> them due to visible but uninvestigated issues.
>>
>> Single-thread applications may or may not see a performance gain,
>> depending on how heavily it uses the malloc functions.  Multi-thread
>> apps will likely see a performance gain.
[...]
>> diff --git a/winsup/cygwin/cygmalloc.h b/winsup/cygwin/cygmalloc.h
>> index 84bad824c..67a9f3b3f 100644
>> --- a/winsup/cygwin/cygmalloc.h
>> +++ b/winsup/cygwin/cygmalloc.h
[...]
>> +/* These defines tune the dlmalloc implementation in malloc.cc */
>>   # define MALLOC_FAILURE_ACTION	__set_ENOMEM ()
>>   # define USE_DL_PREFIX 1
>> +# define USE_LOCKS 1
> 
> Just enabling USE_LOCKS looks wrong to me.  Before enabling USE_LOCKS,
> you should check how the actual locking is performed.  For non WIN32,
> that will be pthread_mutex_lock/unlock, which may not be feasible,
> because it may break expectations during fork.

I did investigate this before setting it, and I've been running with '#define 
USE_LOCKS 1' for many weeks and haven't seen any memory issues of any kind. 
Malloc multi-thread stress testing, fork() stress testing, Cygwin DLL builds, 
Python and binutils builds, routine X usage; all OK.  (Once I straightened out 
sped-up mkimport to actually do what Jon T suggested, blush.)

> What you may want to do is setting USE_LOCKS to 2, and defining your own
> MLOCK_T/ACQUIRE_LOCK/... macros (in the `#if USE_LOCKS > 1' branch of
> the malloc source, see lines 1798ff), using a type which is non-critical
> during forking, as well as during process initialization.  Win32 fast
> R/W Locks come to mind and adding them should be pretty straight-forward.
> This may also allow MSPACES to work OOTB.

With '#define USE_LOCKS 1' the tangled mess of #if-logic in malloc.cc resolves on 
Cygwin to using pthread_mutex_locks, so that seems to be OK as-is unless what 
you're suggesting is preferable for speed (or MSPACES when I get to that).
>> +# define LOCK_AT_FORK 0
> 
> This looks dangerous.  You're removing the locking from fork entirely
> *and* the lock isn't re-initialized in the child.  This reinitializing
> was no problem before because mallock was NO_COPY, but it's a problem
> now because the global malloc_state _gm_ isn't (and mustn't).  The
> current implementation calling
> 
>    #if LOCK_AT_FORK
>        pthread_atfork(&pre_fork, &post_fork_parent, &post_fork_child);
>    #endif
> 
> should do the trick, assuming the USE_LOCKS stuff is working as desired.

I don't remember what led me to #define LOCK_AT_FORK 0, but in the new light of 
this year it's obviously wrong.  I've #define'd it 1.

>> [...]
>> +#if MSPACES
>> +/* If mspaces (thread-specific memory pools) are enabled, use a thread-
>> +   local variable to store a pointer to the calling thread's mspace.
>> +
>> +   On any use of a malloc-family function, if the appropriate mspace cannot
>> +   be determined, the general (non-mspace) form of the corresponding malloc
>> +   function is substituted.  This is not expected to happen often.
>> +*/
>> +static NO_COPY DWORD tls_mspace; // index into thread's TLS array
>> +
>> +static void *
>> +get_current_mspace ()
>> +{
>> +  if (unlikely (tls_mspace == 0))
>> +    return 0;
>> +
>> +  void *m = TlsGetValue (tls_mspace);
>> +  if (unlikely (m == 0))
>> +    {
>> +      m = create_mspace (MSPACE_SIZE, 0);
>> +      if (!m)
>> +        return 0;
>> +      TlsSetValue (tls_mspace, m);
>> +    }
>> +  return m;
>> +}
>> +#endif
> 
> Please define a new slot in _cygtls keeping the memory address returned
> by create_mspace.  You don't have to call TlsGetValue/TlsSetValue.

Thank you for repeating this suggestion.  I now understand why it's better.

I'm going to delay submitting the v2 patch until I see where the investigation of 
Achim's malloc testcase (running zstd on 1600 files, for instance) leads.  I'm 
about to respond to his thread in cygwin-apps.
Thanks & Regards,

..mark