Add cy_function and fused_function to the shared utility module#7556
Add cy_function and fused_function to the shared utility module#7556da-woods merged 4 commits intocython:masterfrom
Conversation
Cython/Utility/CythonFunction.c
Outdated
| //////////////////// CythonFunctionAlways.proto ////////////////////////// | ||
| // This section always gets included whether we're using CythonFunction through | ||
| // shared utility code or not. |
There was a problem hiding this comment.
I'm not sure this is the best bit of naming ever, but a better name wasn't leaping out at me
There was a problem hiding this comment.
Maybe hint at the intention to keep it in the user modules?
CythonFunctionModuleCodeCythonFunctionPerModuleCythonFunctionNonSharedCythonFunctionInlineable
|
|
I guess that comment was really for #7557? (This PR here is a big disappointment if it increases sizes) |
Cython/Utility/CythonFunction.c
Outdated
| //////////////////// CythonFunctionAlways.proto ////////////////////////// | ||
| // This section always gets included whether we're using CythonFunction through | ||
| // shared utility code or not. |
There was a problem hiding this comment.
Maybe hint at the intention to keep it in the user modules?
CythonFunctionModuleCodeCythonFunctionPerModuleCythonFunctionNonSharedCythonFunctionInlineable
Yes correct. I am sorry. I had two tabs open and wrote in wrong one :-(. Lately, I am kind of messy in GH... :-( |
| if (__Pyx_VerifyCachedType((PyObject*)tp, | ||
| __PYX_TYPE_MODULE_PREFIX "fused_cython_function", | ||
| __PYX_SHARED_SIZEOF(__pyx_FusedFunctionObject)) != 0) { | ||
| Py_DECREF(tp); |
There was a problem hiding this comment.
Is this right? I think it needs to cast to a PyObject * (and also in __Pyx_Get_FusedFunction_Type)
This came up in astropy/astropy#19422
There was a problem hiding this comment.
You're right (at least in the Limited API).
I'm a bit puzzled how astropy are getting here though because I don't think they're using the shared utility code feature.
I'll investigate and make sure that nothing else is going wrong
There was a problem hiding this comment.
I'm a bit puzzled how astropy are getting here though because I don't think they're using the shared utility code feature.
Actually __Pyx_Get_FusedFunction_Type is universal so that's fine.
Not sure why this doesn't break every limited API test on Cython though
There was a problem hiding this comment.
Does it matter that they are using CPython3.11?
There was a problem hiding this comment.
No - it's just that our Limited API testing is currently invisibly turned off by mistake.
Implementation notes:
__pyx_[Cython/Fused]Function_initfunctions, which are used to initialize/fetch the type object, differ depending on whether it is locally defined or defined in the shared module. This is done fairly crudely by just using different utility code because it didn't seem worth a complicated mechanism._initfunctions because they needed to be called after the shared module is imported.Sizes:
Shared module before: 154440 bytes
Shared module with cyfunc: 172256 bytes (+17816 bytes)
Shared module with fusedfunc+cyfunc: 172704 bytes (+448 bytes from cyfunc, +18264 bytes from before)
So adding fusedfunc is a pretty negligable difference (which is good, because it's used relatively infrequently compared to cyfunc)
Module that uses CyFunc drops 17880 bytes
Module that uses CyFunc and fusedfunc drops 18328 bytes
(All sizes are stripped)
The other shared types are probably less obvious about whether to add, but could be handled in much the same way. My view is that they're infrequently used so you'd probably want to be able to configure whether they're included rather than always doing it. I'd like to keep this PR to just the function types.