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

login
register
mail settings
Submitter Gregory Szorc
Date Oct. 8, 2016, 3 p.m.
Message ID <136b1a54213542f535f1.1475938848@gps-mbp.local>
Download mbox | patch
Permalink /patch/16928/
State Accepted
Headers show

Comments

Gregory Szorc - Oct. 8, 2016, 3 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@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.
Martijn Pieters - Oct. 8, 2016, 3:20 p.m.
On 8 October 2016 at 17:00, Gregory Szorc <gregory.szorc@gmail.com> wrote:

> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@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@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Gregory Szorc - Oct. 9, 2016, 12:05 p.m.
On Sun, Oct 9, 2016 at 1:56 PM, Kevin Bullock <
kbullock+mercurial@ringworld.org> wrote:

> > On Oct 8, 2016, at 17:00, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> >
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@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
>
> 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.

Patch

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