NotificationsYou must be signed in to change notification settings
Fork32k
Star67.3k

gh-132983: Minor fixes and clean up for the _zstd module#134930

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

serhiy-storchaka wants to merge7 commits intopython:main

base:main

Choose a base branch

fromserhiy-storchaka:zstd-fixes

Open

gh-132983: Minor fixes and clean up for the _zstd module#134930

serhiy-storchaka wants to merge7 commits intopython:mainfromserhiy-storchaka:zstd-fixes

+139 −147

Conversation

Copy link

Member

serhiy-storchaka commentedMay 30, 2025•
edited by bedevere-appbot
Loading

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

pythongh-132983: Minor fixes and clean up for the _zstd module

9aec708

serhiy-storchaka requested review fromAA-Turner andemmatyping

May 30, 2025 14:22

serhiy-storchaka added skip news needs backport to 3.14bugs and security fixes labels

May 30, 2025

bedevere-appbot added the awaiting core review label

May 30, 2025

bedevere-appbot mentioned this pull request

May 30, 2025

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

Closed

13 tasks

emmatyping reviewed

May 30, 2025

View reviewed changes

Copy link

Member

emmatyping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

One question and one concern, but the rest looks good! Thank you for the improvements :)

Modules/_zstd/compressor.c Outdated

		ret = PyDict_SetItem(self->c_dicts, level, capsule);
		/* Add PyCapsule object to self->c_dicts if it is not already present. */
		PyObject *result;
		ret = PyDict_SetDefaultRef(self->c_dicts, level, capsule, &result);

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Out of curiousity, isPyDict_SetDefaultRef the "correct" way to set items in a dictionary for new code?

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The GIL is released after checking that there is no entry for level in self->c_dicts. After it acquired again, it may happen that a new entry has already been added. In that case we should use already added capsule object.

Copy link

Member

emmatypingMay 31, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

self is guarded by a lock, so no other object can modify it while it is locked. We don't use critical sections so I believe we don't unlock when the GIL is released. (Similarly this is true below)

Copy link

MemberAuthor

serhiy-storchakaMay 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Well, then I'll remove this change.

Although I wonder if it's possible to get rid of the lock here and if it would be of any use.

Copy link

Member

emmatypingMay 31, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, I think it might be possible to remove the lock. The dict_buffer/len are immutable, so the only concern would likely be actually setting the c_dicts member. Perhaps this can be investigated in a follow up?

Copy link

MemberAuthor

serhiy-storchakaMay 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

For now, I removed these changes. We can investigate later if there is a benefit of removing the lock for these calls.

Modules/_zstd/decompressor.c Outdated

Comment on lines 69 to 71

		if (self->d_dict!= NULL) {
		ZSTD_freeDDict(ret);
		}

Copy link

Member

emmatypingMay 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This check doesn't look quite right. We're already in a block whereself->d_dict == NULL.

Copy link

MemberAuthor

serhiy-storchakaMay 30, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is the same case as above, but simpler. After the GIL was released and acquired, we cannot already assume that self->d_dict == NULL.

Copy link

Member

emmatypingMay 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Similar to the above, perhaps this could be addressed in a follow-up? (or if you'd like to tackle removing locking from ZstdDict here I suppose that's fine)

AA-Turner reviewed

May 31, 2025

View reviewed changes

Modules/_zstd/_zstdmodule.c

Comment on lines -510 to -515

		if (!PyType_Check(c_parameter_type) \|\| !PyType_Check(d_parameter_type)) {
		PyErr_SetString(PyExc_ValueError,
		"The two arguments should be CompressionParameter and "
		"DecompressionParameter types.");
		return NULL;
		}

Copy link

Member

AA-TurnerMay 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Sanity check on removing the type checks here?

Copy link

Member

emmatypingMay 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The type checks are already handled by argument clinic, so these are actually redundant.

Modules/_zstd/compressor.c OutdatedShow resolvedHide resolved

Modules/_zstd/decompressor.c OutdatedShow resolvedHide resolved

serhiy-storchakaand others added3 commits

May 31, 2025 21:56

Remove defensive double checks after releasing the GIL.

0251f9e

Apply suggestions from code review

710a2b3

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

Merge remote-tracking branch 'refs/remotes/origin/zstd-fixes' into zs…

88e9c0a

…td-fixes

mostafaammer added this tolavitaconnect@MOSTAFAAMMER

May 31, 2025

serhiy-storchaka added3 commits

May 31, 2025 22:35

Refactor _zstd_load_c_dict and _zstd_load_d_dict.

cffe18b

ts

0dd3b2a

Merge branch 'main' into zstd-fixes

4877c81

emmatyping approved these changes

May 31, 2025

View reviewed changes

Copy link

Member

emmatyping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The changes look good! One minor wording suggestion but great otherwise. Thank you!

Modules/_zstd/_zstdmodule.c


		/* Wrong type */
		PyErr_SetString(PyExc_TypeError,
		"zstd_dict argument should be ZstdDict object.");

Copy link

Member

emmatypingMay 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

	"zstd_dict argument should be ZstdDict object.");
	"zstd_dict argument should beaZstdDict object.");

Modules/_zstd/_zstdmodule.c

Comment on lines +22 to +23

		ZstdDict*
		_Py_parse_zstd_dict(const_zstd_statestate,PyObjectdict,int*ptype)

Copy link

Member

emmatypingMay 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is a great refactor, thank you!

Labels

awaiting core review needs backport to 3.14

bugs and security fixes

skip news

3 participants

Movatterモバイル変換

Uh oh!

gh-132983: Minor fixes and clean up for the _zstd module#134930

Are you sure you want to change the base?

gh-132983: Minor fixes and clean up for the _zstd module#134930

Conversation

serhiy-storchaka commentedMay 30, 2025• edited by bedevere-appbotLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

emmatyping left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emmatypingMay 31, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emmatypingMay 31, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchakaMay 30, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

emmatyping left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

serhiy-storchaka commentedMay 30, 2025•
edited by bedevere-appbot
Loading

emmatypingMay 31, 2025•
edited
Loading

emmatypingMay 31, 2025•
edited
Loading

serhiy-storchakaMay 30, 2025•
edited
Loading