New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-32436: Implement PEP 567 #5027

Merged
merged 24 commits into from Jan 23, 2018

Conversation

Projects
None yet
7 participants
@1st1
Member

1st1 commented Dec 28, 2017

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

@1st1 1st1 self-assigned this Dec 28, 2017

@1st1 1st1 requested review from asvetlov, rhettinger, skrah and python/windows-team as code owners Dec 28, 2017

@1st1 1st1 removed request for python/windows-team, asvetlov, rhettinger and skrah Dec 28, 2017

@1st1 1st1 force-pushed the 1st1:pep567 branch 3 times, most recently from 384c38b to e439870 Dec 28, 2017

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

This comment has been minimized.

@methane

methane Jan 2, 2018

Member

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

This comment has been minimized.

@1st1

1st1 Jan 15, 2018

Member

sure


#ifdef Py_DEBUG
assert(iter->i_level >= 0);
iter->i_nodes[iter->i_level] = NULL;

This comment has been minimized.

@methane

methane Jan 2, 2018

Member

PyTuple_Pack()

This comment has been minimized.

@1st1

1st1 Jan 15, 2018

Member

Thanks, I completely forgot about it.


#ifdef Py_DEBUG
assert(iter->i_level >= 0);
iter->i_nodes[iter->i_level] = NULL;

This comment has been minimized.

@methane

methane Jan 2, 2018

Member

Subclassing Hamt is allowed?

This comment has been minimized.

@1st1

1st1 Jan 15, 2018

Member

No, it's not even exposed to Python.

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

This comment has been minimized.

@1st1

1st1 Jan 15, 2018

Member

Updated

{
/* 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.

This comment has been minimized.

@njsmith

njsmith Jan 4, 2018

Contributor

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.

This comment has been minimized.

@1st1

1st1 Jan 15, 2018

Member

Updated

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

This comment has been minimized.

@methane

methane Jan 5, 2018

Member

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

This comment has been minimized.

@1st1

1st1 Jan 15, 2018

Member

Fixed in a few places.

@1st1 1st1 force-pushed the 1st1:pep567 branch 4 times, most recently from 23b9f67 to 62fedcc Jan 15, 2018

@1st1

This comment has been minimized.

Member

1st1 commented Jan 17, 2018

@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]
 }                                                                                                                                                                                             ^


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

This comment has been minimized.

@methane

methane Jan 18, 2018

Member

Can it be moved to internal header?

This comment has been minimized.

@1st1

1st1 Jan 18, 2018

Member

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

This comment has been minimized.

@1st1

1st1 Jan 18, 2018

Member

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

}

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

This comment has been minimized.

@methane

methane Jan 18, 2018

Member

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

This comment has been minimized.

@1st1

1st1 Jan 18, 2018

Member

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

This comment has been minimized.

@1st1

1st1 Jan 18, 2018

Member

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.

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

This comment has been minimized.

@methane

methane Jan 18, 2018

Member

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

This comment has been minimized.

@methane

methane Jan 18, 2018

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

This comment has been minimized.

@1st1

1st1 Jan 18, 2018

Member

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

This comment has been minimized.

Member

methane commented Jan 18, 2018

Py_UNREACHABLE() is macro for unreachable flow.

@1st1 1st1 force-pushed the 1st1:pep567 branch from 5975dd2 to cc8873c Jan 22, 2018

@1st1

This comment has been minimized.

Member

1st1 commented Jan 22, 2018

Thank you, Inada-san.

@1st1

This comment has been minimized.

Member

1st1 commented Jan 23, 2018

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

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

4 checks passed

bedevere/issue-number Issue number 32436 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@1st1 1st1 deleted the 1st1:pep567 branch Jan 23, 2018

@1st1 1st1 restored the 1st1:pep567 branch Jan 23, 2018

@smurfix

This comment has been minimized.

smurfix commented Feb 15, 2018

Is there a pure-Python version of contextvars?

@vltr

This comment has been minimized.

vltr commented Apr 17, 2018

@fantix fantix referenced this pull request May 7, 2018

Open

asyncio support #2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment