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

2016-10-08 Thread Martijn Pieters
On 8 October 2016 at 17:00, Gregory Szorc  wrote:

> # HG changeset patch
> # User Gregory Szorc 
> # 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@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>



-- 
Martijn Pieters
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


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

2016-10-09 Thread Kevin Bullock
> On Oct 8, 2016, at 17:00, Gregory Szorc  wrote:
> 
> # HG changeset patch
> # User Gregory Szorc 
> # 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

Queued this series on the strength of Martijn's review. I also removed the 
#define IS_PY3K from patch 3 since it's now in util.h. If I'm wrong on that 
point let me know and we can add it back in. (I'm currently running tests 
against it.)

pacem in terris / мир / शान्ति / ‎‫سَلاَم‬ / 平和
Kevin R. Bullock

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


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

2016-10-09 Thread Gregory Szorc
On Sun, Oct 9, 2016 at 1:56 PM, Kevin Bullock <
kbullock+mercur...@ringworld.org> wrote:

> > On Oct 8, 2016, at 17:00, Gregory Szorc  wrote:
> >
> > # HG changeset patch
> > # User Gregory Szorc 
> > # 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
>
> Queued this series on the strength of Martijn's review. I also removed the
> #define IS_PY3K from patch 3 since it's now in util.h. If I'm wrong on that
> point let me know and we can add it back in. (I'm currently running tests
> against it.)
>

Removing the redundant IS_PY3K is correct: I didn't realize util.h
contained the #define until a few minutes after I wrote this patch.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel