[PATCH 01 of 11] bdiff: use Python memory allocator in fixws

[Jun Wu]

Excerpts from Augie Fackler's message of 2017-03-09 18:24:22 -0800:
> 
> > On Mar 9, 2017, at 18:12, Gregory Szorc <gregory.szorc at gmail.com> wrote:
> > 
> > On Thu, Mar 9, 2017 at 5:46 PM, Augie Fackler <raf at durin42.com> wrote:
> > On Thu, Mar 09, 2017 at 01:59:09PM -0800, Gregory Szorc wrote:
> > > # HG changeset patch
> > > # User Gregory Szorc <gregory.szorc at gmail.com>
> > > # Date 1489089265 28800
> > > #      Thu Mar 09 11:54:25 2017 -0800
> > > # Node ID 7a8ce919a13a9ff6e73f391fe14d228e18387f15
> > > # Parent  1871a1ee64ed49172b1568b89cdbab126284b309
> > > bdiff: use Python memory allocator in fixws
> > >
> > > Python has its own memory allocation APIs. For allocations
> > > <= 512 bytes, it allocates memory from arenas. This means that
> > > average small allocations don't call the system allocator, which
> > > makes them faster. Also, arena allocations cut down on memory
> > > fragmentation, which can matter for performance in long-running
> > > processes.
> > >
> > > Another advantage of using the Python memory allocator is that
> > > allocations are tracked by Python. This is a bigger deal in
> > > Python 3, as modern versions of Python have some decent built-in
> > > tools for examining memory usage, leaks, etc.
> > 
> > This is all true...for PyObject* functions. PyMem_Malloc is a trivial
> > wrapper macro around malloc:
> > 
> > https://hg.python.org/cpython/file/v2.7.10/Include/pymem.h#l74 
> > 
> > (YMMV if PYMALLOC_DEBUG is defined, but I don't think that's the default)
> > 
> > OTOH, at least this explains why you couldn't see a performance change?
> > 
> > Indeed. I've apparently been living too much in Python 3 lately, as Python 3's PyMem_Malloc() *does* use the arena-based "pymalloc" allocator for small allocations.
> 
> Only in some build configurations - do you know what the defaults are? (In particular, -DWITH_PYMALLOC at compile time, see https://github.com/python/cpython/blob/master/Objects/obmalloc.c#L153 - it's a bit of a maze of structs and preprocessor hoops.)
> 
> I'm hesitant to move towards cpython integrated allocation functions because it might make future fuzzing or pypy work more difficult.

For PyPy, I think as long as the pure C layer is free of Py*, it's okay.
*_moudle.c using Py* seem fine to me.

I'm not sure about fuzzing. Maybe we could use a macro if that's a concern.

> 
> > 
> > PyObject_Malloc() does exist in Python 2 and does use arena-based allocations. But very few things in CPython use it and it is undocumented in the C API docs. So we should probably stay away.
> > 
> > Since the patches are no-op for Python 2 and represent a positive change in Python 3, do you want to just queue them with an in-flight commit message change to add "in Python 3?" I can resend just part 1 with that change if you want.
> >  
> > 
> > >
> > > This patch converts a trivial malloc() + free() in the bdiff code
> > > to use the Python allocator APIs. Since the object being
> > > operated on is a line, chances are it will use an arena. So,
> > > this could have a net positive impact on performance (although
> > > I didn't measure it).
> > >
> > > diff --git a/mercurial/bdiff_module.c b/mercurial/bdiff_module.c
> > > --- a/mercurial/bdiff_module.c
> > > +++ b/mercurial/bdiff_module.c
> > > @@ -158,7 +158,7 @@ static PyObject *fixws(PyObject *self, P
> > >       r = PyBytes_AsString(s);
> > >       rlen = PyBytes_Size(s);
> > >
> > > -     w = (char *)malloc(rlen ? rlen : 1);
> > > +     w = (char *)PyMem_Malloc(rlen ? rlen : 1);
> > >       if (!w)
> > >               goto nomem;
> > >
> > > @@ -178,7 +178,7 @@ static PyObject *fixws(PyObject *self, P
> > >       result = PyBytes_FromStringAndSize(w, wlen);
> > >
> > >  nomem:
> > > -     free(w);
> > > +     PyMem_Free(w);
> > >       return result ? result : PyErr_NoMemory();
> > >  }
> > >
> > > _______________________________________________
> > > Mercurial-devel mailing list
> > > Mercurial-devel at mercurial-scm.org
> > > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel 
>