Add support for PEP 567 context variables to tasklets #243

akruis · 2021-04-30T23:56:34Z

A possible implementation for #239.

… function Use a Py_LOCAL_INLINE function instead of a complicated macro. It is much simpler to debug. In the next commit, we will context switching to this function.

Add a private context attribute and appropriate methods to class tasklet. Document the changes in the manual. New methods: tasklet.set_context(context), tasklet.context_run(...) New readonly attribute: tasklet.context_id

Doc/library/stackless/pickling.rst

Add/improve pickling of the context of tasklets. New pickle flag "PICKLEFLAGS_PICKLE_CONTEXT", new undocumented function stackless._tasklet_get_unpicklable_state()

akruis · 2021-05-05T13:46:06Z

I'm now fairly happy with this pull request.
@kristjanvalur I really appreciate your comments.

kristjanvalur · 2021-05-05T13:49:18Z

I'll try to give it a good read tonight.

Doc/library/stackless/tasklets.rst

kristjanvalur · 2021-05-05T21:26:51Z

Include/slp_structs.h

@@ -134,6 +134,7 @@ typedef struct _tasklet {
    int recursion_depth;
    PyObject *def_globals;
    PyObject *tsk_weakreflist;
+    PyObject *context;  /* if running: the saved context, otherwise the context for the tasklet */


What is a saved context? If it is the context of a previous tasklet, surely that tasklet's context is stored in that tasklet's
context member.

In C-Python the thread state (PyThreadState or short ts for the current thread state) contains a counted reference to a Context object: ts->context. This reference may be NULL.

In Stackless Python each tasklet has its own counted reference to a Context object: tasklet->context.

Now, if you switch from one tasklet to another the reference count of the involved context objects must not change. (During a tasklet switch a recursion into the interpreter is not allowed and therefore Py_DECREF is not allowed.) Therefore its is only possible to "exchange" context between variables.

exchange the values of old_tasklet->context and ts->context

exchange the values of new_tasklet->context and ts->context

If you create the main tasklet in slp_initialize_main_and_current() only one exchange happens:

exchange the values of main_tasklet->context and ts->context

Same on exit of the main tasklet in schedule_task_destruct().

exchange the values of main_tasklet->context and ts->context

The effect of this schema is, that the value of tasklet->context depends on the state of the tasklet.

if tasklet is current: tasklet->context is the value of ts->context before the main tasklet was created. ts->context is the context of the current tasklet.

if tasklet is not current: tasklet->context is the context of the tasklet.

To answer your question: the "saved" context is the value of ts->context when the main-tasklet was created.
Of course I have to improve the comment.

Why do we need to create a context when the main tasklet is created? how about this:

main tasklet is created and entered. it inherits the threads context. ts->stackless->current == ts->stackless->main_tasklet, and ts->context is possibly non null and main_tasklet->context == NULL. (sorry if my recollection of stackless ts is a bit rusty)

create a new tasklet. It gets a copy of the ts->context as described.

switch to another tasklet. . old_tasklet->context = ts->context, and ts->context = new_tasklet->context

I'm probably missing some fine detail, though, but it still seems to me that the main tasklet's context should be the same as the thread's context that created it. That way, a runnint tasklet has a context of NULL, and a sleeping context has a possibly non-null context. I don't see why we need to keep the context of the main thread before initializing the main tasklet as a separate thing, because the main tasklet is the entry context of the thread.

You are perfectly right. Keeping the context of the main thread is just a side effect of the implementation. And the implementation - the exchange values between thread-state and tasklet logic - was just an attempt to keep the reference counting correct without too much thinking about it: the exchange operation simply does not affect reference counts and there was already suitable macro/inline-function (SLP_EXCHANGE_EXCINFO). But your suggestion most likely also works an is much simpler. See 71d8e0e

Stackless/module/clinic/taskletobject.c.h

kristjanvalur · 2021-05-05T21:33:29Z

Stackless/module/scheduling.c

+        c = ts_->context; \
+        ts_->context = t_->context; \
+        t_->context = c; \
+        ts_->context_ver++; \


See previous comment about why the currently runnint tasklet has a "saved" context? Also, what is context_ver?

Abaut context_ver:

A context is a Mapping from ContextVar objects to their values. A ContextVar caches its current value and the current context_ver. To invalidate the cache context_ver must be incremented on each change of ts->context. See

stackless/Python/context.c

Line 759 in 4c7f020

var->var_cached_tsver = ts->context_ver;

Stackless/module/taskletobject.c

kristjanvalur

Wow. This is a major addition, probably the biggest change in many years.
I didn't review the C code in detail or the tests, I trust these run fine. Also, my C python eyes have become a bit rusty. But I did have some questions about context and tasklets.

kristjanvalur · 2021-05-05T21:38:49Z

Also, great work, Anselm!

akruis · 2021-05-08T10:37:54Z

Also, great work, Anselm!

Thank's. But your remarks make it even better. Many thanks for your review. It is really hard to write good quality code, if you're alone.

- better documentation of the concept - fix a left-over from the time, when setting the context of a main- tasklet was not supported - fix the context handling, when the main tasklet starts/ends. Copy the context from/to the thread state.

- fix the context handling, when the main tasklet starts/ends. Copy the context from/to the thread state. Fix the test case.

akruis · 2021-05-08T23:40:14Z

I fixed the issues found by Kristján and one other issue.

Use stackless._stackless._test_outside instead of stackless.test_outside.

kristjanvalur · 2021-05-11T14:48:02Z

Couple of things I'd like explained.

Its the saved tasklet of the main before task initialization above. why we need a sepaate context for the main tasklet.
Comments mention that contexts are useful for tasklets in conjunction with async. I agree that context are useful (previously we had stacklesslib.tasklet_local or something, based on threading.local), and having a uniform storage like this, which works for threads, co-routines, and tasklets, is good. But do you see a specific use case where we have to do manual manipulation of tasklet contexts?

contexts managed by stackless are great, by the way, since we (probably) don't need to manipulate them, or have these "run in context" functions like a async hub has to do (i.e. manually switching context). ... Maybe this can be added to the doc, explainin that there really needs to be no manual management of contexts when using tasklets.

Simplify context switching as sugested by Kristján.

akruis · 2021-05-18T09:17:25Z

Couple of things I'd like explained.
1. Its the saved tasklet of the main before task initialization above. why we need a sepaate context for the main tasklet.

Good point. I simplified the code in 71d8e0e. (When I coded the first version I just copied the approach used for the exception
state without thinking much about the requirements.)

2. Comments mention that contexts are useful for tasklets in conjunction with async.  I agree that context are useful (previously we had stacklesslib.tasklet_local or something, based on threading.local), and having a uniform storage like this, which works for threads, co-routines, and tasklets, is good.  But do you see a specific use case where we have to do manual manipulation of tasklet contexts?
contexts managed by stackless are great, by the way, since we (probably) don't need to manipulate them, or have these "run in context" functions like a async hub has to do (i.e. manually switching context). ... Maybe this can be added to the doc, explainin that there really needs to be no manual management of contexts when using tasklets.

I already have a valid use case for tasklet.context_run(). This method is required, if you have to execute something on behalf of a tasklet. In the example at https://github.com/akruis/slp_coroutines/blob/03333e72356a06835b742b4e78b88c544a3a00bb/src/slp_coroutine.py#L109-L112 the tasklet wants you to await a a coroutine.

I can imagine a requirement for tasklet.set_context()

if you rebind a tasklet, i.e. if you have a pool of worker tasklets and
after unpickling a tasklet

For tasklet.context_id probably debugging is the only valid use case.

kristjanvalur

Great stuff. sorry for my late review, Busy with things :)

