Merge lp:~gz/brz/groupscompress_pyx_tidy into lp:brz

[Martin Packman]

Tidy up _groupcompress_pyx

Use cimport over cdef extern for common libraries.

Remove 'safe' memory wrappers and use pymalloc for main allocation.

Change code that was avoiding issues with old Pyrex versions.

Remove unused diff-delta.c variables.

Add test for __sizeof__ implementation.

[Jelmer Vernooij (jelmer)]

Looks reasonable.

review: Approve

=== modified file 'breezy/bzr/_groupcompress_pyx.pyx'
--- breezy/bzr/_groupcompress_pyx.pyx	2018-08-18 19:43:40 +0000
+++ breezy/bzr/_groupcompress_pyx.pyx	2018-11-16 13:31:49 +0000
@@ -22,22 +22,26 @@
 cdef extern from "python-compat.h":
     pass
 
-
-cdef extern from "Python.h":
-    ctypedef struct PyObject:
-        pass
-    int PyBytes_CheckExact(object)
-    char * PyBytes_AS_STRING(object)
-    Py_ssize_t PyBytes_GET_SIZE(object)
-    object PyBytes_FromStringAndSize(char *, Py_ssize_t)
-
-
-cdef extern from *:
-    ctypedef unsigned long size_t
-    void * malloc(size_t) nogil
-    void * realloc(void *, size_t) nogil
-    void free(void *) nogil
-    void memcpy(void *, void *, size_t) nogil
+from libc.stdlib cimport (
+    free,
+    )
+from libc.string cimport (
+    memcpy,
+    )
+
+from cpython.bytes cimport (
+    PyBytes_AS_STRING,
+    PyBytes_CheckExact,
+    PyBytes_FromStringAndSize,
+    PyBytes_GET_SIZE,
+    )
+from cpython.object cimport (
+    PyObject,
+    )
+from cpython.mem cimport (
+    PyMem_Free,
+    PyMem_Malloc,
+    )
 
 
 cdef extern from "delta.h":
@@ -78,29 +82,6 @@
     unsigned int rabin_hash (unsigned char *data)
 
 
-cdef void *safe_malloc(size_t count) except NULL:
-    cdef void *result
-    result = malloc(count)
-    if result == NULL:
-        raise MemoryError('Failed to allocate %d bytes of memory' % (count,))
-    return result
-
-
-cdef void *safe_realloc(void * old, size_t count) except NULL:
-    cdef void *result
-    result = realloc(old, count)
-    if result == NULL:
-        raise MemoryError('Failed to reallocate to %d bytes of memory'
-                          % (count,))
-    return result
-
-
-cdef int safe_free(void **val) except -1:
-    assert val != NULL
-    if val[0] != NULL:
-        free(val[0])
-        val[0] = NULL
-
 def make_delta_index(source):
     return DeltaIndex(source)
 
@@ -130,10 +111,7 @@
 
 cdef class DeltaIndex:
 
-    # We need Pyrex 0.9.8+ to understand a 'list' definition, and this object
-    # isn't performance critical
-    # cdef readonly list _sources
-    cdef readonly object _sources
+    cdef readonly list _sources
     cdef source_info *_source_infos
     cdef delta_index *_index
     cdef public unsigned long _source_offset
@@ -144,8 +122,10 @@
         self._sources = []
         self._index = NULL
         self._max_num_sources = 65000
-        self._source_infos = <source_info *>safe_malloc(sizeof(source_info)
-                                                        * self._max_num_sources)
+        self._source_infos = <source_info *>PyMem_Malloc(
+            sizeof(source_info) * self._max_num_sources)
+        if self._source_infos == NULL:
+            raise MemoryError('failed to allocate memory for DeltaIndex')
         self._source_offset = 0
         self._max_bytes_to_index = 0
         if max_bytes_to_index is not None:
@@ -157,18 +137,9 @@
     def __sizeof__(self):
         # We want to track the _source_infos allocations, but the referenced
         # void* are actually tracked in _sources itself.
