Navigation Menu

Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@cython.trashcan directive to enable the Python trashcan for deallocations #2842

Merged
merged 1 commit into from Feb 18, 2019

Conversation

jdemeyer
Copy link
Contributor

@jdemeyer jdemeyer commented Feb 14, 2019

Fix #1354

This enables a @cython.trashcan directive which can enable using the Python trashcan for deallocations. This is optional, disabled by default (but one could argue about making it enabled by default).

Also see https://bugs.python.org/issue35983

Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, but I'll wait for a reaction on the corresponding PR on CPython side.

@@ -74,6 +74,41 @@ static int __Pyx_PyType_Ready(PyTypeObject *t) {
return r;
}

/////////////// TRASHCAN.proto ///////////////
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to scream. ;-) Just name it PyTrashcan or PyTrashcanSupport.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to scream. ;-) Just name it PyTrashcan or PyTrashcanSupport.

I prefer to use TRASHCAN in the name because the upstream macros are also called Py_TRASHCAN_SAFE_BEGIN and Py_TRASHCAN_SAFE_END. It will be more confusing if Cython uses a different made-up name.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was referring to the name of the utility code snippet, not the C macros (where upper case is normal). There is no precedent for an all-upper-case utility code name.

// Unlike the Py_TRASHCAN_SAFE_BEGIN/Py_TRASHCAN_SAFE_END macros, they
// allow dealing correctly with subclasses.
//
// This requires Python version >= 2.7.4 or >= 3.2.4
Copy link
Contributor

Choose a reason for hiding this comment

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

We could increase the minimum version for Cy3.0 iff this feature is used, i.e. we can make it fail to compile with a C #error right here if PY_VERSION_HEX < 2.7.4.
But I'd rather turn the macros into no-ops for older versions. In 99% of the cases, that's still better than not working at all. I mean, you can currently get a crash with Py2.7.3 in rare situations, and you'd get a crash in the future for them. Nothing lost, really.


// These macros are taken from https://github.com/python/cpython/pull/11841
// Unlike the Py_TRASHCAN_SAFE_BEGIN/Py_TRASHCAN_SAFE_END macros, they
// allow dealing correctly with subclasses.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider using the original macros in Py3.8+ if/when that contains the fixes.
I also wonder if PyPy's cpyext supports the trashcan at all, or if this is a CPython-only feature (for now?).

Cython/Utility/ExtensionTypes.c Outdated Show resolved Hide resolved
tests/run/trashcan.pyx Outdated Show resolved Hide resolved
Cython/Compiler/Symtab.py Outdated Show resolved Hide resolved
Cython/Compiler/ModuleNode.py Outdated Show resolved Hide resolved
@jdemeyer
Copy link
Contributor Author

Concerning testing: it's hard to test that the trashcan is not used: the only observable effect is a stack overflow (typically leading to a segmentation fault).

@jdemeyer
Copy link
Contributor Author

jdemeyer commented Feb 15, 2019

Actually, I spoke too quick: the order of deallocations between base class and subclass is different, so trashcan usage can be detected.

@jdemeyer
Copy link
Contributor Author

PyPy doesn't use the trashcan (it supports the Py_TRASHCAN macros as no-op). So disabling them in Cython under PyPy is the right thing to do.

@jdemeyer
Copy link
Contributor Author

I think that I took care of all your comments.

tests/run/trashcan.pyx Outdated Show resolved Hide resolved
@scoder
Copy link
Contributor

scoder commented Feb 18, 2019

Perfect! I'll merge it right away to avoid bit-rot, because it is a clear improvement over not supporting the trashcan mechanism at all. If the CPython devs decide that they want thing different, we can adapt to it on our side later.

@scoder scoder added this to the 3.0 milestone Feb 18, 2019
@scoder scoder merged commit f781880 into cython:master Feb 18, 2019
@jdemeyer
Copy link
Contributor Author

Thanks! Should I take care of docs and changelog?

@scoder
Copy link
Contributor

scoder commented Feb 18, 2019

Please do. :)

@jdemeyer jdemeyer mentioned this pull request Feb 19, 2019
@embray
Copy link
Contributor

embray commented Feb 19, 2019

I finally took the time to understand this, and not that it matters (since this is already merged), but this makes sense to me now and I agree that it's useful to be able to (re-)use this mechanism when implementing Cython types. Thanks!

My thinking had been that it should be possible to re-implement something like this specifically for cypari, and while it could be done it would be much more of a PITA, when we can reuse the mechanism that's already there in the Python interpreter for exactly this purpose.

@embray
Copy link
Contributor

embray commented Feb 19, 2019

IIUC it should be possible to use this also without the additional feature in Cython, but it's certainly cleaner this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants