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.
Cheers, Mark.
On Thu, 28 Jan 2021, 11:18 pm Mark Shannon, 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.
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.
The extra implementation complexity beyond what's already needed to cope with closure cells also isn't that much.
Cheers, Nick.
Cheers, Mark.
Hi Nick,
On 29/01/2021 1:21 pm, Nick Coghlan wrote:
On Thu, 28 Jan 2021, 11:18 pm Mark Shannon, <mark@hotpy.org 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, 12:13 am Mark Shannon, mark@hotpy.org wrote:
Hi
It is a lot more complex, because you need to worry about coherency. 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...
The technical pay-off from having the mapping there is that most read-only dict operations are handled by the existing MappingProxyType.
If you drop the mapping, then you lose any code sharing, and have to implement the full mapping API from scratch, instead of just the mutation and single key lookup parts.
You also reduce compatibility with the status quo, since writes to unknown keys will fail rather than being stored solely in the mapping section, and there's also no way to preserve the "PyEval_GetLocals()" C API, which is expected to return a borrowed reference to a plain Python dict stored in the C level "f_locals" field on the frame struct.
The cache coherency issues on the bulk mapping APIs this perpetuates also aren't any worse than those on the locals() built-in itself: they'll see the state as of the last snapshot update rather than the live state on the frame (which may not exist any more if the proxy outlives the underlying frame).
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.
Cheers, Nick.
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.
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.
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.
On Sat, 30 Jan 2021, 10:18 pm Mark Shannon, mark@hotpy.org wrote:
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.
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 Sat, 30 Jan 2021, 10:55 pm Nick Coghlan, ncoghlan@gmail.com wrote:
On Sat, 30 Jan 2021, 10:18 pm Mark Shannon, mark@hotpy.org 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.
What I realised is that even if we don't offer that feature on the new frame locals proxy type, the *old* C API can still support it, unless & until that API is deprecated, as PyEval_GetLocals() bypasses the new API and accesses the frame state directly.
Cheers, Nick.
On Sun, 31 Jan 2021 at 14:09, Nick Coghlan ncoghlan@gmail.com wrote:
On Sat, 30 Jan 2021, 10:55 pm Nick Coghlan, ncoghlan@gmail.com wrote:
On Sat, 30 Jan 2021, 10:18 pm Mark Shannon, mark@hotpy.org 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.
Hi Mark,
I've been working on a project heavily relying on frame.f_locals.
Are you planning to remove it?
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
On Wed, 3 Feb 2021, 10:16 pm Sven R. Kunze, srkunze@mail.de 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
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.
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
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
Hi Mark,
On 04.02.21 12:47, Mark Shannon wrote:
Hi Sven,
On 04/02/2021 9:06 am, Sven R. Kunze wrote:
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?
I use it for remote execution in human-friendly manner.
I plan to opensource the lib for everybody to use, so I was a worried that this change could break it.
The function will never be able to use those values.
I realize quite now that the use-case usually is on module-level where locals=globals:
import sys frame=sys._getframe(0) frame.f_locals['testvar']='testvalue' print(testvar)
testvalue
So, setting a var was never an issue; also probably because it's seldomly used in this context.
Funny enough, that the lib would even start to work properly when functions-locals would be writable.
Regards, Sven
participants (3)
-
Mark Shannon -
Nick Coghlan -
Sven R. Kunze