Move fields relevant to sys.exc_info out of frame into generator/threadstate

[markshannon]

BPO	13897
Nosy	@jcea, @amauryfa, @ncoghlan, @pitrou, @scoder, @benjaminp, @markshannon, @JimJJewett, @serhiy-storchaka
Files	exc_info.patch: Patch to rev. 58bd6a58365d exc_info2.patch: Patch to revision 8a47d2322df0 exc_info3.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2018-02-26.16:07:11.849>
created_at = <Date 2012-01-28.16:24:46.541>
labels = ['interpreter-core', 'type-feature']
title = 'Move fields relevant to sys.exc_info out of frame into\tgenerator/threadstate'
updated_at = <Date 2018-02-26.16:07:11.848>
user = 'https://github.com/markshannon'

bugs.python.org fields:

activity = <Date 2018-02-26.16:07:11.848>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2018-02-26.16:07:11.849>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2012-01-28.16:24:46.541>
creator = 'Mark.Shannon'
dependencies = []
files = ['24508', '25177', '40015']
hgrepos = []
issue_num = 13897
keywords = ['patch']
message_count = 24.0
messages = ['152167', '152202', '152205', '152207', '152220', '152221', '152260', '152261', '153260', '155049', '155058', '157841', '157850', '157854', '157856', '157863', '157865', '158027', '158028', '158251', '247341', '294287', '312819', '312934']
nosy_count = 11.0
nosy_names = ['jcea', 'amaury.forgeotdarc', 'ncoghlan', 'pitrou', 'scoder', 'ron_adam', 'benjamin.peterson', 'Mark.Shannon', 'Jim.Jewett', 'serhiy.storchaka', 'deleted0524']
pr_nums = []
priority = 'normal'
resolution = 'out of date'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue13897'
versions = ['Python 3.6']

[markshannon]

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.

[amauryfa]

• 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?

[benjaminp]

Extensions modules should really be using PyErr_Occurred/Fetch etc.

[ncoghlan]

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 bpo-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).

[markshannon]

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 bpo-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 bpo-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.

[markshannon]

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)?

[amauryfa]

IMO "tstate->exc_value" has nothing to do with generators. Changing its name seems gratuitous breakage to me.

[markshannon]

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.

[markshannon]

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 bpo-13607.

[jimjjewett]

I think the latest patch is indeed cleaner.

[markshannon]

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).