Hi Nick, On 29/01/2021 1:21 pm, Nick Coghlan wrote:

On Thu, 28 Jan 2021, 11:18 pm Mark Shannon, mailto:mark@hotpy.org> wrote:

Hi Nick,

Regarding `f_locals` PEP 558 states:

""" Instead of being a direct reference to the internal dynamic snapshot used to populate the independent snapshots returned by locals(), frame.f_locals will be updated to instead return a dedicated proxy type (implemented as a private subclass of the existing types.MappingProxyType) that has two internal attributes not exposed as part of the Python runtime API:

*    mapping: an implicitly updated snapshot of the function local variables and closure references, as well as any arbitrary items that have been set via the mapping API, even if they don't have storage allocated for them on the underlying frame *    frame: the underlying frame that the snapshot is for

"""

This seems rather complex, and consequently fragile. I fear that this is just going to result in different bugs, rather than fixing the bugs it proposes to fix.

Why not just make `f_local` a direct view on the underlying frame? It would be simpler to understand, more robust, and should perform better.

The concern I have with the simplification is that I don't know what would break if trace hooks lost the ability to stash additional state that the compiler doesn't know anything about in f_locals.

Do you know of any tools do that? It seems highly unlikely to me. In fact tool authors are asking for less state, not more: https://bugs.python.org/issue42197 (for performance, rather than correctness reasons, I should note)

Rather than trying to assess how common such usage is, and whether we care about breaking any use cases that people have for it, I instead elected to just keep it working.

"keep it working" implies that it works now. It doesn't. https://bugs.python.org/issue30744

The extra implementation complexity beyond what's already needed to cope with closure cells also isn't that much.

It is a lot more complex, because you need to worry about coherency. With a direct proxy coherency is not an issue. https://bugs.python.org/issue30744 shows that the interactions between stateful local caches, nonlocals, and concurrency is complex and likely to be buggy. If you are going to substitute one stateful construct for another, then you need to provide evidence that it won't introduce new bugs. Cheers, Mark.

On Sat, 30 Jan 2021 at 10:30, Nick Coghlan wrote:

On Sat, 30 Jan 2021, 12:13 am Mark Shannon, 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.

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 Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia

On Sat, 30 Jan 2021, 10:18 pm Mark Shannon, wrote:

Hi Nick,

...

On Sat, 30 Jan 2021 at 10:30, Nick Coghlan wrote:

...

On Sat, 30 Jan 2021, 12:13 am Mark Shannon, wrote:

...

With a direct proxy coherency is not an issue.

For things in the frame, it *is* a direct proxy - reads pull from the

On 30/01/2021 4:44 am, Nick Coghlan wrote: 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.

The read side is currently completely handled by the existing MappingProxyType, so the frame locals proxy subclass only implements the extra MutableMapping bits. "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.

We don't support running the same *frame* from multiple threads, so only one thread can be updating the frame directly, and the proxy writes to both the frame and the snapshot. But avoiding the need to update the entire snapshot to read specific variables from the frame would definitely be a compelling technical benefit regardless.

...

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?

f_locals *isn't* broken a lot of the time. The PEP aims to keep that code working unchanged rather than forcing people to modify their code to handle problems they may not have.

The break in compatibility with locals() seems much more intrusive, yet you are OK with that (as am I).

PyEval_GetLocals() is part of the stable ABI and returns a borrowed reference. That means there are a lot of implementation restrictions around keeping that API working. A follow-up PEP could propose deprecating and removing the API as intrinsically broken, but I don't want to go that far in PEP 558. By contrast, for the Python level API, Nathaniel was able to make a compelling case that most locals() usage would at worst suffer a performance degradation with the semantic change, and at least some use cases would see latent defects transparently fixed. Cheers, Nick.

Cheers, Mark.

On Sun, 31 Jan 2021 at 14:09, Nick Coghlan wrote:

On Sat, 30 Jan 2021, 10:55 pm Nick Coghlan, wrote:

...

On Sat, 30 Jan 2021, 10:18 pm Mark Shannon, wrote:

...

The break in compatibility with locals() seems much more intrusive, yet you are OK with that (as am I). PyEval_GetLocals() is part of the stable ABI and returns a borrowed reference. That means there are a lot of implementation restrictions around keeping that API working. A follow-up PEP could propose deprecating and removing the API as intrinsically broken, but I don't want to go that far in PEP 558. After sleeping on this, I'm now convinced that you're right, and we can reasonably drop the "stash extra info in the frame locals snapshot" feature from the *new* optimised frame locals C API.

The PR at https://github.com/python/peps/pull/1787 has been updated to accept your suggestion into the PEP rather than listing it as an open question. It also cleans up a bunch more references to the old "store the proxy on the frame" idea that I missed when moving away from the idea. I haven't updated the reference implementation at https://github.com/python/cpython/pull/3640/files to match though, and don't know when I might get to doing that myself. If you're interested in tackling that, the offer of PEP co-authorship definitely still stands :) While I haven't listed it in the PR as an open issue, there is one potential point for simplification that I haven't resolved yet: whether the new APIs should *completely* ignore the shared dynamic snapshot that ``PyEval_GetLocals()`` uses. At the moment, most of the APIs (including the Python ``locals()`` builtin) will still see extra values written via that backwards compatibility interface, as they implicitly update it, and then use it to generate their final return value. The only APIs that will ignore it completely are the Python level frame.f_locals, and the C level PyLocals_GetView() and PyFrame_GetLocalsView(), as those rely on the new fast locals proxy, and bypass the dynamic snapshot. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia

On Wed, 3 Feb 2021, 10:16 pm Sven R. Kunze, wrote:

Hi Mark,

I've been working on a project heavily relying on frame.f_locals.

Are you planning to remove it?

No, PEP 558 doesn't remove it, it enhances it to be a live view of the frame state instead of an inconsistently updated snapshot. The potential incompatibility Mark is referring to is the fact that even optimised frames currently allow writing to arbitrary keys in frame.f_locals and making the bound values visible to the locals() builtin and other consumers of frame.f_locals. For PEP 558, it's an open question as to whether that behaviour will become limited to the PyEval_GetLocals() backwards compatible C API, with the updated Python frame API instead throwing KeyError for attempts to write to unknown keys on optimised frames. Regards, Nick.

On 30.01.21 13:18, Mark Shannon wrote:

...

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

Best, Sven

Hi Sven, On 04/02/2021 9:06 am, Sven R. Kunze wrote:

On 03.02.21 23:37, Nick Coghlan wrote:

...

No, PEP 558 doesn't remove it, it enhances it to be a live view of the frame state instead of an inconsistently updated snapshot.

As long as it is possible to **write** to existing keys to **add new keys** to frame.f_locals, I am actually quite happy.

Out of interest, why would you want to add new keys to the locals of a function frame? The function will never be able to use those values.

...

The potential incompatibility Mark is referring to is the fact that even optimised frames currently allow writing to arbitrary keys in frame.f_locals and making the bound values visible to the locals() builtin and other consumers of frame.f_locals.

For PEP 558, it's an open question as to whether that behaviour will become limited to the PyEval_GetLocals() backwards compatible C API, with the updated Python frame API instead throwing KeyError for attempts to write to unknown keys on optimised frames.

So, it seems the restricting behavior is for special cases only (not including with-statements which I would care about).

I just wanted to mention that accessing problematic parts of the interpreter's internals can be quite helpful when doing concept-proofing or working the way through until a "batteries-included" solution exist.

Thanks, Sven