This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Unsupported provider

classification
Title: Move fields relevant to sys.exc_info out of frame into generator/threadstate
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.6
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: Jim.Jewett, Mark.Shannon, amaury.forgeotdarc, benjamin.peterson, deleted0524, jcea, ncoghlan, pitrou, ron_adam, scoder, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2012-01-28 16:24 by Mark.Shannon, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
exc_info.patch Mark.Shannon, 2012-02-13 11:57 Patch to rev. 58bd6a58365d review
exc_info2.patch Mark.Shannon, 2012-04-11 12:06 Patch to revision 8a47d2322df0 review
exc_info3.patch Mark.Shannon, 2015-07-25 11:18 review
Messages (24)
msg152167 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2012-01-28 16:24
The frame object has a number of fields which belong to the generator object.
By creating a CoState struct, these fields can be moved to generator/threadstate where they belong.

The interpreter no longer has to swap around exception state whenever generators are entered/exited.
The frame object is made more compact, reducing allocation/deallocation overhead (although benchmarking showed no significant difference) 

The attached patch reduces the code base by about 50 LOC overall.
msg152202 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2012-01-28 23:53
- Why is it called CoState?  is it related to coroutines?
- I'm a bit worried about the removal of tstate->exc_value, and the changes in sys.exc_info().  "tstate->exc_value" is used by many extension modules, do you really want to replace this?
msg152205 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-01-29 00:41
Extensions modules should really be using PyErr_Occurred/Fetch etc.
msg152207 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-01-29 02:00
The division of responsibilities between generator objects and the eval loop is currently a little messy. The eval loop deals almost entirely with frame objects and also handles swapping exception states around on behalf of generators, which is why the generator specific exception state currently lives on frame objects (i.e. to avoid the eval loop needing to know too much about the internal structure of generator objects).

Before generator related state can reasonably be moved out of the frame objects, those responsibilities should be divided more cleanly. Ron Adam has made an initially attempt at tackling that problem in issue 13607.

