Merge pull request #126 from pycompression/review

rhpvorderman · web-flow · commit 01b1e02a8f57 · 2022-09-27T11:11:52.000+02:00
Small performance and style refactorings
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -7,6 +7,12 @@ Changelog
 .. This document is user facing. Please word the changes in such a way
 .. that users understand how the changes affect the new version.
 
+version 1.1.0-dev
+-----------------
++ Small performance-oriented refactorings: argument parsing formats and
+  keywords are now static, some very small functions are now declared inline
+  and struct ordering was changed.
+
 version 1.0.1
 ------------------
 + Fixed failing tests and wheel builds for PyPy.
diff --git a/src/isal/_isalmodule.c b/src/isal/_isalmodule.c
@@ -1,12 +1,15 @@
-//  Copyright (c) 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010,
-// 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021, 2022
-// Python Software Foundation; All Rights Reserved
+/*
+Copyright (c) 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010,
+2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021, 2022
+Python Software Foundation; All Rights Reserved
 
-// This file is part of python-isal which is distributed under the 
-// PYTHON SOFTWARE FOUNDATION LICENSE VERSION 2.
+This file is part of python-isal which is distributed under the 
+PYTHON SOFTWARE FOUNDATION LICENSE VERSION 2.
 
-// This file is not originally from the CPython distribution. But it does contain mostly example code
-// from the Python docs. Also dual licensing just for this one file seemed silly.
+This file is not originally from the CPython distribution. But it does 
+contain mostly example code from the Python docs. Also dual licensing just 
+for this one file seemed silly.
+*/
 
 #define PY_SSIZE_T_CLEAN
 #include <Python.h>
@@ -27,17 +30,18 @@ PyInit__isal(void)
     PyObject *m;
 
     m = PyModule_Create(&_isal_module);
-    if (m == NULL)
+    if (m == NULL) {
         return NULL;
-
+    }
     PyModule_AddIntMacro(m, ISAL_MAJOR_VERSION);
     PyModule_AddIntMacro(m, ISAL_MINOR_VERSION);
     PyModule_AddIntMacro(m, ISAL_PATCH_VERSION);
 
     PyObject *isal_version = PyUnicode_FromFormat(
         "%d.%d.%d", ISAL_MAJOR_VERSION, ISAL_MINOR_VERSION, ISAL_PATCH_VERSION);  
-    if (isal_version == NULL)
+    if (isal_version == NULL) {
         return NULL;
+    }
     PyModule_AddObject(m, "ISAL_VERSION", isal_version);
     return m;
 }
