bpo-32436: Implement PEP 567 #5027

1st1 · 2017-12-28T05:24:21Z

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

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.
Changes in asyncio (including Modules/_asynciomodule.c).
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.
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 · 2018-01-02T01:13:32Z

Python/hamt.c

@@ -251,6 +251,29 @@ Further Reading
 3. Clojure's PersistentHashMap implementation:
   https://github.com/clojure/clojure/blob/master/src/jvm/
        clojure/lang/PersistentHashMap.java


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

methane · 2018-01-02T01:54:25Z

Python/hamt.c

+
+#ifdef Py_DEBUG
+        assert(iter->i_level >= 0);
+        iter->i


PyTuple_Pack()

Thanks, I completely forgot about it.

methane · 2018-01-02T03:51:02Z

Python/hamt.c

+
+#ifdef Py_DEBUG
+        assert(iter->i_level >= 0);
+        iter->i


Subclassing Hamt is allowed?

No, it's not even exposed to Python.

methane · 2018-01-02T04:07:01Z

Python/hamt.c

+    return (uint32_t)__builtin_popcountl(i);
+#elif defined(__clang__) && (__clang_major__ > 3)
+    return (uint32_t)__builtin_popcountl(i);
+#else


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

njsmith · 2018-01-04T22:01:33Z

Python/context.c

+{
+    /* Take hash of the name and XOR it with the default object hash.
+       We do this to ensure that even sequentially allocated ContextVar
+       objects have drastically different hashes.


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.

methane · 2018-01-05T06:21:58Z

Python/hamt.c

+static inline Py_ssize_t
+hamt_node_bitmap_count(PyHamtNode_Bitmap *node)
+{
+    return Py_SIZE(node) >> 1;


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

Fixed in a few places.

1st1 · 2018-01-17T14:01:59Z

@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 · 2018-01-17T23:52:23Z

Include/context.h

+
+
+/* This method is exposed only for CPython tests. Don not use it. */
+PyAPI_FUNC(PyObject *) _PyContext_NewHamtForTests(void);


Can it be moved to internal header?

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

(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 · 2018-01-18T07:53:07Z

Python/context.c

+    }
+
+    int res = _PyHamt_Eq(
+        ((PyContext *)v)->ctx_vars, ((PyContext *)w)->ctx_vars);


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

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

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 · 2018-01-18T11:36:32Z

Python/hamt.c

+            return NULL;
+        }
+        if (comp_err == 1) {  /* key == key_or_null */
+            comp_err = PyObject_RichCompareBool(val, val_or_node, Py_EQ);


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

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

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 · 2018-01-18T11:46:42Z

Py_UNREACHABLE() is macro for unreachable flow.

1st1 · 2018-01-22T15:47:29Z

Thank you, Inada-san.

1st1 · 2018-01-23T00:11:11Z

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

smurfix · 2018-02-15T14:15:49Z

Is there a pure-Python version of contextvars?

vltr · 2018-04-17T17:39:41Z

@smurfix see here

thehesiod · 2020-04-23T03:09:30Z

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 · 2020-04-23T08:59:34Z

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.

1st1 self-assigned this Dec 28, 2017

1st1 requested review from asvetlov, rhettinger, skrah and a team as code owners December 28, 2017 05:24

the-knights-who-say-ni added the CLA signed label Dec 28, 2017

bedevere-bot added the awaiting merge label Dec 28, 2017

1st1 removed request for a team, asvetlov, rhettinger and skrah December 28, 2017 05:35

1st1 added awaiting changes and removed awaiting merge labels Dec 28, 2017

1st1 force-pushed the pep567 branch 3 times, most recently from 384c38b to e439870 Compare December 30, 2017 03:38

methane reviewed Jan 2, 2018

View reviewed changes

njsmith reviewed Jan 4, 2018

View reviewed changes

methane reviewed Jan 5, 2018

View reviewed changes

1st1 force-pushed the pep567 branch 4 times, most recently from 23b9f67 to 62fedcc Compare January 16, 2018 17:44

methane reviewed Jan 18, 2018

View reviewed changes

1st1 added 17 commits January 22, 2018 10:46

ThreadState now refs to a Context object (fixes Context.run/__getitem__)

c4750a7

Document hamt_t enum types

b1c97f0

Add a test

4f10c5e

Add Context.copy() method

47d5a99

Harden PyContext_Exit to check that the current context matches

7549c6a

Simplify context_new() logic

db384f3

Token.reset() checks the context too now

e299b4c

Token.old_val -> Token.old_value

dad0651

Test that subclassing is prohibited

78b0361

Make ContextVar.reset raise an error for already used tokens

3673bd9

Drop unused file

9223883

Reword a comment

8c82581

Address Inada-sun comments

17fa9c0

Cache empty HAMT node

52dc25b

Don't GC-track ContextVars if name/default are not tracked

81ae871

Handle an error if rehashing fails

7070cca

PyContext_CopyCurrent() & PyContext_Copy() (sync with the PEP)

cc8873c

1st1 force-pushed the pep567 branch from 5975dd2 to cc8873c Compare January 22, 2018 15:46

Fix asynciomodule.c to use new PyContext_CopyCurrent

e353e95

1st1 merged commit f23746a into python:master Jan 23, 2018

bedevere-bot removed the awaiting merge label Jan 23, 2018

1st1 deleted the pep567 branch January 23, 2018 00:11

1st1 restored the pep567 branch January 23, 2018 00:11

fantix mentioned this pull request May 7, 2018

asyncio support MagicStack/contextvars#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-32436: Implement PEP 567 #5027

bpo-32436: Implement PEP 567 #5027

1st1 commented Dec 28, 2017 •

edited

Loading

methane Jan 2, 2018

1st1 Jan 15, 2018

methane Jan 2, 2018

1st1 Jan 15, 2018

methane Jan 2, 2018

1st1 Jan 15, 2018

methane Jan 2, 2018

1st1 Jan 15, 2018

njsmith Jan 4, 2018

1st1 Jan 15, 2018

methane Jan 5, 2018

1st1 Jan 15, 2018

1st1 commented Jan 17, 2018

methane left a comment •

edited

Loading

methane Jan 17, 2018

1st1 Jan 18, 2018

1st1 Jan 18, 2018 •

edited

Loading

methane Jan 18, 2018

1st1 Jan 18, 2018

1st1 Jan 18, 2018

methane Jan 18, 2018

methane Jan 18, 2018

1st1 Jan 18, 2018

methane commented Jan 18, 2018

1st1 commented Jan 22, 2018

1st1 commented Jan 23, 2018

smurfix commented Feb 15, 2018

vltr commented Apr 17, 2018

thehesiod commented Apr 23, 2020

asvetlov commented Apr 23, 2020



		/* This method is exposed only for CPython tests. Don not use it. */
		PyAPI_FUNC(PyObject *) _PyContext_NewHamtForTests(void);

bpo-32436: Implement PEP 567 #5027

bpo-32436: Implement PEP 567 #5027

Conversation

1st1 commented Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 commented Jan 17, 2018

methane left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

methane commented Jan 18, 2018

1st1 commented Jan 22, 2018

1st1 commented Jan 23, 2018

smurfix commented Feb 15, 2018

vltr commented Apr 17, 2018

thehesiod commented Apr 23, 2020

asvetlov commented Apr 23, 2020

1st1 commented Dec 28, 2017 •

edited

Loading

methane left a comment •

edited

Loading

1st1 Jan 18, 2018 •

edited

Loading