-        # XXX: Cython is capable of doing sizeof(class) and returning the size
-        #      of the underlying struct. Pyrex (<= 0.9.9) refuses, so we need
-        #      to do it manually. *sigh* Note that we might get it wrong
-        #      because of alignment issues.
-        cdef Py_ssize_t size
-        # PyObject start, vtable *, 3 object pointers, 2 C ints
-        size = ((sizeof(PyObject) + sizeof(void*) + 3*sizeof(PyObject*)
-                 + sizeof(unsigned long)
-                 + sizeof(unsigned int))
+        return (sizeof(DeltaIndex)
                 + (sizeof(source_info) * self._max_num_sources)
                 + sizeof_delta_index(self._index))
-        return size
 
     def __repr__(self):
         return '%s(%d, %d)' % (self.__class__.__name__,
@@ -178,7 +149,7 @@
         if self._index != NULL:
             free_delta_index(self._index)
             self._index = NULL
-        safe_free(<void **>&self._source_infos)
+        PyMem_Free(self._source_infos)
 
     def _has_index(self):
         return (self._index != NULL)
@@ -326,10 +297,6 @@
     cdef _expand_sources(self):
         raise RuntimeError('if we move self._source_infos, then we need to'
                            ' change all of the index pointers as well.')
-        self._max_num_sources = self._max_num_sources * 2
-        self._source_infos = <source_info *>safe_realloc(self._source_infos,
-                                                sizeof(source_info)
-                                                * self._max_num_sources)
 
     def make_delta(self, target_bytes, max_delta_size=0):
         """Create a delta from the current source to the target bytes."""
@@ -399,7 +366,7 @@
     return _apply_delta(source, source_size, delta, delta_size)
 
 
-cdef unsigned char *_decode_copy_instruction(unsigned char *bytes,
+cdef unsigned char *_decode_copy_instruction(unsigned char *data,
     unsigned char cmd, unsigned int *offset,
     unsigned int *length) nogil: # cannot_raise
     """Decode a copy instruction from the next few bytes.
@@ -417,38 +384,41 @@
     size = 0
     count = 0
     if (cmd & 0x01):
-        off = bytes[count]
+        off = data[count]
         count = count + 1
     if (cmd & 0x02):
-        off = off | (bytes[count] << 8)
+        off = off | (data[count] << 8)
         count = count + 1
     if (cmd & 0x04):
-        off = off | (bytes[count] << 16)
+        off = off | (data[count] << 16)
         count = count + 1
     if (cmd & 0x08):
-        off = off | (bytes[count] << 24)
+        off = off | (data[count] << 24)
         count = count + 1
     if (cmd & 0x10):
-        size = bytes[count]
+        size = data[count]
         count = count + 1
     if (cmd & 0x20):
-        size = size | (bytes[count] << 8)
+        size = size | (data[count] << 8)
         count = count + 1
     if (cmd & 0x40):
-        size = size | (bytes[count] << 16)
+        size = size | (data[count] << 16)
         count = count + 1
     if (size == 0):
         size = 0x10000
     offset[0] = off
     length[0] = size
-    return bytes + count
+    return data + count
 
 
 cdef object _apply_delta(char *source, Py_ssize_t source_size,
                          char *delta, Py_ssize_t delta_size):
     """common functionality between apply_delta and apply_delta_to_source."""
-    cdef unsigned char *data, *top
-    cdef unsigned char *dst_buf, *out, cmd
+    cdef unsigned char *data
+    cdef unsigned char *top
+    cdef unsigned char *dst_buf
+    cdef unsigned char *out
+    cdef unsigned char cmd
     cdef Py_ssize_t size
     cdef unsigned int cp_off, cp_size
     cdef int failed
@@ -510,7 +480,6 @@
         raise RuntimeError('Did not extract the number of bytes we expected'
             ' we were left with %d bytes in "size", and top - data = %d'
             % (size, <int>(top - data)))
-        return None
 
     # *dst_size = out - dst_buf;
     if (out - dst_buf) != PyBytes_GET_SIZE(result):
@@ -567,7 +536,7 @@
     return PyBytes_FromStringAndSize(<char *>c_bytes, count)
 
 
-def decode_base128_int(bytes):
+def decode_base128_int(data):
     """Decode an integer from a 7-bit lsb encoding."""
     cdef int offset
     cdef int val
@@ -579,11 +548,11 @@
     offset = 0
     val = 0
     shift = 0
-    if not PyBytes_CheckExact(bytes):
+    if not PyBytes_CheckExact(data):
         raise TypeError('bytes is not a string')
-    c_bytes = <unsigned char*>PyBytes_AS_STRING(bytes)
+    c_bytes = <unsigned char*>PyBytes_AS_STRING(data)
     # We take off 1, because we have to be able to decode the non-expanded byte
-    num_low_bytes = PyBytes_GET_SIZE(bytes) - 1
+    num_low_bytes = PyBytes_GET_SIZE(data) - 1
     while (c_bytes[offset] & 0x80) and offset < num_low_bytes:
         val = val | ((c_bytes[offset] & 0x7F) << shift)
         shift = shift + 7

=== modified file 'breezy/bzr/diff-delta.c'
--- breezy/bzr/diff-delta.c	2017-06-08 23:30:31 +0000
+++ breezy/bzr/diff-delta.c	2018-11-16 13:31:49 +0000
@@ -220,7 +220,7 @@
     unsigned int i, j, hmask, memsize, fit_in_old, copied_count;
     struct unpacked_index_entry *entry;
     struct delta_index *index;
-    struct index_entry *packed_entry, **packed_hash, *old_entry, *copy_from;
+    struct index_entry *packed_entry, **packed_hash, *old_entry;
     struct index_entry null_entry = {0};
     void *mem;
 
@@ -334,7 +334,6 @@
              * old_index->hash_mask? Would it make any real difference?
              */
             j = i & old_index->hash_mask;
-            copy_from = old_index->hash[j];
             for (old_entry = old_index->hash[j];
                  old_entry < old_index->hash[j + 1] && old_entry->ptr != NULL;
                  old_entry++) {
@@ -905,7 +904,6 @@
     int inscnt;
     const unsigned char *ref_data, *ref_top, *data, *top;
     unsigned char *out;
-    unsigned long source_size;
 
     if (!trg_buf || !trg_size)
         return DELTA_BUFFER_EMPTY;
@@ -920,8 +918,6 @@
     if (!out)
         return DELTA_OUT_OF_MEMORY;
 
-    source_size = index->last_src->size + index->last_src->agg_offset;
-
     /* store target buffer size */
     i = trg_size;
     while (i >= 0x80) {
@@ -1053,7 +1049,8 @@
              */
             assert(moff < msource->size);
             moff += msource->agg_offset;
-            assert(moff + msize <= source_size);
+            assert(moff + msize
+	    	<= index->last_src->size + index->last_src->agg_offset);
             if (moff & 0x000000ff)
                 out[outpos++] = moff >> 0,  i |= 0x01;
             if (moff & 0x0000ff00)

=== modified file 'breezy/tests/test__groupcompress.py'
--- breezy/tests/test__groupcompress.py	2018-08-18 19:43:40 +0000
+++ breezy/tests/test__groupcompress.py	2018-11-16 13:31:49 +0000
@@ -16,6 +16,8 @@
 
 """Tests for the python and pyrex extensions of groupcompress"""
 
+import sys
+
 from .. import (
     tests,
     )
@@ -273,6 +275,13 @@
         di = self._gc_module.DeltaIndex(b'test text\n')
         self.assertEqual('DeltaIndex(1, 10)', repr(di))
 
+    def test_sizeof(self):
+        di = self._gc_module.DeltaIndex()
+        # Exact value will depend on platform but should include sources
+        # source_info is a pointer and two longs so at least 12 bytes
+        lower_bound = di._max_num_sources * 12
+        self.assertGreater(sys.getsizeof(di), lower_bound)
+
     def test__dump_no_index(self):
         di = self._gc_module.DeltaIndex()
         self.assertEqual(None, di._dump_index())