Once the responsibilities are divided appropriately, *then* we can look at the fields that ceval no longer touches and see if they should be moved somewhere else (whether that's directly into the generator struct or into a new struct referenced from the generator struct).
msg152220 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2012-01-29 09:22
Nick Coghlan wrote:
> Nick Coghlan <ncoghlan@gmail.com> added the comment:
> 
> The division of responsibilities between generator objects and the eval loop is currently a little messy. The eval loop deals almost entirely with frame objects and also handles swapping exception states around on behalf of generators, which is why the generator specific exception state currently lives on frame objects (i.e. to avoid the eval loop needing to know too much about the internal structure of generator objects).

In my patch the exception state does not need swapping at all, so the 
question of whose responsibility it is become mute.
Neither generator nor ceval need to do it.

> 
> Before generator related state can reasonably be moved out of the frame objects, those responsibilities should be divided more cleanly. Ron Adam has made an initially attempt at tackling that problem in issue 13607.
> 
> Once the responsibilities are divided appropriately, *then* we can look at the fields that ceval no longer touches and see if they should be moved somewhere else (whether that's directly into the generator struct or into a new struct referenced from the generator struct).

This patch is largely orthogonal to that proposed for issue 13607.
In fact the new CoState struct would be a better place to pass data 
between PyEval_EvalFrame() and gen_send(), rather than adding yet 
another field to the frame.
msg152221 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2012-01-29 09:26
Amaury Forgeot d'Arc wrote:
> 
> - Why is it called CoState?  is it related to coroutines?

Yes it is related to coroutines, threads and generators *are*
(a limited form of) asymmetric coroutines, even if we don't usually 
think of them that way).

Perhaps GenState would be a better name
(treating the threadstate as a sort of top level pseudo-generator)?
msg152260 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2012-01-29 21:09
IMO "tstate->exc_value" has nothing to do with generators.  Changing its name seems gratuitous breakage to me.
msg152261 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2012-01-29 21:23
The important part is not the change of name, but wrapping them in a 
struct which can be embedded in both PyThreadState and PyGenObject.

The state->exc_XXX trio of values are the currently handled exception
(sys.exc_info()) and are shadowed by generator exception handlers.
My patch models that shadowing rather than swapping the values in and 
out. This allows me to eliminate save_exc_state(), swap_exc_state()
and restore_and_clear_exc_state() completely.
msg153260 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2012-02-13 11:57
This issue was too broad. The new patch is focussed on sys.exc_info.
All hints of coroutines have been removed and f_yieldfrom is untouched.

New patch reduces code by 65 lines and does not conflict with issue 13607.
msg155049 - (view) Author: Jim Jewett (Jim.Jewett) * (Python triager) Date: 2012-03-07 05:10
I think the latest patch is indeed cleaner.
msg155058 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2012-03-07 08:23
Jim Jewett wrote:
 > http://bugs.python.org/review/13897/diff/4186/14521
 > File Python/sysmodule.c (right):
 >
 > http://bugs.python.org/review/13897/diff/4186/14521#newcode211
 > Python/sysmodule.c:211: while ((exc_info->exc_type == NULL ||
 > exc_info->exc_type == Py_None) &&
 > This while loop is new, but it isn't clear how it is related to
 > encapsulating the exception state.  Is this fixing an unrelated bug, or
 > is it from generators, or ..?
 >
 > http://bugs.python.org/review/13897/show

Running generators form a stack, much like frames.
Calling a generator with next or send, pushes it onto the stack,
yielding pops it.

Now consider, if you will, the threadstate object as a sort of
non-yielding (it cannot be popped) generator which forms the base
of this stack.

In this patch, rather than swapping the exception state between 
generator-owned frame and threadstate whenever entering or leaving a 
generator, each generator (and the threadstate) has its own exception state.

It order to find the topmost exception state, sys.exc_info searches the 
stack of generators until it finds one.
In practice the generator stack will be very shallow, only 1 or 2 deep,
as it is rare to have generators calling other generators
(although this will become a bit more common with PEP 380).
msg157841 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-09 13:01
This looks rather good on the principle. I think PyExcState needn't be public, it should be _PyExcState.
I haven't checked the detailed mechanics of the patch, I hope someone else can.
msg157850 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2012-04-09 15:40
FWIW, Cython keeps the exception state in the generator struct and that works nicely.

Note that Amaury is right in that extensions use tstate->exc_value and friends. Cython does so quite extensively, for example. I don't see any use in changing the plain fields into a struct, but it will definitely break code, and not just some. This is also unrelated to the topic of this issue, so it should be a separate issue in the first place, and that should then be rejected IMHO.

Also note that there is a separate issue 14098 (with patch) on providing a public C-API for these three fields - as long as there is none but direct access to these public fields, a change that basically removes them should not even be seriously considered.
msg157854 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-09 15:45
> Note that Amaury is right in that extensions use tstate->exc_value and
> friends. Cython does so quite extensively, for example.

I understand for Cython, but why would pedestrian extension code look up
tstate->exc_value?
msg157856 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2012-04-09 15:56
3rd party code should not be accessing fields in the threadstate object,
but without the accessors proposed in issue 14098 there may be no alternative.

Once the patch for issue 14098 has been applied it, would it then be acceptable to remove the surperfluous fields from the frameobject?
The logic for accessing sys.exc_info can be moved to into the new 
accessor fucntions.
msg157863 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2012-04-09 17:07
I can't speak for much outside of Cython, and Cython generated modules would best be regenerated with a newer Cython version anyway in order to work with Py3.3. I'm not sure that's currently required, though.

As long as there is a way to access these fields directly from the struct (with the usual preprocessor conditional), I don't think Cython will actually start to use the PyErr_[GS]etExcInfo() functions in CPython - simply for performance reasons. Inlining the access is just way faster than calling into external functions, especially when that happens more than once.

I'm still surprised about the general lack of resistence against incompatible changes to a public struct, though, especially when they come without an obvious gain. Is this really just for code beautification? I agree that this would be nice, but how can it be worth breaking code?

I don't see any objection to the generator related changes in this patch.
msg157865 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-09 17:17
> As long as there is a way to access these fields directly from the
> struct (with the usual preprocessor conditional), I don't think Cython
> will actually start to use the PyErr_[GS]etExcInfo() functions in
> CPython - simply for performance reasons.

Isn't this premature optimization? Do you have any workload where you
measured a performance degradation?
msg158027 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2012-04-11 12:06
According to PEP 384 (Defining a Stable ABI) the thread state object is opaque, so we should be free to add or remove fields.

Nevertheless, I have added a new patch that simply moves the f_exc... fields from the frame object to the generator. It is not as efficient as the first patch, but at least it moves fields relevant only to generators into the generator object.
msg158028 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-11 12:11
> According to PEP 384 (Defining a Stable ABI) the thread state object
> is opaque, so we should be free to add or remove fields.

Mmh, be aware the stable ABI is only a restricted part of the official
API. There are APIs which are not part of the stable ABI, but are still
public and officially supported.

That said, I agree the thread state *structure* shouldn't be part of the
official API. If necessary, we can provide accessors for some of its
members.
msg158251 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2012-04-14 09:16
>> As long as there is a way to access these fields directly from the
>> struct (with the usual preprocessor conditional), I don't think Cython
>> will actually start to use the PyErr_[GS]etExcInfo() functions in
>> CPython - simply for performance reasons.
>
> Isn't this premature optimization? Do you have any workload where you
> measured a performance degradation?

To be honest - I don't know if it makes a measurable difference. However, the code is there, it's required for all currently released CPython versions (i.e. those still being supported for years to come), and it is used in seriously performance critical places, such as each generator/coroutine iteration - twice. Why should we add overhead to those without need?
msg247341 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2015-07-25 11:18
Updated patch to support exception chaining and to merge cleanly.
msg294287 - (view) Author: deleted0524 (deleted0524) Date: 2017-05-23 21:36
This issue is rather old, so I will create a new GitHub PR for the code change. This issue can be closed.

Note that https://bugs.python.org/issue25612 is a manifestation of the problem that this was intended to solve.
msg312819 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-02-25 15:33
Aren't they were moved in issue25612? Is there something that should be done in this issue?
msg312934 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2018-02-26 15:42
Nothing left to do. This is now obsolete.
History
Date User Action Args
2022-04-11 14:57:26adminsetgithub: 58105
2018-02-26 16:07:11serhiy.storchakasetstatus: open -> closed
2018-02-26 15:42:32Mark.Shannonsetstatus: pending -> open
resolution: out of date
messages: + msg312934

stage: patch review -> resolved
2018-02-25 15:34:05serhiy.storchakasetstatus: open -> pending
2018-02-25 15:33:57serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg312819
2017-05-23 21:36:00deleted0524setnosy: + deleted0524
messages: + msg294287
2015-07-25 11:18:04Mark.Shannonsetfiles: + exc_info3.patch

messages: + msg247341
versions: + Python 3.6, - Python 3.4
2012-10-15 19:24:03terry.reedysetversions: + Python 3.4, - Python 3.3
2012-04-14 09:16:12scodersetmessages: + msg158251
2012-04-11 12:11:02pitrousetmessages: + msg158028
2012-04-11 12:06:25Mark.Shannonsetfiles: + exc_info2.patch

messages: + msg158027
2012-04-09 17:17:40pitrousetmessages: + msg157865
2012-04-09 17:07:01scodersetmessages: + msg157863
2012-04-09 15:56:56Mark.Shannonsetmessages: + msg157856
2012-04-09 15:45:22pitrousetmessages: + msg157854
2012-04-09 15:40:21scodersetnosy: + scoder
messages: + msg157850
2012-04-09 13:01:46pitrousetnosy: + pitrou
messages: + msg157841
2012-03-07 08:23:45Mark.Shannonsetmessages: + msg155058
title: Move fields relevant to sys.exc_info out of frame into generator/threadstate -> Move fields relevant to sys.exc_info out of frame into generator/threadstate
2012-03-07 05:10:17Jim.Jewettsetnosy: + Jim.Jewett
messages: + msg155049
2012-02-13 12:00:12Mark.Shannonsetfiles: - coro.patch
2012-02-13 11:57:41Mark.Shannonsetfiles: + exc_info.patch

messages: + msg153260
title: Move fields relevant to coroutine/generators out of frame into generator/threadstate -> Move fields relevant to sys.exc_info out of frame into generator/threadstate
2012-01-31 16:36:41jceasetnosy: + jcea
2012-01-29 21:23:10Mark.Shannonsetmessages: + msg152261
2012-01-29 21:09:02amaury.forgeotdarcsetmessages: + msg152260
2012-01-29 09:26:04Mark.Shannonsetmessages: + msg152221
2012-01-29 09:22:59Mark.Shannonsetmessages: + msg152220
title: Move fields relevant to coroutine/generators out of frame into generator/threadstate -> Move fields relevant to coroutine/generators out of frame into generator/threadstate
2012-01-29 02:00:25ncoghlansetnosy: + ron_adam
messages: + msg152207
2012-01-29 00:41:25benjamin.petersonsetmessages: + msg152205
2012-01-28 23:57:50pitrousetnosy: + ncoghlan, benjamin.peterson

stage: patch review
2012-01-28 23:53:56amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg152202
2012-01-28 16:24:46Mark.Shannoncreate