bpo-32436: Implement PEP 567

[1st1]

The PR implements PEP 567. It consists of a few major blocks:

1. New public APIs. Files of interest:

   • Includes/context.h: the public C API;
   • Includes/internal/context.h: structs layout;
   • Python/context.c: implementation;
   • Modules/_contextvarsmodule.c and Lib/contextvars.py: the contextvars module.

2. Changes in asyncio (including Modules/_asynciomodule.c).

3. HAMT immutable dict implementation. Files of interest: Python/hamt.c and Include/internal/hamt.h.

   • HAMT APIs are private, and are not exported to Python.h.

   • I stress-tested HAMT with millions of random key/value insertions/deletions/modifications. I also used the llvm tooling to ensure that tests cover 99% of the C code. I'm pretty confident that there are no memory leaks and that the implementation is stable.

4. Few minor changes throughout the codebase, including pystate.h and pystate.c, and modifications of build/make scripts.

Tests in test_context and test_asyncio pass the refleaks test mode.

https://bugs.python.org/issue32436

[methane]

I think writing URL is time to break 79-columns rule.

[1st1]

sure

[methane]

PyTuple_Pack()

[1st1]

Thanks, I completely forgot about it.

[methane]

Subclassing Hamt is allowed?

[1st1]

No, it's not even exposed to Python.

[methane]

https://docs.microsoft.com/en-us/cpp/intrinsics/popcnt16-popcnt-popcnt64

[1st1]

Updated

[njsmith]

dictobject.c makes a big deal about how Python hashes don't need to be randomly allocated. I assume the issue here is that we know these hashes will be used in HAMTs, and compared to regular dicts, HAMTs are more sensitive to equidistribution? Probably it's worth mentioning this motivation in the comment.

[1st1]

Updated

[methane]

Using >> 1 instead of / 2 is 20th century's hack.
All compilers can do this optimization nowdays.

[1st1]

Fixed in a few places.

[1st1]

@methane Thanks for the initial review, Inada-san. Do you have any other comments/suggestions?

[methane]

GCC produce these errors. I think we have macro for it, but I don't remember it.

Python/hamt.c: In function ‘hamt_node_collision_assoc’:
Python/hamt.c:1432:1: warning: control reaches end of non-void function [-Wreturn-type]
 }                                                                                                                                                                                             ^
Python/hamt.c: In function ‘_PyHamt_Without’:
Python/hamt.c:2365:1: warning: control reaches end of non-void function [-Wreturn-type]
 }                                                                                                                                                                                             ^
Python/hamt.c: In function ‘hamt_node_bitmap_without’:
Python/hamt.c:1093:1: warning: control reaches end of non-void function [-Wreturn-type]
 }                                                                                                                                                                                             ^
Python/hamt.c: In function ‘hamt_node_array_without’:
Python/hamt.c:1911:1: warning: control reaches end of non-void function [-Wreturn-type]
 }                                                                                                                                                                                             ^
Python/hamt.c: In function ‘hamt_node_collision_without’:
Python/hamt.c:1526:1: warning: control reaches end of non-void function [-Wreturn-type]
 }                                                                                                                                                                                             ^
Python/hamt.c: In function ‘_PyHamt_Find’:
Python/hamt.c:2395:1: warning: control reaches end of non-void function [-Wreturn-type]
 }                                                                                                                                                                                             ^
Python/hamt.c: In function ‘hamt_baseiter_tp_iternext’:
Python/hamt.c:2548:1: warning: control reaches end of non-void function [-Wreturn-type]
 }                                                                                                                                                                                             ^
Python/hamt.c: In function ‘hamt_py_get’:                                                                                                                                                     Python/hamt.c:2801:1: warning: control reaches end of non-void function [-Wreturn-type]
 }                                                                                                                                                                                             ^
Python/hamt.c: In function ‘hamt_tp_subscript’:
Python/hamt.c:2749:1: warning: control reaches end of non-void function [-Wreturn-type]
 }                                                                                                                                                                                             ^

