
Hi Nick, On 30/01/2021 4:44 am, Nick Coghlan wrote:
On Sat, 30 Jan 2021 at 10:30, Nick Coghlan <ncoghlan@gmail.com> wrote:
On Sat, 30 Jan 2021, 12:13 am Mark Shannon, <mark@hotpy.org> wrote:
With a direct proxy coherency is not an issue.
For things in the frame, it *is* a direct proxy - reads pull from the frame object, and writes go to both the frame object and the mapping: https://github.com/python/cpython/pull/3640/files#diff-7b8cef249e5cca077d30d...
Reviewing the code again, I'd misremembered how the draft implementation works - right now, reads are relying on the snapshot being up to date.
Given the resulting C API compatibility break and the loss of code sharing, I don't think it's a net win to drop the extra storage.
I'm not sure what you mean by "loss of code sharing", but I don't see how a mapping that is both a cache and a proxy can be easier to implement than a mapping that is just a proxy. "reads are relying on the snapshot being up to date", doesn't that just mean that reads can be wrong? There is no way to keep a snapshot reliably up to date in the presence of threads.
For now, I have a PR adding this as an open question: https://github.com/python/peps/pull/1787/files
Given the performance benefit of being able to more reasonably drop the implicit call to `PyFrame_LocalsToFast()`, I'm mostly convinced that switching reads to pull from the frame if possible is the right thing to do, even if it reduces the amount of code that can be inherited without modification from MappingProxyType.
The API compatibility concerns would mean the extra mapping store still needed to stick around, but it would only be used for: > * providing backwards compatibility for the `PyEval_GetLocals()` and C-level `f_locals` interfaces * reading and writing names that don't have entries in the `fast_refs` mapping * writing shadow updates for names in the `fast_refs` mapping
Given that f_locals is broken, why is keeping compatibility for this obscure, and probably unused case worthwhile? The break in compatibility with locals() seems much more intrusive, yet you are OK with that (as am I). Cheers, Mark.