<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Dec 6, 2016 at 3:46 AM, Jun Wu <span dir="ltr"><<a href="mailto:quark@fb.com" target="_blank">quark@fb.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"># HG changeset patch<br>
# User Jun Wu <<a href="mailto:quark@fb.com">quark@fb.com</a>><br>
# Date 1481024689 0<br>
#      Tue Dec 06 11:44:49 2016 +0000<br>
# Node ID 36b4288ea10a9a76c1d993d47e002b<wbr>aaf7705ffb<br>
# Parent  2463b16bbd3710b619e0e948651db9<wbr>948341f990<br>
# Available At <a href="https://bitbucket.org/quark-zju/hg-draft" rel="noreferrer" target="_blank">https://bitbucket.org/quark-<wbr>zju/hg-draft</a><br>
#              hg pull <a href="https://bitbucket.org/quark-zju/hg-draft" rel="noreferrer" target="_blank">https://bitbucket.org/quark-<wbr>zju/hg-draft</a> -r 36b4288ea10a<br>
parsers: use buffer to store revlog index<br></blockquote><div><br></div><div>I know this is queued already, but I think C code should have more eyes than Python code.<br><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Previously, the revlog index passed to parse_index2 must be a "string",<br>
which means we have to read the whole revlog index into memory. This patch<br>
makes the code accept a generic Py_buffer, to be more flexible - it could be<br>
a "string", or anything that implements the buffer interface, like a mmap-ed<br>
region.<br>
<br>
Note: ideally we want to remove the "data" field. However, it is still used<br>
in parse_index2:<br>
<br>
    if (idx->inlined) {<br>
        cache = Py_BuildValue("iO", 0, idx->data);<br>
        ....<br>
    }<br>
    ....<br>
    tuple = Py_BuildValue("NN", idx, cache);<br>
    ....<br>
    return tuple;<br>
<br>
Its only users are revlogio.parseindex and revlog.__init__:<br>
<br>
    # revlogio.parseindex<br>
    index, cache = parsers.parse_index2(data, inline)<br>
    return index, getattr(index, 'nodemap', None), cache<br>
<br>
    # revlog.__init__<br>
    d = self._io.parseindex(indexdata, self._inline)<br>
    self.index, nodemap, self._chunkcache = d<br>
<br>
Maybe we could move the logic (testing inline and returnning "data" object)<br>
to revlog.py. But that should be a separate patch.<br>
<br>
diff --git a/mercurial/parsers.c b/mercurial/parsers.c<br>
--- a/mercurial/parsers.c<br>
+++ b/mercurial/parsers.c<br>
@@ -754,4 +754,5 @@ typedef struct {<br>
        /* Type-specific fields go here. */<br>
        PyObject *data;        /* raw bytes of index */<br>
+       Py_buffer buf;         /* buffer of data */<br>
        PyObject **cache;      /* cached tuples */<br>
        const char **offsets;  /* populated on demand */<br>
@@ -809,5 +810,5 @@ static const char *index_deref(indexObje<br>
        }<br>
<br>
-       return PyBytes_AS_STRING(self->data) + pos * v1_hdrsize;<br>
+       return (const char *)(self->buf.buf) + pos * v1_hdrsize;<br>
 }<br>
<br>
@@ -2390,7 +2391,7 @@ static int index_assign_subscript(indexO<br>
 static Py_ssize_t inline_scan(indexObject *self, const char **offsets)<br>
 {<br>
-       const char *data = PyBytes_AS_STRING(self->data);<br>
+       const char *data = (const char *)self->buf.buf;<br>
        Py_ssize_t pos = 0;<br>
-       Py_ssize_t end = PyBytes_GET_SIZE(self->data);<br>
+       Py_ssize_t end = self->buf.len;<br>
        long incr = v1_hdrsize;<br>
        Py_ssize_t len = 0;<br>
@@ -2426,4 +2427,5 @@ static int index_init(indexObject *self,<br>
        self->cache = NULL;<br>
        self->data = NULL;<br>
+       memset(&self->buf, 0, sizeof(self->buf));<br>
        self->headrevs = NULL;<br>
        self->filteredrevs = Py_None;<br>
@@ -2434,9 +2436,13 @@ static int index_init(indexObject *self,<br>
        if (!PyArg_ParseTuple(args, "OO", &data_obj, &inlined_obj))<br>
                return -1;<br>
-       if (!PyBytes_Check(data_obj)) {<br>
-               PyErr_SetString(PyExc_<wbr>TypeError, "data is not a string");<br>
+       if (!PyObject_CheckBuffer(data_<wbr>obj)) {<br>
+               PyErr_SetString(PyExc_<wbr>TypeError,<br>
+                               "data does not support buffer interface");<br>
                return -1;<br>
        }<br>
-       size = PyBytes_GET_SIZE(data_obj);<br>
+<br>
+       if (PyObject_GetBuffer(data_obj, &self->buf, PyBUF_SIMPLE) == -1)<br>
+               return -1;<br>
+       size = self->buf.len;<br>
<br>
        self->inlined = inlined_obj && PyObject_IsTrue(inlined_obj);<br>
@@ -2479,4 +2485,8 @@ static void index_dealloc(indexObject *s<br>
        _index_clearcaches(self);<br>
        Py_XDECREF(self->filteredrevs)<wbr>;<br>
+       if (self->buf.buf) {<br>
+               PyBuffer_Release(&self->buf);<br>
+               memset(&self->buf, 0, sizeof(self->buf));<br>
+       }<br>
        Py_XDECREF(self->data);<br>
        Py_XDECREF(self->added);<br>
@@ -2578,5 +2588,6 @@ static PyTypeObject indexType = {<br>
  *<br>
  * index: an index object that lazily parses RevlogNG records<br>
- * cache: if data is inlined, a tuple (index_file_content, 0), else None<br>
+ * cache: if data is inlined, a tuple (0, index_file_content), else None<br>
+ *        index_file_content could be a string, or a buffer<br>
  *<br>
  * added complications are for backwards compatibility<br>
______________________________<wbr>_________________<br>
Mercurial-devel mailing list<br>
<a href="mailto:Mercurial-devel@mercurial-scm.org">Mercurial-devel@mercurial-scm.<wbr>org</a><br>
<a href="https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel" rel="noreferrer" target="_blank">https://www.mercurial-scm.org/<wbr>mailman/listinfo/mercurial-<wbr>devel</a><br>
</blockquote></div><br></div></div>