Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-32436: Implement PEP 567 #5027
Conversation
1st1
self-assigned this
Dec 28, 2017
1st1
requested review from
asvetlov,
rhettinger,
skrah and
python/windows-team
as
code owners
Dec 28, 2017
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
python/windows-team,
asvetlov,
rhettinger and
skrah
Dec 28, 2017
1st1
added
awaiting changes
and removed
awaiting merge
labels
Dec 28, 2017
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
||
#ifdef Py_DEBUG | ||
assert(iter->i_level >= 0); | ||
iter->i_nodes[iter->i_level] = NULL; |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
||
#ifdef Py_DEBUG | ||
assert(iter->i_level >= 0); | ||
iter->i_nodes[iter->i_level] = NULL; |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
return (uint32_t)__builtin_popcountl(i); | ||
#elif defined(__clang__) && (__clang_major__ > 3) | ||
return (uint32_t)__builtin_popcountl(i); | ||
#else |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
{ | ||
/* 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.
This comment has been minimized.
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.
This comment has been minimized.
static inline Py_ssize_t | ||
hamt_node_bitmap_count(PyHamtNode_Bitmap *node) | ||
{ | ||
return Py_SIZE(node) >> 1; |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
1st1
force-pushed the
1st1:pep567
branch
4 times, most recently
from
23b9f67
to
62fedcc
Jan 15, 2018
This comment has been minimized.
This comment has been minimized.
@methane Thanks for the initial review, Inada-san. Do you have any other comments/suggestions? |
methane
reviewed
Jan 18, 2018
•
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
|
1st1
added some commits
Jan 15, 2018
1st1
force-pushed the
1st1:pep567
branch
from
5975dd2
to
cc8873c
Jan 22, 2018
This comment has been minimized.
This comment has been minimized.
Thank you, Inada-san. |
This comment has been minimized.
This comment has been minimized.
@gvanrossum has just approved the PEP so I guess it's time to merge this! |
1st1
merged commit f23746a
into
python:master
Jan 23, 2018
bedevere-bot
removed
the
awaiting merge
label
Jan 23, 2018
1st1
deleted the
1st1:pep567
branch
Jan 23, 2018
1st1
restored the
1st1:pep567
branch
Jan 23, 2018
This comment has been minimized.
This comment has been minimized.
smurfix
commented
Feb 15, 2018
Is there a pure-Python version of |
1st1 commentedDec 28, 2017
•
edited
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
andLib/contextvars.py
: the contextvars module.Changes in asyncio (including
Modules/_asynciomodule.c
).HAMT immutable dict implementation. Files of interest:
Python/hamt.c
andInclude/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
andpystate.c
, and modifications of build/make scripts.Tests in
test_context
andtest_asyncio
pass the refleaks test mode.https://bugs.python.org/issue32436