diff --git a/src/isal/igzip_libmodule.c b/src/isal/igzip_libmodule.c
@@ -1,45 +1,47 @@
-//  Copyright (c) 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010,
-// 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021, 2022
-// Python Software Foundation; All Rights Reserved
-
-// This file is part of python-isal which is distributed under the 
-// PYTHON SOFTWARE FOUNDATION LICENSE VERSION 2.
-
-// This file was modified from Cpython Modules/bz2module.c file from the 3.9
-// branch.
-
-// Changes compared to CPython:
-// - The BZ2Decompressor has been used as a basis for IgzipDecompressor.
-//   Functionality is almost the same. IgzipDecompressor does have a more
-//   elaborate __init__ to set settings. It also implements decompress_buf more
-//   akin to how decompression is implemented in isal_shared.h
-// - Constants were added that are particular to igzip_lib.
-// - Argument parsers were written using th CPython API rather than argument
-//   clinic.
+/* 
+Copyright (c) 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010,
+2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021, 2022
+Python Software Foundation; All Rights Reserved
+
+This file is part of python-isal which is distributed under the 
+PYTHON SOFTWARE FOUNDATION LICENSE VERSION 2.
+
+This file was modified from Cpython Modules/bz2module.c file from the 3.9
+branch.
+
+Changes compared to CPython:
+- The BZ2Decompressor has been used as a basis for IgzipDecompressor.
+  Functionality is almost the same. IgzipDecompressor does have a more
+  elaborate __init__ to set settings. It also implements decompress_buf more
+  akin to how decompression is implemented in isal_shared.h
+- Constants were added that are particular to igzip_lib.
+- Argument parsers were written using th CPython API rather than argument
+  clinic.
+*/
 
 #include "isal_shared.h"
 
 typedef struct {
     PyObject_HEAD
-    struct inflate_state state;
-    char eof;           /* T_BOOL expects a char */
     PyObject *unused_data;
     PyObject *zdict;
-    char needs_input;
     uint8_t *input_buffer;
     Py_ssize_t input_buffer_size;
-
     /* inflate_state>avail_in is only 32 bit, so we store the true length
        separately. Conversion and looping is encapsulated in
        decompress_buf() */
     Py_ssize_t avail_in_real;
+    char eof;           /* T_BOOL expects a char */
+    char needs_input;
+    /* Struct inflate state contains a massive buffer at the end. Put it at
+       the end of the IgzipDecompressor so members can be accessed easily. */
+    struct inflate_state state;
 } IgzipDecompressor;
 
 static void
 IgzipDecompressor_dealloc(IgzipDecompressor *self)
 {
-    if(self->input_buffer != NULL)
-        PyMem_Free(self->input_buffer);
+    PyMem_Free(self->input_buffer);
     Py_CLEAR(self->unused_data);
     Py_CLEAR(self->zdict);
     Py_TYPE(self)->tp_free((PyObject *)self);
@@ -62,16 +64,16 @@ decompress_buf(IgzipDecompressor *self, Py_ssize_t max_length)
     
     int err;
 
-    // In Python 3.10 sometimes sys.maxsize is passed by default. In those cases
-    // we do want to use DEF_BUF_SIZE as start buffer.
+    /* In Python 3.10 sometimes sys.maxsize is passed by default. In those cases
+       we do want to use DEF_BUF_SIZE as start buffer. */
     if ((max_length < 0) || max_length == PY_SSIZE_T_MAX) {
         hard_limit = PY_SSIZE_T_MAX;
         obuflen = DEF_BUF_SIZE;
     } else {
-        // Assume that decompressor is used in file decompression with a fixed
-        // block size of max_length. In that case we will reach max_length almost
-        // always (except at the end of the file). So it makes sense to allocate
-        // max_length.
+        /* Assume that decompressor is used in file decompression with a fixed
+           block size of max_length. In that case we will reach max_length almost
+           always (except at the end of the file). So it makes sense to allocate
+           max_length. */
         hard_limit = max_length;
         obuflen = max_length;
         if (obuflen > DEF_MAX_INITIAL_BUF_SIZE){
@@ -102,8 +104,10 @@ decompress_buf(IgzipDecompressor *self, Py_ssize_t max_length)
                 isal_inflate_error(err);
                 goto error;
             }
-        } while (self->state.avail_out == 0 && self->state.block_state != ISAL_BLOCK_FINISH);
-    } while(self->avail_in_real != 0 && self->state.block_state != ISAL_BLOCK_FINISH);
+        } while (self->state.avail_out == 0 && 
+                 self->state.block_state != ISAL_BLOCK_FINISH);
+    } while(self->avail_in_real != 0 && 
+            self->state.block_state != ISAL_BLOCK_FINISH);
 
     if (self->state.block_state == ISAL_BLOCK_FINISH)
         self->eof = 1;
@@ -189,7 +193,8 @@ decompress(IgzipDecompressor *self, uint8_t *data, size_t len, Py_ssize_t max_le
                 goto error;
             char * new_data_ptr = PyBytes_AS_STRING(new_data);
             bitbuffer_copy(&(self->state), new_data_ptr, bytes_in_bitbuffer);
-            memcpy(new_data_ptr + bytes_in_bitbuffer, self->state.next_in, self->avail_in_real);
+            memcpy(new_data_ptr + bytes_in_bitbuffer, self->state.next_in, 
+                   self->avail_in_real);
             Py_XSETREF(self->unused_data, new_data);
         }
     }
