gh-132983: Convert dict_content to take Py_buffer #133924
Open
+90
−63
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
11 tasks
AA-Turner
commented
May 12, 2025
Comment on lines
+68
to
+73
| self->dict_buffer = PyMem_RawMalloc(dict_content->len); | ||
| if (!self->dict_buffer) { | ||
| return PyErr_NoMemory(); | ||
| } | ||
| memcpy(self->dict_buffer, dict_content->buf, dict_content->len); | ||
| self->dict_len = dict_content->len; |
There was a problem hiding this comment.
cc @vstinner, I think RawMalloc is correct here but would be grateful for a review / confirmation please
There was a problem hiding this comment.
It seems like you told the GIL in this code, so you can use PyMem_Malloc() instead. The Raw variant is when you don't hold the GIL.
| @@ -60,24 +59,28 @@ _zstd_ZstdDict_new_impl(PyTypeObject *type, PyObject *dict_content, | |||
| } | |||
|
|
|||
| /* Check dict_content's type */ | |||
| self->dict_content = PyBytes_FromObject(dict_content); | |||
| if (self->dict_content == NULL) { | |||
| if (dict_content == NULL) { | |||
There was a problem hiding this comment.
It is never NULL.
BTW, overriding arbitrary exception with a TypeError in the current code is wrong.
| if (Py_SIZE(self->dict_content) < 8) { | ||
| self->dict_buffer = PyMem_RawMalloc(dict_content->len); | ||
| if (!self->dict_buffer) { | ||
| return PyErr_NoMemory(); |
| @@ -124,23 +124,33 @@ PyDoc_STRVAR(ZstdDict_dictid_doc, | |||
| "The special value '0' means a 'raw content' dictionary," | |||
| "without any restrictions on format or content."); | |||
|
|
|||
| PyDoc_STRVAR(ZstdDict_dictcontent_doc, | |||
| "The content of a Zstandard dictionary, as a bytes object."); | |||
|
|
|||
| static PyObject * | |||
| ZstdDict_str(PyObject *ob) | |||
There was a problem hiding this comment.
It is wrong to implement __str__. __repr__ should be implemented.
| static PyObject * | ||
| ZstdDict_str(PyObject *ob) | ||
| { | ||
| ZstdDict *dict = ZstdDict_CAST(ob); | ||
| return PyUnicode_FromFormat("<ZstdDict dict_id=%u dict_size=%zd>", | ||
| dict->dict_id, Py_SIZE(dict->dict_content)); | ||
| dict->dict_id, dict->dict_len); |
There was a problem hiding this comment.
Suggested change
| dict->dict_id, dict->dict_len); | |
| (unsigned int)dict->dict_id, dict->dict_len); |
| self->dict_len = dict_content->len; | ||
|
|
||
| /* Both ordinary and "raw content" dictionaries must be 8 bytes minimum */ | ||
| if (self->dict_len < 8) { |
There was a problem hiding this comment.
This can be checked at the beginning, before any allocations.
| } | ||
|
|
||
| self->dict_content = NULL; | ||
| self->d_dict = NULL; |
There was a problem hiding this comment.
Suggested change
| self->d_dict = NULL; | |
| self->d_dict = NULL; | |
| self->dict_buffer = NULL; |
Otherwise self->dict_buffer is not initialized in the destructor.
Comment on lines
+67
to
71
| char *dict_buffer = self->dict_buffer; | ||
| Py_ssize_t dict_len = self->dict_len; | ||
| Py_BEGIN_ALLOW_THREADS | ||
| self->d_dict = ZSTD_createDDict(dict_buffer, | ||
| dict_len); |
There was a problem hiding this comment.
Just inline them, as in the code below.
Suggested change
| char *dict_buffer = self->dict_buffer; | |
| Py_ssize_t dict_len = self->dict_len; | |
| Py_BEGIN_ALLOW_THREADS | |
| self->d_dict = ZSTD_createDDict(dict_buffer, | |
| dict_len); | |
| Py_BEGIN_ALLOW_THREADS | |
| self->d_dict = ZSTD_createDDict(self->dict_buffer, self->dict_len); |
Comment on lines
+173
to
177
| char *dict_buffer = self->dict_buffer; | ||
| Py_ssize_t dict_len = self->dict_len; | ||
| Py_BEGIN_ALLOW_THREADS | ||
| cdict = ZSTD_createCDict(dict_buffer, | ||
| dict_len, |
There was a problem hiding this comment.
Suggested change
| char *dict_buffer = self->dict_buffer; | |
| Py_ssize_t dict_len = self->dict_len; | |
| Py_BEGIN_ALLOW_THREADS | |
| cdict = ZSTD_createCDict(dict_buffer, | |
| dict_len, | |
| Py_BEGIN_ALLOW_THREADS | |
| cdict = ZSTD_createCDict(self->dict_buffer, self->dict_len, |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ZstdDict'sdict_contentis just binary data, we don't need to allocate an entire PyBytesObject for it. This also lets us use thePy_bufferargument clinic converter.We replace the
dict_contentmember ofZstdDictwithdict_bufferanddict_len.ZstdDict.dict_contentis still exposed as a bytes object to Python via a new getter.A
cc @Rogdham