[methane]

Can it be moved to internal header?

[1st1]

No, because we're using it in _testcapimodule.c to test the HAMT datatype directly in Lib/test/test_context.py.

[1st1]

(unless I'm missing something and it's possible to expose the HAMT type to Python somehow without making a public C API method)

[methane]

I'm not sure what context equality should be.
How is it used?

[1st1]

It has to be implemented because per PEP 567 Context implements collections.abc.Mapping, which has a __eq__ method.

[1st1]

And since Context behaves like a Mapping in all its methods, Context.__eq__ should simply compare if another Context object has the same contents or not.

[methane]

I think we should use val_or_node == val instead of RichCompareBool.

[methane]

>>> from _testcapi import hamt
>>> h = hamt()
>>> h2 = h.set('a', [])
>>> h3 = h2.set('a', [])
>>> h3.get('a').append(42)
>>> h2.get('a')
[42]

I tried to reproduce it with contextvars, but I faced fatal error. (Sorry, I used non debug build)
https://gist.github.com/methane/2ce5eda468a4091be60c8233080fe265

[1st1]

Oh, this is a real bug, thanks so much for finding it!

Funny enough, I initially got this right, and was using val_or_node == val. At sometime I decided to review/document hamt.c and changed it to PyObject_RichCompareBool without giving it too much thought.

Anyways, I'll fix the HAMT and it will also fix the fatal error you saw. That error is interesting on its own. What happened is that PyContextVar_Set caches a borrowed ref to the last value set. Because you were setting another empty list object to the variable, it wasn't really stored (because of the bug in hamt.c), so we stored a borrowed reference to an object that was about to be GCed.

[methane]

Py_UNREACHABLE() is macro for unreachable flow.

[1st1]

Thank you, Inada-san.

[1st1]

@gvanrossum has just approved the PEP so I guess it's time to merge this!

[smurfix]

Is there a pure-Python version of contextvars?

[vltr]

@smurfix see here

[thehesiod]

perhaps I'm missing something, but it seems the fact that only async tasks inherit a copy of their parent contexts is not mentioned in the docs: https://docs.python.org/3/library/contextvars.html It was surprising to me because threading.local did not behave this way by default, and in fact it makes the API behave differently between async and threading worlds per:

import asyncio
import contextvars
import threading

var = contextvars.ContextVar('var')
var.set('spam')


def doit():
    print(f"thread: {var.get()}")


async def async_sub_doit():
    print(f"async sub: {var.get()}")


async def async_doit():
    print(f"async: {var.get()}")
    var.set('span2')
    print(f"async: {var.get()}")
    await asyncio.create_task(async_sub_doit())

print(f"main: {var.get()}")

try:
    thread = threading.Thread(target=doit)
    thread.start()
    thread.join()
except Exception as e:
    print(f"Thread exception: {e}")

asyncio.run(async_doit())

which results in the spawned thread not inheriting the contextvars from the parent, whereas the spawned async tasks do. I think this is an important distinction to make to developers. Personally looking at the API docs I didn't expect it to make a copy given it seems like it's supposed to be a fresh context per task. I think it's neat, but unexpected and certainly looks like magic when looking at code like: https://github.com/DataDog/dd-trace-py/blob/v0.36.1/ddtrace/contrib/asyncio/wrappers.py#L52 which sets the current task's contextvar value, then calls the underlying create task, then swaps back. It seemed to me it wouldn't work because you had to execute under the context of the new task, but thanks to some trickery it ends up working :). TLDR would be nice if documented and clarified behavior between the two modes.

[asvetlov]

Yes, a task has a copy of contextvars by design.
A thread doesn't copy vars but has a fresh context for the sake of backward compatibility; otherwise, the behavior of libraries like decimal will be broken in a backward-incompatible way.
I can imagine an argument to threading.Thread to optionally pass a copy of the current thread's context vars into a spawned thread, maybe it is a nice improvement.