Re: [RFC PATCH] http: use xmalloc with cURL

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 12, 2019, 19:23 UTC
Message-ID: <nycvar.QRO.7.76.6.1908122118020.46@tvgsbejvaqbjf.bet>
In-Reply-To: <CAPUEspg62pRNH6=_VvWDxQ4YujHUJAoTTampc0L4t68QMj30xg@mail.gmail.com>

Hi Carlo,

On Sun, 11 Aug 2019, Carlo Arenas wrote:

Show 18 quoted lines

> On Sun, Aug 11, 2019 at 4:20 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Sat, 10 Aug 2019, Carlo Marcelo Arenas Belón wrote:
> > > cURL is very strict about its allocator being thread safe and so that might
> > > be an issue to look for.
> >
> > Looks good to me.
>
> it is not ready yet for using though, at minimum I have to fix the #ifdef
> so there is no risky of breaking someone's build because they happen
> to still be using and old enough cURL (ex: 7.10)
>
> there is also the risk that xmalloc might not be sufficiently thread
> safe (ex: when it triggers unuse_one_window() through the use of a
> try_to_free_routine in packfile.c but we could mitigate the risk for
> now by doing it only first #ifdef NED (I promise to take it out before
> it even gets to next, assuming Junio would trust me enough after the
> last blow up to even get it into pu)

I was under the impression that we _already_ use `xmalloc()` in plenty of multi-threaded scenarios...

And yes, nedmalloc meets the demand: it is thread-safe. All allocators to be used by Git have to be.

> alternatively, getting some feedback from people that know that code
> better like Shawn, since I have to admit I haven't read it enough to
> be fully confident it is safe (although I believe it was enough, or I
> wouldn't had sent it for feedback otherwise)

If you are referring to Shawn Pearce (and I assume you do, because the other Shawn I found in the shortlog only worked briefly on `git clean` and `git p4`), I have the sad duty to inform you that he passed away.

> > Still, it is a good idea to ask libcurl to use the same allocator as Git.
>
> and it should have a positive performance impact, which should be nice
> to look for.

I actually do not expect any performance impact, given that communicating via HTTP(S) is partially a waiting game, in any case, the network will be the bottleneck, not allocating memory.

Ciao, Dscho