@@ -257,13 +262,14 @@ PyDoc_STRVAR(igzip_lib_compress__doc__,
 "    the header and trailer are controlled by the flag parameter.");
 
 #define IGZIP_LIB_COMPRESS_METHODDEF    \
-    {"compress", (PyCFunction)(void(*)(void))igzip_lib_compress, METH_VARARGS|METH_KEYWORDS, igzip_lib_compress__doc__}
+    {"compress", (PyCFunction)(void(*)(void))igzip_lib_compress, \
+     METH_VARARGS|METH_KEYWORDS, igzip_lib_compress__doc__}
 
 static PyObject *
 igzip_lib_compress(PyObject *module, PyObject *args, PyObject *kwargs)
 {
-    char *keywords[] = {"", "level", "flag", "mem_level", "hist_bits", NULL};
-    char *format ="y*|iiii:compress";
+    static char *keywords[] = {"", "level", "flag", "mem_level", "hist_bits", NULL};
+    static char *format ="y*|iiii:compress";
     Py_buffer data = {NULL, NULL};
     int level = ISAL_DEFAULT_COMPRESSION;
     int flag = COMP_DEFLATE;
@@ -298,13 +304,14 @@ PyDoc_STRVAR(igzip_lib_decompress__doc__,
 "    The initial output buffer size.");
 
 #define IGZIP_LIB_DECOMPRESS_METHODDEF    \
-    {"decompress", (PyCFunction)(void(*)(void))igzip_lib_decompress, METH_VARARGS|METH_KEYWORDS, igzip_lib_decompress__doc__}
+    {"decompress", (PyCFunction)(void(*)(void))igzip_lib_decompress, \
+     METH_VARARGS|METH_KEYWORDS, igzip_lib_decompress__doc__}
 
 static PyObject *
 igzip_lib_decompress(PyObject *module, PyObject *args, PyObject *kwargs)
 {
-    char *keywords[] = {"", "flag", "hist_bits", "bufsize", NULL};
-    char *format ="y*|iin:decompress";
+    static char *keywords[] = {"", "flag", "hist_bits", "bufsize", NULL};
+    static char *format ="y*|iin:decompress";
     Py_buffer data = {NULL, NULL};
     int flag = DECOMP_DEFLATE;
     int hist_bits = ISAL_DEF_MAX_HIST_BITS;
@@ -315,7 +322,7 @@ igzip_lib_decompress(PyObject *module, PyObject *args, PyObject *kwargs)
             &data, &flag, &hist_bits, &bufsize)) {
         return NULL;
     }
-    PyObject * return_value = igzip_lib_decompress_impl(&data, flag, hist_bits, bufsize);
+    PyObject *return_value = igzip_lib_decompress_impl(&data, flag, hist_bits, bufsize);
     PyBuffer_Release(&data);
     return return_value;
 }
@@ -340,13 +347,16 @@ PyDoc_STRVAR(igzip_lib_IgzipDecompressor_decompress__doc__,
 "the unused_data attribute.");
 
 #define IGZIP_LIB_IGZIPDECOMPRESSOR_DECOMPRESS_METHODDEF    \
-    {"decompress", (PyCFunction)(void(*)(void))igzip_lib_IgzipDecompressor_decompress, METH_VARARGS|METH_KEYWORDS, igzip_lib_IgzipDecompressor_decompress__doc__}
+    {"decompress", (PyCFunction)(void(*)(void))igzip_lib_IgzipDecompressor_decompress, \
+     METH_VARARGS|METH_KEYWORDS, igzip_lib_IgzipDecompressor_decompress__doc__}
 
 static PyObject *
