Patchwork parsers: use buffer to store revlog index

login
register
mail settings
Submitter Jun Wu
Date Dec. 6, 2016, 11:46 a.m.
Message ID <36b4288ea10a9a76c1d9.1481024760@x1c>
Download mbox | patch
Permalink /patch/17827/
State Accepted
Headers show

Comments

Jun Wu - Dec. 6, 2016, 11:46 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1481024689 0
#      Tue Dec 06 11:44:49 2016 +0000
# Node ID 36b4288ea10a9a76c1d993d47e002baaf7705ffb
# Parent  2463b16bbd3710b619e0e948651db9948341f990
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 36b4288ea10a
parsers: use buffer to store revlog index

Previously, the revlog index passed to parse_index2 must be a "string",
which means we have to read the whole revlog index into memory. This patch
makes the code accept a generic Py_buffer, to be more flexible - it could be
a "string", or anything that implements the buffer interface, like a mmap-ed
region.

Note: ideally we want to remove the "data" field. However, it is still used
in parse_index2:

    if (idx->inlined) {
        cache = Py_BuildValue("iO", 0, idx->data);
        ....
    }
    ....
    tuple = Py_BuildValue("NN", idx, cache);
    ....
    return tuple;

Its only users are revlogio.parseindex and revlog.__init__:

    # revlogio.parseindex
    index, cache = parsers.parse_index2(data, inline)
    return index, getattr(index, 'nodemap', None), cache

    # revlog.__init__
    d = self._io.parseindex(indexdata, self._inline)
    self.index, nodemap, self._chunkcache = d

Maybe we could move the logic (testing inline and returnning "data" object)
to revlog.py. But that should be a separate patch.
Augie Fackler - Dec. 6, 2016, 9:22 p.m.
On Tue, Dec 06, 2016 at 11:46:00AM +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1481024689 0
> #      Tue Dec 06 11:44:49 2016 +0000
> # Node ID 36b4288ea10a9a76c1d993d47e002baaf7705ffb
> # Parent  2463b16bbd3710b619e0e948651db9948341f990
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 36b4288ea10a
> parsers: use buffer to store revlog index

Looks reasonable, queued, thanks.

