Re: [PATCH] compat: Allow static initializer for pthreads on Windows

From: Stefan Beller <sbeller@google.com>
Date: Oct 27, 2016, 17:01 UTC
Message-ID: <CAGZ79kbP3pgPHgv-x1Q-Q1QwmXc=gOyxWhXh2SngO8WSZc3PFA@mail.gmail.com>
In-Reply-To: <xmqqd1im8foi.fsf@gitster.mtv.corp.google.com>

> Johannes Sixt <j6t@kdbg.org> writes:
> This is the pessimization that I am talking about. I would not mind at all if
> it were only for the attribute subsystem, but the proposed patch would
> pessimize *all* uses of pthread_mutex_lock.

It would only pessimize *uninitialized* mutexes? For initialized mutexes the added burden is super cheap (one additional condition).

The positive aspect of this way the patch proposes would be that any future contributor not knowing the details of how to do mutexes right on Windows, would not totally break the whole system, i.e. this seems to be more maintainable in the future as it reduces the friction between pthreads mutexes and the way we can do things in Git in a platform independent way

On Wed, Oct 26, 2016 at 11:33 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 8 quoted lines

>>
>> Lazy on-demand initialization as needed, perhaps?  The on-demand
>> initialization mechanism may become no-op on some platforms that can
>> do static initialization.
>
> Ah, I think I misunderstood your "please rewrite".  Did you mean to
> add "void attr_start(void)" helper function to attr.c that does
> series of pthread_mutex_init() calls as needed?

Well one init for now.

>  That function can
> be called from main() of platforms that cannot statically initialize
> mutices,

By main you mean the main() in common-main.c or cmd_main in git.c ?

Those both look like the wrong place. Of course it would work adding it there, but it smells like a maintenance nightmare.

And then we would modify the attr_start command depending on the platform, i.e.

#ifdef WIN32
void attr_start(void)
{
    pthread_mutex_init(..);
}
#else
void attr_start(void)
{
    /* nothing as it is statically init'd */
}
#endif

> while on other platforms it can be a no-op as long as the
> variables are statically initialized?  If so, that would not pessimize
> any platform, I would think.

I would think this pessimizes ALL platforms from a maintenance perspective (but what do I know about maintaining stuff as an eager young contributor ;)

So I am willing to go that route, though I do not quite understand where exactly you'd expect me to put the initializer as all places I can think of are not the right place.

Thanks, Stefan