akruis · 2021-05-18T19:58:25Z

I'm busy too, during the day. Stackless is an evening project. It gets me away from watching youtube all the time. There isn't much you can do in the evenings these days.
Again many thanks for reviewing.

Anselm Kruis added 2 commits May 1, 2021 01:40

Stackless issue python#239: refactor SLP_EXCHANGE_EXCINFO to become a…

1a87f32

… function Use a Py_LOCAL_INLINE function instead of a complicated macro. It is much simpler to debug. In the next commit, we will context switching to this function.

akruis requested a review from kristjanvalur April 30, 2021 23:57

akruis commented May 1, 2021

View reviewed changes

Doc/library/stackless/pickling.rst Show resolved Hide resolved

Anselm Kruis added 3 commits May 4, 2021 22:52

Merge branch '3.7-slp' into slp-issue239-slp37-pep567

bac0c5a

Stackless issue python#239: add support for PEP 567 context variables

4148ec3

Add/improve pickling of the context of tasklets. New pickle flag "PICKLEFLAGS_PICKLE_CONTEXT", new undocumented function stackless._tasklet_get_unpicklable_state()

Merge branch '3.7-slp' into slp-issue239-slp37-pep567

54fbc39

kristjanvalur reviewed May 5, 2021

View reviewed changes