>
> Previously, the revlog index passed to parse_index2 must be a "string",
> which means we have to read the whole revlog index into memory. This patch
> makes the code accept a generic Py_buffer, to be more flexible - it could be
> a "string", or anything that implements the buffer interface, like a mmap-ed
> region.
>
> Note: ideally we want to remove the "data" field. However, it is still used
> in parse_index2:
>
>     if (idx->inlined) {
>         cache = Py_BuildValue("iO", 0, idx->data);
>         ....
>     }
>     ....
>     tuple = Py_BuildValue("NN", idx, cache);
>     ....
>     return tuple;
>
> Its only users are revlogio.parseindex and revlog.__init__:
>
>     # revlogio.parseindex
>     index, cache = parsers.parse_index2(data, inline)
>     return index, getattr(index, 'nodemap', None), cache
>
>     # revlog.__init__
>     d = self._io.parseindex(indexdata, self._inline)
>     self.index, nodemap, self._chunkcache = d
>
> Maybe we could move the logic (testing inline and returnning "data" object)
> to revlog.py. But that should be a separate patch.
>
> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> --- a/mercurial/parsers.c
> +++ b/mercurial/parsers.c
> @@ -754,4 +754,5 @@ typedef struct {
>       /* Type-specific fields go here. */
>       PyObject *data;        /* raw bytes of index */
> +	Py_buffer buf;         /* buffer of data */
>       PyObject **cache;      /* cached tuples */
>       const char **offsets;  /* populated on demand */
> @@ -809,5 +810,5 @@ static const char *index_deref(indexObje
>       }
>
> -	return PyBytes_AS_STRING(self->data) + pos * v1_hdrsize;
> +	return (const char *)(self->buf.buf) + pos * v1_hdrsize;
>  }
>
> @@ -2390,7 +2391,7 @@ static int index_assign_subscript(indexO
>  static Py_ssize_t inline_scan(indexObject *self, const char **offsets)
>  {
> -	const char *data = PyBytes_AS_STRING(self->data);
> +	const char *data = (const char *)self->buf.buf;
>       Py_ssize_t pos = 0;
> -	Py_ssize_t end = PyBytes_GET_SIZE(self->data);
> +	Py_ssize_t end = self->buf.len;
>       long incr = v1_hdrsize;
>       Py_ssize_t len = 0;
> @@ -2426,4 +2427,5 @@ static int index_init(indexObject *self,
>       self->cache = NULL;
>       self->data = NULL;
> +	memset(&self->buf, 0, sizeof(self->buf));
>       self->headrevs = NULL;
>       self->filteredrevs = Py_None;
> @@ -2434,9 +2436,13 @@ static int index_init(indexObject *self,
>       if (!PyArg_ParseTuple(args, "OO", &data_obj, &inlined_obj))
>               return -1;
> -	if (!PyBytes_Check(data_obj)) {
> -		PyErr_SetString(PyExc_TypeError, "data is not a string");
> +	if (!PyObject_CheckBuffer(data_obj)) {
> +		PyErr_SetString(PyExc_TypeError,
> +				"data does not support buffer interface");
>               return -1;
>       }
> -	size = PyBytes_GET_SIZE(data_obj);
> +
> +	if (PyObject_GetBuffer(data_obj, &self->buf, PyBUF_SIMPLE) == -1)
> +		return -1;
> +	size = self->buf.len;
>
>       self->inlined = inlined_obj && PyObject_IsTrue(inlined_obj);
> @@ -2479,4 +2485,8 @@ static void index_dealloc(indexObject *s
>       _index_clearcaches(self);
>       Py_XDECREF(self->filteredrevs);
> +	if (self->buf.buf) {
> +		PyBuffer_Release(&self->buf);
> +		memset(&self->buf, 0, sizeof(self->buf));
> +	}
>       Py_XDECREF(self->data);
>       Py_XDECREF(self->added);
> @@ -2578,5 +2588,6 @@ static PyTypeObject indexType = {
>   *
>   * index: an index object that lazily parses RevlogNG records
> - * cache: if data is inlined, a tuple (index_file_content, 0), else None
> + * cache: if data is inlined, a tuple (0, index_file_content), else None
> + *        index_file_content could be a string, or a buffer
>   *
>   * added complications are for backwards compatibility
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Gregory Szorc - Dec. 13, 2016, 6:11 a.m.
On Tue, Dec 6, 2016 at 3:46 AM, Jun Wu <quark@fb.com> wrote:

> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1481024689 0
> #      Tue Dec 06 11:44:49 2016 +0000
> # Node ID 36b4288ea10a9a76c1d993d47e002baaf7705ffb
> # Parent  2463b16bbd3710b619e0e948651db9948341f990
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r
> 36b4288ea10a
> parsers: use buffer to store revlog index
>

I know this is queued already, but I think C code should have more eyes
than Python code.

This patch LGTM. Only thing I would have done differently is store a
Py_buffer* instead of a Py_buffer to ensure alignment of indexObject struct
entries. But I don't think this struct is that performance sensitive to
care. Feel free to address in a follow-up if you disagree.


>
> Previously, the revlog index passed to parse_index2 must be a "string",
> which means we have to read the whole revlog index into memory. This patch
> makes the code accept a generic Py_buffer, to be more flexible - it could
> be
> a "string", or anything that implements the buffer interface, like a
> mmap-ed
> region.
>
> Note: ideally we want to remove the "data" field. However, it is still used
> in parse_index2:
>
>     if (idx->inlined) {
>         cache = Py_BuildValue("iO", 0, idx->data);
>         ....
>     }
>     ....
>     tuple = Py_BuildValue("NN", idx, cache);
>     ....
>     return tuple;
>
> Its only users are revlogio.parseindex and revlog.__init__:
>
>     # revlogio.parseindex
>     index, cache = parsers.parse_index2(data, inline)
>     return index, getattr(index, 'nodemap', None), cache
>
>     # revlog.__init__
>     d = self._io.parseindex(indexdata, self._inline)
>     self.index, nodemap, self._chunkcache = d
>
> Maybe we could move the logic (testing inline and returnning "data" object)
> to revlog.py. But that should be a separate patch.
>
> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> --- a/mercurial/parsers.c
> +++ b/mercurial/parsers.c
> @@ -754,4 +754,5 @@ typedef struct {
>         /* Type-specific fields go here. */
>         PyObject *data;        /* raw bytes of index */
> +       Py_buffer buf;         /* buffer of data */
>         PyObject **cache;      /* cached tuples */
>         const char **offsets;  /* populated on demand */
> @@ -809,5 +810,5 @@ static const char *index_deref(indexObje
>         }
>
> -       return PyBytes_AS_STRING(self->data) + pos * v1_hdrsize;
> +       return (const char *)(self->buf.buf) + pos * v1_hdrsize;
>  }
>
> @@ -2390,7 +2391,7 @@ static int index_assign_subscript(indexO
>  static Py_ssize_t inline_scan(indexObject *self, const char **offsets)
>  {
> -       const char *data = PyBytes_AS_STRING(self->data);
> +       const char *data = (const char *)self->buf.buf;
>         Py_ssize_t pos = 0;
> -       Py_ssize_t end = PyBytes_GET_SIZE(self->data);
> +       Py_ssize_t end = self->buf.len;
>         long incr = v1_hdrsize;
>         Py_ssize_t len = 0;
> @@ -2426,4 +2427,5 @@ static int index_init(indexObject *self,
>         self->cache = NULL;
>         self->data = NULL;
> +       memset(&self->buf, 0, sizeof(self->buf));
>         self->headrevs = NULL;
>         self->filteredrevs = Py_None;
> @@ -2434,9 +2436,13 @@ static int index_init(indexObject *self,
>         if (!PyArg_ParseTuple(args, "OO", &data_obj, &inlined_obj))
>                 return -1;
> -       if (!PyBytes_Check(data_obj)) {
> -               PyErr_SetString(PyExc_TypeError, "data is not a string");
> +       if (!PyObject_CheckBuffer(data_obj)) {
> +               PyErr_SetString(PyExc_TypeError,
> +                               "data does not support buffer interface");
>                 return -1;
>         }
> -       size = PyBytes_GET_SIZE(data_obj);
> +
> +       if (PyObject_GetBuffer(data_obj, &self->buf, PyBUF_SIMPLE) == -1)
> +               return -1;
> +       size = self->buf.len;
>
>         self->inlined = inlined_obj && PyObject_IsTrue(inlined_obj);
> @@ -2479,4 +2485,8 @@ static void index_dealloc(indexObject *s
>         _index_clearcaches(self);
>         Py_XDECREF(self->filteredrevs);
> +       if (self->buf.buf) {
> +               PyBuffer_Release(&self->buf);
> +               memset(&self->buf, 0, sizeof(self->buf));
> +       }
>         Py_XDECREF(self->data);
>         Py_XDECREF(self->added);
> @@ -2578,5 +2588,6 @@ static PyTypeObject indexType = {
>   *
>   * index: an index object that lazily parses RevlogNG records
> - * cache: if data is inlined, a tuple (index_file_content, 0), else None
> + * cache: if data is inlined, a tuple (0, index_file_content), else None
> + *        index_file_content could be a string, or a buffer
>   *
>   * added complications are for backwards compatibility
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - Dec. 13, 2016, 2:04 p.m.
On Mon, 12 Dec 2016 22:11:02 -0800, Gregory Szorc wrote:
> This patch LGTM. Only thing I would have done differently is store a
> Py_buffer* instead of a Py_buffer to ensure alignment of indexObject struct
> entries. But I don't think this struct is that performance sensitive to
> care. Feel free to address in a follow-up if you disagree.

Just curious. What kind of alignment issue would be solved by not storing
a Py_buffer struct?
Gregory Szorc - Dec. 13, 2016, 4:28 p.m.
On Tue, Dec 13, 2016 at 6:04 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Mon, 12 Dec 2016 22:11:02 -0800, Gregory Szorc wrote:
> > This patch LGTM. Only thing I would have done differently is store a
> > Py_buffer* instead of a Py_buffer to ensure alignment of indexObject
> struct
> > entries. But I don't think this struct is that performance sensitive to
> > care. Feel free to address in a follow-up if you disagree.
>
> Just curious. What kind of alignment issue would be solved by not storing
> a Py_buffer struct?
>

Without looking at what is actually inside Py_buffer, I'm somewhat paranoid
that struct members won't be aligned on word boundaries, creating members
that span words, leading to sub-optimal access of highly-accessed members.
I know Python is generally pretty good about struct alignment. And the
compiler may not be "packing" structs unless ordered. So perhaps we're fine
here?
Jun Wu - Dec. 13, 2016, 7:24 p.m.
Excerpts from Gregory Szorc's message of 2016-12-13 08:28:14 -0800:
> On Tue, Dec 13, 2016 at 6:04 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> > On Mon, 12 Dec 2016 22:11:02 -0800, Gregory Szorc wrote:
> > > This patch LGTM. Only thing I would have done differently is store a
> > > Py_buffer* instead of a Py_buffer to ensure alignment of indexObject
> > struct
> > > entries. But I don't think this struct is that performance sensitive to
> > > care. Feel free to address in a follow-up if you disagree.
> >
> > Just curious. What kind of alignment issue would be solved by not storing
> > a Py_buffer struct?
> >
> 
> Without looking at what is actually inside Py_buffer, I'm somewhat paranoid
> that struct members won't be aligned on word boundaries, creating members
> that span words, leading to sub-optimal access of highly-accessed members.
> I know Python is generally pretty good about struct alignment. And the
> compiler may not be "packing" structs unless ordered. So perhaps we're fine
> here?

I think we are fine as the struct is not explicitly packed.
Yuya Nishihara - Dec. 14, 2016, 1:28 p.m.
On Tue, 13 Dec 2016 19:24:42 +0000, Jun Wu wrote:
> Excerpts from Gregory Szorc's message of 2016-12-13 08:28:14 -0800:
> > On Tue, Dec 13, 2016 at 6:04 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > 
> > > On Mon, 12 Dec 2016 22:11:02 -0800, Gregory Szorc wrote:
> > > > This patch LGTM. Only thing I would have done differently is store a
> > > > Py_buffer* instead of a Py_buffer to ensure alignment of indexObject
> > > struct
> > > > entries. But I don't think this struct is that performance sensitive to
> > > > care. Feel free to address in a follow-up if you disagree.
> > >
> > > Just curious. What kind of alignment issue would be solved by not storing
> > > a Py_buffer struct?
> > >
> > 
> > Without looking at what is actually inside Py_buffer, I'm somewhat paranoid
> > that struct members won't be aligned on word boundaries, creating members
> > that span words, leading to sub-optimal access of highly-accessed members.
> > I know Python is generally pretty good about struct alignment. And the
> > compiler may not be "packing" structs unless ordered. So perhaps we're fine
> > here?
> 
> I think we are fine as the struct is not explicitly packed.

Yeah, struct fields should be aligned well by default even on x86 architectures
where unaligned access is allowed.

Patch

diff --git a/mercurial/parsers.c b/mercurial/parsers.c
--- a/mercurial/parsers.c
+++ b/mercurial/parsers.c
@@ -754,4 +754,5 @@  typedef struct {
 	/* Type-specific fields go here. */
 	PyObject *data;        /* raw bytes of index */
+	Py_buffer buf;         /* buffer of data */
 	PyObject **cache;      /* cached tuples */
 	const char **offsets;  /* populated on demand */
@@ -809,5 +810,5 @@  static const char *index_deref(indexObje
 	}
 
-	return PyBytes_AS_STRING(self->data) + pos * v1_hdrsize;
+	return (const char *)(self->buf.buf) + pos * v1_hdrsize;
 }
 
@@ -2390,7 +2391,7 @@  static int index_assign_subscript(indexO
 static Py_ssize_t inline_scan(indexObject *self, const char **offsets)
 {
-	const char *data = PyBytes_AS_STRING(self->data);
+	const char *data = (const char *)self->buf.buf;
 	Py_ssize_t pos = 0;
-	Py_ssize_t end = PyBytes_GET_SIZE(self->data);
+	Py_ssize_t end = self->buf.len;
 	long incr = v1_hdrsize;
 	Py_ssize_t len = 0;
@@ -2426,4 +2427,5 @@  static int index_init(indexObject *self,
 	self->cache = NULL;
 	self->data = NULL;
+	memset(&self->buf, 0, sizeof(self->buf));
 	self->headrevs = NULL;
 	self->filteredrevs = Py_None;
@@ -2434,9 +2436,13 @@  static int index_init(indexObject *self,
 	if (!PyArg_ParseTuple(args, "OO", &data_obj, &inlined_obj))
 		return -1;
-	if (!PyBytes_Check(data_obj)) {
-		PyErr_SetString(PyExc_TypeError, "data is not a string");
+	if (!PyObject_CheckBuffer(data_obj)) {
+		PyErr_SetString(PyExc_TypeError,
+				"data does not support buffer interface");
 		return -1;
 	}
-	size = PyBytes_GET_SIZE(data_obj);
+
+	if (PyObject_GetBuffer(data_obj, &self->buf, PyBUF_SIMPLE) == -1)
+		return -1;
+	size = self->buf.len;
 
 	self->inlined = inlined_obj && PyObject_IsTrue(inlined_obj);
@@ -2479,4 +2485,8 @@  static void index_dealloc(indexObject *s
 	_index_clearcaches(self);
 	Py_XDECREF(self->filteredrevs);
+	if (self->buf.buf) {
+		PyBuffer_Release(&self->buf);
+		memset(&self->buf, 0, sizeof(self->buf));
+	}
 	Py_XDECREF(self->data);
 	Py_XDECREF(self->added);
@@ -2578,5 +2588,6 @@  static PyTypeObject indexType = {
  *
  * index: an index object that lazily parses RevlogNG records
- * cache: if data is inlined, a tuple (index_file_content, 0), else None
+ * cache: if data is inlined, a tuple (0, index_file_content), else None
+ *        index_file_content could be a string, or a buffer
  *
  * added complications are for backwards compatibility