GeezerVison is showing github.com — add support for watching writes to selected dictionaries · Issue #91052 · python/cpython
add support for watching writes to selected dictionaries #91052Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = None
created_at = <Date 2022-03-01.22:19:05.128>
labels = ['expert-C-API', '3.11']
title = 'add support for watching writes to selected dictionaries'
updated_at = <Date 2022-03-15.17:17:40.946>
user = 'https://github.com/carljm' bugs.python.org fields:
activity = <Date 2022-03-15.17:17:40.946>
actor = 'carljm'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['C API']
creation = <Date 2022-03-01.22:19:05.128>
creator = 'carljm'
dependencies = []
files = []
hgrepos = []
issue_num = 46896
keywords = ['patch']
message_count = 22.0
messages = ['414307', '414330', '414462', '414466', '414467', '414468', '414470', '414531', '414540', '414551', '414803', '414804', '414808', '414810', '414839', '414877', '414882', '414898', '414899', '415253', '415261', '415268']
nosy_count = 9.0
nosy_names = ['gvanrossum', 'barry', 'gregory.p.smith', 'carljm', 'dino.viehland', 'Mark.Shannon', 'brandtbucher', 'Dennis Sweeney', 'itamaro']
pr_nums = ['31787']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue46896'
versions = ['Python 3.11'] Reacted by Chad Furman, Erlend E. Aasland and Techcable Activitycarljm commented on Mar 1, 2022 CPython extensions providing optimized execution of Python bytecode (e.g. the Cinder JIT), or even CPython itself (e.g. the faster-cpython project) may wish to inline-cache access to frequently-read and rarely-changed namespaces, e.g. module globals. Rather than requiring a dict version guard on every cached read, the best-performing way to do this is is to mark the dictionary as “watched” and set a callback on writes to watched dictionaries. This optimizes the cached-read fast-path at a small cost to the (relatively infrequent and usually less perf sensitive) write path.
We have an implementation of this in Cinder ( https://docs.google.com/document/d/1l8I-FDE1xrIShm9eSNJqsGmY_VanMDX5-aK_gujhYBI/edit#heading=h.n2fcxgq6ypwl ), used already by the Cinder JIT and its specializing interpreter. We would like to make the Cinder JIT available as a third-party extension to CPython ( https://docs.google.com/document/d/1l8I-FDE1xrIShm9eSNJqsGmY_VanMDX5-aK_gujhYBI/ ), and so we are interested in adding dict watchers to core CPython.
The intention in this issue is not to add any specific optimization or cache (yet); just the ability to mark a dictionary as “watched” and set a write callback.
The callback will be global, not per-dictionary (no extra function pointer stored in every dict). CPython will track only one global callback; it is a well-behaved client’s responsibility to check if a callback is already set when setting a new one, and daisy-chain to the previous callback if so. Given that multiple clients may mark dictionaries as watched, a dict watcher callback may receive events for dictionaries that were marked as watched by other clients, and should handle this gracefully.
There is no provision in the API for “un-watching” a watched dictionary; such an API could not be used safely in the face of potentially multiple dict-watching clients.
The Cinder implementation marks dictionaries as watched using the least bit of the dictionary version (so version increments by 2); this also avoids any additional memory usage for marking a dict as watched.
Initial proposed API, comments welcome:
// Mark given dictionary as "watched" (global callback will be called if it is modified)
void PyDict_Watch(PyObject* dict);
// Check if given dictionary is already watched
int PyDict_IsWatched(PyObject* dict);
typedef enum {
PYDICT_EVENT_CLEARED,
PYDICT_EVENT_DEALLOCED,
PYDICT_EVENT_MODIFIED
} PyDict_WatchEvent;
// Callback to be invoked when a watched dict is cleared, dealloced, or modified.
// In clear/dealloc case, key and new_value will be NULL. Otherwise, new_value will be the
// new value for key, NULL if key is being deleted.
typedef void(*PyDict_WatchCallback)(PyDict_WatchEvent event, PyObject* dict, PyObject* key, PyObject* new_value);
// Set new global watch callback; supply NULL to clear callback
void PyDict_SetWatchCallback(PyDict_WatchCallback callback);
// Get existing global watch callback
PyDict_WatchCallback PyDict_GetWatchCallback();The callback will be called immediately before the modification to the dict takes effect, thus the callback will also have access to the prior state of the dict. changed the title [-]add support for watching writes to selecting dictionaries[/-] [+]add support for watching writes to selected dictionaries[/+] on Mar 1, 2022changed the title [-]add support for watching writes to selecting dictionaries[/-] [+]add support for watching writes to selected dictionaries[/+] on Mar 1, 2022gpshead commented on Mar 2, 2022 At first quick glance, this makes sense and the API looks reasonable.
Question: what happens on interpreter shutdown?
Shutdown obviously finalized and clears out most all dicts. I guess the C callback simply gets called for each of these? That makes sense. Just wondering if there are any ramifications of that. The callback is in C so it shouldn't have issues with this.
A pyperformance suite run on an interpreter modified to support this but having no callbacks registered would be useful. (basically judging if there is measurable overhead added by the watched bit check - I doubt it'll be noticeable in most code) carljm commented on Mar 3, 2022 Thanks gps! Working on a PR and will collect pyperformance data as well.
We haven't observed any issues in Cinder with the callback just being called at shutdown, too, but if there are problems with that it should be possible to just have CPython clear the callback at shutdown time. brandtbucher commented on Mar 3, 2022
CPython will track only one global callback; it is a well-behaved client’s responsibility to check if a callback is already set when setting a new one, and daisy-chain to the previous callback if so.
Hm, this is a bit scary. Could we (or others) end up with unguarded stale caches if some buggy extension forgets to chain the calls correctly?
Core CPython seems most at risk of this, since we would most likely be registered first. brandtbucher commented on Mar 3, 2022 Also, when you say "only one global callback": does that mean per-interpreter, or per-process? carljm commented on Mar 3, 2022
Could we (or others) end up with unguarded stale caches if some buggy extension forgets to chain the calls correctly?
Yes. I can really go either way on this. I initially opted for simplicity in the core support at the cost of asking a bit more of clients, on the theory that a) there are lots of ways for a buggy C extension to cause crashes with bad use of the C API, and b) I don't expect there to be very many extensions using this API. But it's also true that the consequences of a mistake here could be hard to debug (and easily blamed to the wrong place), and there might turn out to be more clients for dict-watching than I expect! If the consensus is to prefer CPython tracking an array of callbacks instead, we can try that.
when you say "only one global callback": does that mean per-interpreter, or per-process?
Good question! The currently proposed API suggests per-process, but it's not a question I've given a lot of thought to yet; open to suggestions. It seems like in general the preference is to avoid global state and instead tie things to an interpreter instance? I'll need to do a bit of research to understand exactly how that would affect the implementation. Doesn't seem like it should be a problem, though it might make the lookup at write time to see if we have a callback a bit slower. gpshead commented on Mar 3, 2022 Per interpreter seems best.
If someone using this feature writes a buggy implementation of a callback that doesn't chain reliably, that is a bug in their code and all of the fallout from that is "just" a bug to be fixed in said code.
Think of it like a C signal handler, the OS doesn't chain for you - it is the signal handler installer's responsibility to chain to the previous one. Not an unusual pattern for callback hooks in C.
We already have such global C callback hooks in SetProfile and SetTrace. https://docs.python.org/3/c-api/init.html#profiling-and-tracing
Those don't even provide for chaining. We could do the same here and not let a hook be set more than once instead of providing a Get API to allow for manual chaining. That seems less useful. markshannon commented on Mar 4, 2022 Why so coarse?
Getting a notification for every change of a global in module, is likely to make use the use of global variables extremely expensive.
var = 0
CONST = 1
def foo(...):
...
I may well want to be notified if foo or CONST gets modified, but performance could suffer badly if we make a callback every time var is changed.
--------------
What happens if a watched dictionary is modified in a callback?
--------------
How do you plan to implement this? Steal a bit from ma_version_tag or replace ma_version_tag?
If you replace ma_version_tag this could actually speed things up a tad.
You'd probably need a PEP to replace PEP-509, but I think this may need a PEP anyway. brandtbucher commented on Mar 4, 2022
Why so coarse?
Getting a notification for every change of a global in module, is likely to make use the use of global variables extremely expensive.
Perhaps a compromise is possible here: one global group/chain of callbacks registered for all dictionaries in an interpreter seems reasonable (to keep overhead low), but we could reduce the number of times it’s actually called by, say, tagging specific values of specific dictionaries to be watched.
For example, we could just tag the low bit of any pointer in a dictionary’s values that we want to be notified of changes to. Something like that seems like it could really keep the cost of watching down without sacrificing power. carljm commented on Sep 15, 2022 I am on the hook, just not finding time... I definitely plan to make it happen in time for 3.12 though. carljm commented on Oct 7, 2022 Reacted by Guido van Rossum, mpage and Виталий Дмитриев
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|