[PATCH 4 of 4] dirs: document performance reasons for bypassing Python C API

[Martijn Pieters]

On 8 October 2016 at 17:00, Gregory Szorc <gregory.szorc at gmail.com> wrote:

> # HG changeset patch
> # User Gregory Szorc <gregory.szorc at gmail.com>
> # Date 1475938278 -7200
> #      Sat Oct 08 16:51:18 2016 +0200
> # Node ID 136b1a54213542f535f155de5ce01049b3577b4a
> # Parent  da39a5835ea2d1aae1a51995b6d7e593f598be4b
> dirs: document performance reasons for bypassing Python C API
>
> So someone isn't tempted to change it.
>

+100 for the comment. Hacking in the internals of immutable Python types
outside of the C-API would otherwise increase the WTF-frequency to
intolerable levels, as the old code already did.

The only thing that could *perhaps* be added (in the commit message or in
the comment) is a pointer to
https://docs.python.org/2/c-api/string.html#c._PyString_Resize /
https://docs.python.org/3/c-api/bytes.html#c._PyBytes_Resize, where the
string length hacking originates from, so that future maintainers can at
least follow *that* implementation as a reference guide if this were to
break against future CPython changes.


>
> diff --git a/mercurial/dirs.c b/mercurial/dirs.c
> --- a/mercurial/dirs.c
> +++ b/mercurial/dirs.c
> @@ -55,8 +55,16 @@ static int _addpath(PyObject *dirs, PyOb
>         Py_ssize_t pos = PyBytes_GET_SIZE(path);
>         PyObject *key = NULL;
>         int ret = -1;
>
> +       /* This loop is super critical for performance. That's why we
> inline
> +       * access to Python structs instead of going through a supported
> API.
> +       * The implementation, therefore, is heavily dependent on CPython
> +       * implementation details. We also commit violations of the Python
> +       * "protocol" such as mutating immutable objects. But since we only
> +       * mutate objects created in this function or in other well-defined
> +       * locations, the references are known so these violations should go
> +       * unnoticed. */
>         while ((pos = _finddir(cpath, pos - 1)) != -1) {
>                 PyObject *val;
>
>                 /* It's likely that every prefix already has an entry
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>



-- 
Martijn Pieters
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20161008/9c250ace/attachment.html>