On Tue, Dec 12, 2017 at 10:36 PM, Guido van Rossum <gu...@python.org> wrote: > Some more feedback: > >> This proposal builds directly upon concepts originally introduced >> in :pep:`550`. > > The phrase "builds upon" typically implies that the other resource must be > read and understood first. I don't think that we should require PEP 550 for > understanding of PEP 567. Maybe "This proposal is a simplified version of > :pep:`550`." ?
I agree, "simplified version" is better. > >> The notion of "current value" deserves special consideration: >> different asynchronous tasks that exist and execute concurrently >> may have different values. This idea is well-known from thread-local >> storage but in this case the locality of the value is not always >> necessarily to a thread. Instead, there is the notion of the >> "current ``Context``" which is stored in thread-local storage, and >> is accessed via ``contextvars.get_context()`` function. >> Manipulation of the current ``Context`` is the responsibility of the >> task framework, e.g. asyncio. > > This begs two (related) questions: > - If it's stored in TLS, why isn't it equivalent to TLS? > - If it's read-only (as mentioned in the next paragraph) how can the > framework modify it? > > I realize the answers are clear, but at this point in the exposition you > haven't given the reader enough information to answer them, so this > paragraph may confuse readers. I'll think how to rephrase it. > >> Specification >> ============= >> [points 1, 2, 3] > > Shouldn't this also list Token? (It must be a class defined here so users > can declare the type of variables/arguments in their code representing these > tokens.) > >> The ``ContextVar`` class has the following constructor signature: >> ``ContextVar(name, *, default=no_default)``. > > I think a word or two about the provenance of `no_default` would be good. (I > think it's an internal singleton right?) Ditto for NO_DEFAULT in the C > implementation sketch. Fixed. > >> class Task: >> def __init__(self, coro): > > Do we need a keyword arg 'context=None' here too? (I'm not sure what would > be the use case, but somehow it stands out in comparison to call_later() > etc.) call_later() is low-level and it needs the 'context' argument as Task and Future use it in their implementation. It would be easy to add 'context' parameter to Task and loop.create_task(), but I don't know about any concrete use-case for that just yet. > >> CPython C API >> ------------- >> TBD > > Yeah, what about it? :-) I've added it: https://github.com/python/peps/pull/508/files I didn't want to get into too much detail about the C API until I have a working PR. Although I feel that the one I describe in the PEP now is very close to what we'll have. > >> The internal immutable dictionary for ``Context`` is implemented >> using Hash Array Mapped Tries (HAMT). They allow for O(log N) ``set`` >> operation, and for O(1) ``get_context()`` function. [...] > > I wonder if we can keep the HAMT out of the discussion at this point. I have > nothing against it, but given that you already say you're leaving out > optimizations and nothing in the pseudo code given here depends on them I > wonder if they shouldn't be mentioned later. (Also the appendix with the > perf analysis is the one thing that I think we can safely leave out, just > reference PEP 550 for this.) I've added a new section "Implementation Notes" that mentions HAMT and ContextVar.get() cache. Both refer to PEP 550's lengthy explanations. > >> class _ContextData > > Since this isn't a real class anyway I think the __mapping attribute might > as well be named _mapping. Ditto for other __variables later. Done. Yury _______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com