-igzip_lib_IgzipDecompressor_decompress(IgzipDecompressor *self, PyObject *args, PyObject *kwargs)
+igzip_lib_IgzipDecompressor_decompress(IgzipDecompressor *self, 
+                                       PyObject *args, 
+                                       PyObject *kwargs)
 {
-    char *keywords[] = {"", "max_length", NULL};
-    char *format = "y*|n:decompress";
+    static char *keywords[] = {"", "max_length", NULL};
+    static char *format = "y*|n:decompress";
     Py_buffer data = {NULL, NULL};
     Py_ssize_t max_length = -1;
 
@@ -383,8 +393,8 @@ igzip_lib_IgzipDecompressor__new__(PyTypeObject *type,
                                    PyObject *args, 
                                    PyObject *kwargs)
 {
-    char *keywords[] = {"flag", "hist_bits", "zdict", NULL};
-    char *format = "|iiO:IgzipDecompressor";
+    static char *keywords[] = {"flag", "hist_bits", "zdict", NULL};
+    static char *format = "|iiO:IgzipDecompressor";
     int flag = ISAL_DEFLATE;
     int hist_bits = ISAL_DEF_MAX_HIST_BITS;
     PyObject *zdict = NULL;
@@ -458,8 +468,9 @@ static PyMemberDef IgzipDecompressor_members[] = {
      READONLY, IgzipDecompressor_unused_data__doc__},
     {"needs_input", T_BOOL, offsetof(IgzipDecompressor, needs_input), READONLY,
      IgzipDecompressor_needs_input_doc},
-    {"crc", T_UINT, offsetof(IgzipDecompressor, state) + offsetof(struct inflate_state, crc), READONLY,
-     IgzipDecompressor_crc_doc},
+    {"crc", T_UINT, 
+     offsetof(IgzipDecompressor, state) + offsetof(struct inflate_state, crc), 
+     READONLY, IgzipDecompressor_crc_doc},
     {NULL}
 };
 
@@ -580,10 +591,12 @@ PyInit_igzip_lib(void)
         return NULL;
     }
 
-    if (PyType_Ready(&IgzipDecompressor_Type) != 0)
+    if (PyType_Ready(&IgzipDecompressor_Type) != 0) {
         return NULL;
+    }
     Py_INCREF(&IgzipDecompressor_Type);