Doc/library/stackless/tasklets.rst Outdated Show resolved Hide resolved

kristjanvalur reviewed May 5, 2021

View reviewed changes

Stackless/module/clinic/taskletobject.c.h Show resolved Hide resolved

kristjanvalur reviewed May 5, 2021

View reviewed changes

Stackless/module/taskletobject.c Outdated Show resolved Hide resolved

Stackless issue python#239: fix a typo in the documentation

266ee33

kristjanvalur reviewed May 5, 2021

View reviewed changes

Anselm Kruis added 3 commits May 9, 2021 00:06

Merge branch '3.7-slp' into slp-issue239-slp37-pep567

a102fda

Stackless issue python#239: fix issues found during review, part 2

4ea2610

- fix the context handling, when the main tasklet starts/ends. Copy the context from/to the thread state. Fix the test case.

Anselm Kruis added 2 commits May 9, 2021 14:02

Merge branch '3.7-slp' into slp-issue239-slp37-pep567

c4dcaf1

Stackless issue python#239: adapt a test case

d524887

Use stackless._stackless._test_outside instead of stackless.test_outside.

Stackless issue python#239: simplify context switching

71d8e0e

Simplify context switching as sugested by Kristján.

kristjanvalur approved these changes May 18, 2021

View reviewed changes

Stackless issue python#239: documentation and comment updates

bdb7bc4

Merge branch '3.7-slp' into slp-issue239-slp37-pep567

1d282cb

akruis merged commit 032a566 into stackless-dev:3.7-slp May 18, 2021

Add support for PEP 567 context variables to tasklets #243

Add support for PEP 567 context variables to tasklets #243

Uh oh!

Conversation

akruis commented Apr 30, 2021

Uh oh!

Uh oh!

akruis commented May 5, 2021

Uh oh!

kristjanvalur commented May 5, 2021

Uh oh!

Uh oh!

kristjanvalur May 5, 2021

Choose a reason for hiding this comment

Uh oh!

akruis May 8, 2021

Choose a reason for hiding this comment

Uh oh!

kristjanvalur May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akruis May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kristjanvalur May 5, 2021

Choose a reason for hiding this comment

Uh oh!

akruis May 8, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kristjanvalur left a comment

Choose a reason for hiding this comment

Uh oh!

kristjanvalur commented May 5, 2021

Uh oh!

akruis commented May 8, 2021

Uh oh!

akruis commented May 8, 2021

Uh oh!

kristjanvalur commented May 11, 2021

Uh oh!

akruis commented May 18, 2021

Uh oh!

kristjanvalur left a comment

Choose a reason for hiding this comment

Uh oh!

akruis commented May 18, 2021

Uh oh!

Uh oh!

kristjanvalur May 11, 2021 •

edited

Loading

akruis May 13, 2021 •

edited

Loading