gh-132983: Introduce `_zstd` bindings module #133027

emmatyping · 2025-04-26T19:38:51Z

This is part 2 (but parallel to part 1) for implementing PEP 784. This is probably the meat of the implementation making up ~half of the LOC in the overall diff.

This change:

Adds the pyzstd license, since this is the first code introduced based on pyzstd
Adds the code for the _zstd module in Modules/_zstd. Tests will be included when the Python compression.zstd module is added. If people think the tests should be included, I can merge the compression.zstd changes into the PR, but I wanted to separate them to make the PRs a bit more manageable.
Adds unix build changes (Windows requires changes in cpython-source-deps and will be added in a follow-up PR)

I added skip news as I'd like to write a holistic NEWS/What's New entry once the entire implementation has landed. If people think each PR should have NEWS I can write something up.

TODO before merge:

try running tests on top of this branch
format PEP 7 style
run buildbots on this PR
verify _zstd gets installed

Issue: Implement PEP 784 - Adding Zstandard to the Python standard library #132983

📚 Documentation preview 📚: https://cpython-previews--133027.org.readthedocs.build/

emmatyping · 2025-04-26T19:47:42Z

~~Hm, I'll dig into the C global failure. Didn't show up when the tests ran on the branch :(~~

E: now fixed

ZeroIntensity

Nice! Some comments at a glance.

Please remove the module state macros. They make this really difficult to review, and don't really add much (is MS_MEMBER(whatever) really that much easier to type than state->whatever?)

Modules/_zstd/zdict.c

Modules/_zstd/_zstdmodule.h

Modules/_zstd/compressor.c

Modules/_zstd/_zstdmodule.c

Modules/_zstd/compressor.c

emmatyping · 2025-04-26T19:58:34Z

Thank you for the review @ZeroIntensity! Some of the decisions are just how things were in pyzstd, so I expect they very well should change for merging the code in. I'll go through your review and read up on ob_lock.

ZeroIntensity · 2025-04-26T20:00:20Z

There's probably not too much to read about it other than somewhere in PEP-703. You'll just want to replace the locks with critical sections, which have some magic to prevent deadlocks.

Doc/license.rst

Modules/_zstd/_zstdmodule.h

StanFromIreland · 2025-04-27T09:39:25Z

Modules/_zstd/_zstdmodule.h

+
+Original implementation by Ma Lin with patches by Rogdham.
+
+Refactored for the CPython standard library by Emma Harper Smith.


We have generally stopped updating these but I don't know about new ones. With blame there is not much need.

I think it is nice to credit where code comes from (especially since blame will only show me), but I am fine removing this if that is the convention.

especially since blame will only show me

Perhaps add git trailers to credit Lin & Rodgham? Originally-Authored-By/Authored-By/etc.

I think the originally part we can leave but the new part should go.

adding them to the commit wouldn’t hurt either.

I'll leave the module description and remove the rest. Perhaps when merged this PR can include the git trailers.

Okay these should all be updated now.

Modules/_zstd/zdict.c

picnixz

Few preliminary comments, although I'm not sure it can be modified under the given license.

Modules/_zstd/_zstdmodule.c

picnixz

Some other comments I managed to find. I didn't review the decompressor but I think most of my comments that I said for the compressor would apply. The most important ones are essentially function signatures where I want you to use PyObject *self and do a manual cast to the expected type inside the function:

#define MyType_CAST(op)	((MyType *)(op))

static void
my_method(PyObject *op) {
    MyType *self = MyType_CAST(op);
    ...
}

The use of a macro (or a static inline function, up to you) is to allow future runtime type checks.

picnixz · 2025-04-27T21:14:53Z

Modules/_zstd/_zstdmodule.c

+}
+
+typedef struct {
+    const int parameter;


General comment: we don't use const T when T is a primitive type such as int even to convene the intent of a constant variable.

Is this true in argument positions too? e.g. should I remove the const Py_ssize_t from buffer.h?

I think we have a bit of both.

Benefits:

avoid mistakes

convene the intent of a read-only value

Downsides:

adds more words to read

for a function parameter, it does nothing except for a pointer where it makes sense (it helps in its usage but it doesn't help more the compiler)

It's really up to you though, but I would say that it'd be good to be consistent in the const usage. I however haven't looked in all places to see if structures are declared with a const or not.

But we can discuss this in a separate PR.

Thanks for the context. I will think about it. If the tradition is to not have it in functions or structs then I think it would be good to remove them throughout.

picnixz · 2025-04-27T21:16:19Z

Modules/_zstd/_zstdmodule.c

+        list = cp_list;
+        list_size = Py_ARRAY_LENGTH(cp_list);
+        type = "compression";
+    } else {


Nit, but can you (in a follow-up PR if you want to) make it so that the new code follow PEP-7? this includes writing something like:

if (...) { ... } else { ... }

I plan on making the code PEP 7 compliant prior to merge, but want to work through the semantic/functional issues first.

picnixz · 2025-04-27T21:17:51Z

Modules/_zstd/_zstdmodule.c

+/*[clinic input]
+_zstd._train_dict
+
+        samples_bytes: PyBytesObject


Can you use 4-space indents here instead of 8 spaces in general? TiA.

Modules/_zstd/_zstdmodule.c

Modules/_zstd/buffer.h

Modules/_zstd/compressor.c

picnixz · 2025-04-27T21:42:52Z

Modules/_zstd/compressor.c

+}
+
+static void
+ZstdCompressor_dealloc(ZstdCompressor *self)


Methods for PyObjects must always take a PyObject * as self and make the cast inside otherwise you'll get an "incorrect function signature" as it's an undefined behavior if we call this function with a PyObject* at runtime.

Modules/_zstd/zdict.c

Doc/license.rst

AA-Turner · 2025-04-27T21:52:26Z

Modules/_zstd/_zstdmodule.c

+            PyErr_SetString(PyExc_ValueError,
+                            "Items in samples_size_list should be an int "
+                            "object, with a size_t value.");


If this exception can be triggered by Python code, we should avoid using C types in it.

I went through all of the exception messages and tried to make sure this was done consistently. Hopefully they look better now!

Modules/_zstd/_zstdmodule.c

This commit introduces the `_zstd` module, with bindings to libzstd from the pyzstd project. It also includes the unix build system configuration. Windows build system support will be integrated independently as it depends on integration with cpython-source-deps.

Also removes module state references from the classes in the _zstd module and instead uses PyType_GetModuleState()

Co-authored-by: Stan Ulbrych <[email protected]>

This should avoid races and deadlocks.

The `compress`/`decompress` functions will be moved to Python code for simplicity. C implementations can always be re-added in the future. Also, mark _zstd as not requiring the GIL.

Co-authored-by: Adam Turner <[email protected]>

... for `get_zstd_state_from_type` Co-authored-by: Bénédikt Tran <[email protected]>

PyModule_AddObject is soft-deprecated, so we should use PyModule_AddObjectRef

Co-authored-by: Bénédikt Tran <[email protected]>

We should instead document that to pickle a dictionary a user should use the `.dict_content` attribute.

emmatyping added the skip news label Apr 26, 2025

emmatyping requested a review from gpshead April 26, 2025 19:38

emmatyping requested review from erlend-aasland and corona10 as code owners April 26, 2025 19:38

bedevere-app bot added the awaiting review label Apr 26, 2025

bedevere-app bot mentioned this pull request Apr 26, 2025

Implement PEP 784 - Adding Zstandard to the Python standard library #132983

Open

5 tasks

emmatyping changed the title ~~gh-132983: Introduce _zstd module~~ gh-132983: Introduce _zstd bindings module Apr 26, 2025

ZeroIntensity reviewed Apr 26, 2025

View reviewed changes

emmatyping requested a review from ericsnowcurrently as a code owner April 26, 2025 21:01

StanFromIreland reviewed Apr 27, 2025

View reviewed changes

picnixz reviewed Apr 27, 2025

View reviewed changes

AA-Turner reviewed Apr 27, 2025

View reviewed changes

Doc/license.rst Outdated Show resolved Hide resolved

AA-Turner reviewed Apr 27, 2025

View reviewed changes

Modules/_zstd/_zstdmodule.c Outdated Show resolved Hide resolved

picnixz mentioned this pull request Apr 27, 2025

Show value of UINT32_MAX in HMAC error messages #133061

Closed

emmatyping and others added 11 commits April 28, 2025 17:52

Add _zstd module

9814e3b

This commit introduces the `_zstd` module, with bindings to libzstd from the pyzstd project. It also includes the unix build system configuration. Windows build system support will be integrated independently as it depends on integration with cpython-source-deps.

Add _zstd to modules

fda87c8

Fix path for compression.zstd module

887e564

Ignore _zstd module like _io

cdba656

Expand module state macros to improve code quality

6b67e9b

Also removes module state references from the classes in the _zstd module and instead uses PyType_GetModuleState()

Remove backticks suggested in review

a99a5d2

Co-authored-by: Stan Ulbrych <[email protected]>

Use critical sections to lock object state

02cd17a

This should avoid races and deadlocks.

Remove compress/decompress and mark module as not reliant on the GIL

54eca74

The `compress`/`decompress` functions will be moved to Python code for simplicity. C implementations can always be re-added in the future. Also, mark _zstd as not requiring the GIL.

Lift critical section to avoid clang warning

f605956

Respond to comments by picnixz

2eadc65

Call out pyzstd explicitly in license description

8eac354

Co-authored-by: Adam Turner <[email protected]>

emmatyping and others added 15 commits April 28, 2025 17:52

Use a much more robust implementation...

26775be

... for `get_zstd_state_from_type` Co-authored-by: Bénédikt Tran <[email protected]>

Use PyList_GetItemRef for thread safety purposes

eae460f

Use a macro for the minimum supported version

2ab5e4a

remove const from primivite types

d5bf1c1

Use PyMem_New in another spot

9e92b9f

Simplify error handling in _get_frame_size

47f815a

Another simplification of error handling in get_frame_info

6a4f7b8

Rename _module_state to mod_state

d7b3805

Rewrite comment explaining the context of the code

c225ea6

Add link to pyzstd

6e8c61c

Add TODO about refactoring dict training code

e52ad06

Use PyModule_AddObjectRef over PyModule_AddObject

2a1ad8b

PyModule_AddObject is soft-deprecated, so we should use PyModule_AddObjectRef

Check result of OutputBufferGrow

94473b9

Simplify return logic in add_constant_to_type

e2b2515

Co-authored-by: Bénédikt Tran <[email protected]>

Ignore return value of _zstd_clear()

cd2f085

Co-authored-by: Bénédikt Tran <[email protected]>

emmatyping force-pushed the 3.14-zstd-c-code branch from da9c225 to cd2f085 Compare April 29, 2025 00:52

emmatyping added 5 commits April 28, 2025 17:57

Remove redundant comments

79e174f

Remove __reduce__ from ZstdDict

ce6f79c

We should instead document that to pickle a dictionary a user should use the `.dict_content` attribute.

Use PyUnicode_FromFormat instead of a buffer

e15dd85

Don't use C constants/types in error messages

685a3d1

Make error messages easier to understand for Python users

1b9f786

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-132983: Introduce `_zstd` bindings module #133027

gh-132983: Introduce `_zstd` bindings module #133027

emmatyping commented Apr 26, 2025 •

edited

Loading

emmatyping commented Apr 26, 2025 •

edited

Loading

ZeroIntensity left a comment

emmatyping commented Apr 26, 2025

ZeroIntensity commented Apr 26, 2025

StanFromIreland Apr 27, 2025

emmatyping Apr 27, 2025

AA-Turner Apr 27, 2025

StanFromIreland Apr 27, 2025

emmatyping Apr 27, 2025

emmatyping Apr 29, 2025

picnixz left a comment

picnixz left a comment

picnixz Apr 27, 2025

emmatyping Apr 27, 2025

picnixz Apr 27, 2025 •

edited

Loading

emmatyping Apr 27, 2025

picnixz Apr 27, 2025

emmatyping Apr 27, 2025

picnixz Apr 27, 2025

picnixz Apr 27, 2025

AA-Turner Apr 27, 2025

emmatyping Apr 29, 2025


		Original implementation by Ma Lin with patches by Rogdham.

		Refactored for the CPython standard library by Emma Harper Smith.

gh-132983: Introduce _zstd bindings module #133027

Are you sure you want to change the base?

gh-132983: Introduce _zstd bindings module #133027

Conversation

emmatyping commented Apr 26, 2025 • edited Loading

emmatyping commented Apr 26, 2025 • edited Loading

ZeroIntensity left a comment

Choose a reason for hiding this comment

emmatyping commented Apr 26, 2025

ZeroIntensity commented Apr 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz left a comment

Choose a reason for hiding this comment

picnixz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz Apr 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gh-132983: Introduce `_zstd` bindings module #133027

gh-132983: Introduce `_zstd` bindings module #133027

emmatyping commented Apr 26, 2025 •

edited

Loading

emmatyping commented Apr 26, 2025 •

edited

Loading

picnixz Apr 27, 2025 •

edited

Loading