-    if (PyModule_AddObject(m, "IgzipDecompressor",  (PyObject *)&IgzipDecompressor_Type) < 0) {
+    if (PyModule_AddObject(m, "IgzipDecompressor",  
+                           (PyObject *)&IgzipDecompressor_Type) < 0) {
         return NULL;
     }
 
diff --git a/src/isal/isal_shared.h b/src/isal/isal_shared.h
@@ -1,23 +1,24 @@
-//  Copyright (c) 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010,
-// 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021, 2022
-// Python Software Foundation; All Rights Reserved
-
-// This file is part of python-isal which is distributed under the 
-// PYTHON SOFTWARE FOUNDATION LICENSE VERSION 2.
-
-// This file was modified from Cpython Modules/zlibmodule.c file from the 3.9
-// branch. This is because the BlocksBuffer used in Python 3.10 and higher is
-// not available in python 3.7-3.9 which this project supports.
-
-// Changes compared to CPython:
-// - igzip_lib.compress and igzip_lib.decompress are equivalent to
-//   zlib.compress and zlib.decompress except that these use a 'flag' and
-//   'hist_bits' argument to set compression headers and trailers and window
-//   size respectively. The igzip_lib functions also offer more control by
-//   allowing to set no header, but include the trailer.
-// - This file also includes some utility functions to set parameters on ISA-L
-//   structs.
-
+/* 
+Copyright (c) 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010,
+2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021, 2022
+Python Software Foundation; All Rights Reserved
+
+This file is part of python-isal which is distributed under the 
+PYTHON SOFTWARE FOUNDATION LICENSE VERSION 2.
+
+This file was modified from Cpython Modules/zlibmodule.c file from the 3.9
+branch. This is because the BlocksBuffer used in Python 3.10 and higher is
+not available in python 3.7-3.9 which this project supports.
+
+Changes compared to CPython:
+- igzip_lib.compress and igzip_lib.decompress are equivalent to
+  zlib.compress and zlib.decompress except that these use a 'flag' and
+  'hist_bits' argument to set compression headers and trailers and window
+  size respectively. The igzip_lib functions also offer more control by
+  allowing to set no header, but include the trailer.
+- This file also includes some utility functions to set parameters on ISA-L
+  structs.
+*/
 #define PY_SSIZE_T_CLEAN
 #include <Python.h>
 #include "structmember.h"         // PyMemberDef
@@ -82,10 +83,11 @@ static const uint32_t LEVEL_BUF_SIZES[24] = {
     ISAL_DEF_LVL3_EXTRA_LARGE
 };
 
-static int mem_level_to_bufsize(int compression_level, int mem_level,
+static inline int mem_level_to_bufsize(int compression_level, int mem_level,
                                 uint32_t * bufsize)
 {
-    if (compression_level < 0 || compression_level > 3 || mem_level < MEM_LEVEL_DEFAULT || mem_level > MEM_LEVEL_EXTRA_LARGE) {
+    if (compression_level < 0 || compression_level > 3 || 
+        mem_level < MEM_LEVEL_DEFAULT || mem_level > MEM_LEVEL_EXTRA_LARGE) {
         *bufsize = 0; return -1;
     }
     *bufsize = LEVEL_BUF_SIZES[compression_level * 6 + mem_level];
@@ -121,7 +123,8 @@ static void isal_inflate_error(int err){
     else if (err == ISAL_INVALID_SYMBOL) msg = "Invalid deflate symbol found";
     else if (err == ISAL_INVALID_LOOKBACK) msg = "Invalid lookback distance found";
     else if (err == ISAL_INVALID_WRAPPER) msg = "Invalid gzip/zlib wrapper found";
-    else if (err == ISAL_UNSUPPORTED_METHOD) msg = "Gzip/zlib wrapper specifies unsupported compress method";
+    else if (err == ISAL_UNSUPPORTED_METHOD) msg = "Gzip/zlib wrapper specifies" 
+                                                   "unsupported compress method";
     else if (err == ISAL_INCORRECT_CHECKSUM) msg = "Incorrect checksum found";
     else msg = "Unknown error";
 
@@ -135,7 +138,7 @@ static void isal_inflate_error(int err){
  * @param state An inflate_state
  * @return size_t 
  */
-static size_t bitbuffer_size(struct inflate_state *state){
+static inline size_t bitbuffer_size(struct inflate_state *state){
     return state->read_in_length / 8;
 }
 
@@ -162,7 +165,7 @@ static int bitbuffer_copy(struct inflate_state *state, char *to, size_t n){
     return 0;
 }
 
-static void
+static inline void
 arrange_input_buffer(uint32_t *avail_in, Py_ssize_t *remains)
 {
     *avail_in = (uint32_t)Py_MIN((size_t)*remains, UINT32_MAX);
@@ -208,7 +211,7 @@ arrange_output_buffer_with_maximum(uint32_t *avail_out,
     return length;
 }
 
-static Py_ssize_t
+static inline Py_ssize_t
 arrange_output_buffer(uint32_t *avail_out,
                       uint8_t **next_out,
                       PyObject **buffer,
@@ -268,7 +271,8 @@ igzip_lib_compress_impl(Py_buffer *data,
         else zst.flush = NO_FLUSH;
 
         do {
-            obuflen = arrange_output_buffer(&(zst.avail_out), &(zst.next_out), &RetVal, obuflen);
+            obuflen = arrange_output_buffer(&(zst.avail_out), &(zst.next_out), 
+                                            &RetVal, obuflen);
             if (obuflen < 0) {
                 PyErr_SetString(PyExc_MemoryError,
                         "Unsufficient memory for buffer allocation");
diff --git a/src/isal/isal_zlibmodule.c b/src/isal/isal_zlibmodule.c
