ENH,API: Make the errstate/extobj a contextvar #23936
Conversation
|
PyPy doesn't support |
|
Yay, less code and faster too! Does the speedup show up in the benchmarks? You are using a capsule because it is easier than a typeobject? I guess that is fine, and it is an implementation detail we can change later.
No, as of now it does not. Basically PyPy implements all the contextvar C-API by calling back into the interpreter's |
I thought I would hide things, and changing the contextvar in C seemed fair (so that I just did a change to expose it and at least do the I think that would be fine also. On the capsule: yes, its just slightly easier to implement, although means two objects are created (capsule struct + capsule). This is one of those things where cython would be nice (also since we could move |
The optimization doesn't make sense and is incorrect to begin with. It just stands in the way of moving to a contextvar. (Maybe there is a way to optimize the contextvar access, but it has to look differently and should be done after we got it.)
Mainly, pass the ufunc name everywhere explicitly, rather than building an unnecessary tuple.
Move them to live exclusively in `extobj.c` source file where they are actually used.
(I don't think its correct, but OK)
This fixes PyPy, which doesn't implement the full C-side API. It is also actually faster probably...
seberg
force-pushed
the
errstate-contextvar
branch
from
cb29c6f
to
0c42ce1
Compare
I think so. Would it be less code? |
|
Thinking about naming, calling it |
…uncs) This may be a few nanoseconds slower in CPython, but should be better for PyPy. It is also maybe just a bit easier to grok since the reset of the contextvar is in Python already anyway.
Done, doesn't make much difference code-wise, but since the
Yeah,
|
seberg
force-pushed
the
errstate-contextvar
branch
from
2773a59
to
73be58f
Compare
There was a problem hiding this comment.
Nice. There is a segfault (maybe at shutdown?) and some other failures I didn't look at.
I found a few nitpicky typos that can be part of the next set of commits
Co-authored-by: Matti Picus <matti.picus@gmail.com>
seberg
force-pushed
the
errstate-contextvar
branch
from
be1a4dd
to
0b896db
Compare
|
I aborted the timing, the full thing takes quite long nowadays. In any case, the only thing I could think of that I expected to slow down is: and it actually got faster with the change: to 619ns. When you ignore FPEs (and they get floated) the speedup is at least 50ns in total. The |
|
Cirrus CI got this weird error again (going to restart, since I don't think its related) EDIT: There was a real bug. Although likely unrelated to the specific failure. |
Impressive how few tests failed, but tests *did* fail and they didn't segfault :)
|
In the previous PR there are still traces of the removed interfaces in the documentation. I see a warning when building the docs: I think that is coming from doc/source/reference/routines.err.rst Could you remove them in this PR or should I make another to do it? |
|
Thanks Sebastian. |
I removed them, see #23964. |
**This builds on gh-23922, so a draft. Also needs some release notes, but it is ready for review (the diff is just too large without gh-23992)**This does a few changes:
In the end, things are now well encapsulated into
extobj.crather than sprinkled around. ItThe choice of a capsule is a bit 🤷 a custom object may actually be almost as lean and neat because it removes some of that custom cleanup.
End-user effects:
np.errstateis now actually thread-safe and additionally context safe.np.errstatecannot be entered twice (but that doesn't make sense). I made its attribute private also (but you shouldn't use them).np.errstateis much faster (I think about 2.5-3 times), not sure if the__slots__matter, but why not.There is still a bunch of follow-ups that should happen eventually. The way the ufuncs use this API isn't adapted yet (and passes unused stuff maybe even, but maybe will add that part here). Mainly, we probably should get the
extobjexactly once.Closes gh-9444
Closes gh-19006