gh-91053: Add an optional callback that is invoked whenever a function is modified #97834

mpage · 2022-10-04T16:54:01Z

JIT compilers may need to invalidate compiled code when a function is modified (e.g. if its code object is modified). This adds the ability to set a callback that, when set, is called each time a function is modified.

Pyperformance results look neutral: https://gist.github.com/mpage/0ffeaa76fa759c8230e91415eec44be3

Issue: Add API to allow extensions to set callback function on creation, modification, and destruction of PyFunctionObject #91053

JIT compilers may need to invalidate compiled code when a function is modified (e.g. if its code object is modified). This adds the ability to set a callback that, when set, is called each time a function is modified.

bedevere-bot · 2022-10-04T16:54:07Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

cpython-cla-bot · 2022-10-04T16:54:32Z

All commit authors signed the Contributor License Agreement.

bedevere-bot · 2022-10-04T16:59:10Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

Modules/_testcapi/func_events.c

Objects/funcobject.c

The primary use case for this is a JIT, which will be per-runtime, not per-interpreter, so move the callback onto the runtime.

bedevere-bot · 2022-10-05T00:39:20Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

… be deleted This should would across platforms and implementations.

mpage · 2022-10-06T00:04:55Z

pyperformance results for the updated version where the callback is stored on the interpreter state still look perf-neutral: https://gist.github.com/mpage/26d2efdf0f6f166cf536c1ee9e5841a9

I wrote a microbenchmark to attempt to measure the overhead this imposes on function creation when no callback is set: https://gist.github.com/mpage/b3006c3b490460272d7a95438345b3f3

The overhead looks like it's small enough to be lost in the noise. Happy to use a different benchmark or a quieter machine if one is available.

Upstream:

mpage@ip-172-31-41-159:~$ ./upstream/python bench_comprehensions.py
0.07469153602141887
mpage@ip-172-31-41-159:~$ ./upstream/python bench_comprehensions.py
0.07751996896695346
mpage@ip-172-31-41-159:~$ ./upstream/python bench_comprehensions.py
0.07642328296788037
mpage@ip-172-31-41-159:~$ ./upstream/python bench_comprehensions.py
0.0777301819762215
mpage@ip-172-31-41-159:~$ ./upstream/python bench_comprehensions.py
0.07707617298001423

This PR with no modified callback set:

mpage@ip-172-31-41-159:~$ ./cpython/python bench_comprehensions.py
0.07540378603152931
mpage@ip-172-31-41-159:~$ ./cpython/python bench_comprehensions.py
0.07542523700976744
mpage@ip-172-31-41-159:~$ ./cpython/python bench_comprehensions.py
0.07418339903233573
mpage@ip-172-31-41-159:~$ ./cpython/python bench_comprehensions.py
0.07634932000655681
mpage@ip-172-31-41-159:~$ ./cpython/python bench_comprehensions.py
0.07911138999043033

carljm

You should also add documentation for the new API in Doc/c-api/function.rst; don't forget the versionadded tag which I missed in the original dict watchers PR, too, and also add mention in Doc/whatsnew/3.12.rst.

Also it's worth running your added tests with -R just to ensure no accidental refleaks; CI won't catch that until post-land.

carljm · 2022-10-07T18:26:38Z

Include/cpython/funcobject.h

+ *
+ * Pass NULL to clear the callback.
+ */
+PyAPI_FUNC(void) PyFunction_SetEventCallback(PyFunction_EventCallback callback);


I think we are going to want to support multiple watchers (up to 8, to match dict and type watchers), which means the API here should look like int PyFunction_AddWatcher which gives you back an id 0-7 (or -1 and sets an exception if there are no more watcher IDs available), and also PyFunction_ClearWatcher(int watcher_id) to clear a watcher. Can look at the dict watcher API in Include/cpython/dictobject.h and Python/dictobject.c to make it as parallel as we can. (Obviously with the difference that here we aren't doing per-function watching so there's no equivalent to PyDict_Watch API.)

carljm · 2022-10-07T18:27:08Z

Include/internal/pycore_interp.h

@@ -175,6 +175,9 @@ struct _is {
 struct types_state types;
 struct callable_cache callable_cache;

+
+ PyFunction_EventCallback func_event_callback;


So this will be PyFunction_EventCallback func_event_callbacks[FUNC_MAX_WATCHERS]; or similar.

carljm · 2022-10-07T18:28:29Z

Objects/funcobject.c

+void
+PyFunction_SetEventCallback(PyFunction_EventCallback callback)
+{
+ PyThreadState *tstate = _PyThreadState_GET();


You can also do PyInterpreterState *interp = _PyInterpreterState_GET(); directly since that's all you're using.

…yncioTestCase (python#93369) Lay the foundation for further work in `asyncio.test_streams`.

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM> Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

Doc: Add references to PEP 686.

) * fix documentation for _thread.lock.acquire * update formatting of _thread.lock.acquire() doc

…eError suggestions (python#97022) Relevant tests moved from test_exceptions to test_traceback to be able to compare both implementations. Co-authored-by: Carl Friedrich Bolz-Tereick <cfbolz@gmx.de>

…aryFile (pythonGH-97015)

mpage · 2022-10-11T00:18:25Z

Ugh I botched rebasing this. Sorry for the noise everyone. Will open a new PR.

carljm · 2022-10-11T02:54:59Z

@mpage once there's been some reviews you should always merge from main, never rebase

bedevere-bot added the awaiting review label Oct 4, 2022

Merge branch 'main' into func-modified-cb

18329a1

JelleZijlstra reviewed Oct 4, 2022

View reviewed changes

Modules/_testcapi/func_events.c Outdated Show resolved Hide resolved

Objects/funcobject.c Outdated Show resolved Hide resolved

mpage added 3 commits October 4, 2022 17:39

Fix the build on windows

501c4dd

Fix refcounting for the result of vectorcall

b727aa2

Move callback into _PyRuntimeState

e3a8230

The primary use case for this is a JIT, which will be per-runtime, not per-interpreter, so move the callback onto the runtime.

mpage requested a review from a team as a code owner October 5, 2022 00:39

mpage marked this pull request as draft October 5, 2022 01:42

mpage added 4 commits October 5, 2022 10:51

Handle multiple events being dispatched

b2e20ef

Add NEWS entry

0a30690

Make the callback per-interpreter, rather than per-runtime

73dd809

Call the builtin id to return the id for a function that's about to…

4fa1cc8

… be deleted This should would across platforms and implementations.

mpage marked this pull request as ready for review October 5, 2022 23:59

Fix prototype for restore_func_event_callback

80f49f0

blockchainGuru1018 approved these changes Oct 6, 2022

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Oct 6, 2022

carljm requested changes Oct 7, 2022

View reviewed changes

arhadthedev and others added 6 commits October 10, 2022 17:16

pythongh-93357: Start porting asyncio server test cases to IsolatedAs…

81fcd2c

…yncioTestCase (python#93369) Lay the foundation for further work in `asyncio.test_streams`.

pythonGH-95913: Update what's new in 3.11 for asyncio (python#97806)

cc60df0

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM> Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

pythongh-90301: Doc: Add references to PEP 686 (python#96816)

39bef03

Doc: Add references to PEP 686.

pythongh-96448: fix documentation for _thread.lock.acquire (python#96449

de65e64

) * fix documentation for _thread.lock.acquire * update formatting of _thread.lock.acquire() doc

pythongh-97008: Add a Python implementation of AttributeError and Nam…

c00e5c5

…eError suggestions (python#97022) Relevant tests moved from test_exceptions to test_traceback to be able to compare both implementations. Co-authored-by: Carl Friedrich Bolz-Tereick <cfbolz@gmx.de>

pythongh-58451: Add optional delete_on_close parameter to NamedTempor…

75af114

…aryFile (pythonGH-97015)

mpage marked this pull request as draft October 11, 2022 00:17

mpage closed this Oct 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-91053: Add an optional callback that is invoked whenever a function is modified #97834

gh-91053: Add an optional callback that is invoked whenever a function is modified #97834

mpage commented Oct 4, 2022 •

edited by bedevere-bot

Loading

bedevere-bot commented Oct 4, 2022

cpython-cla-bot bot commented Oct 4, 2022 •

edited

Loading

bedevere-bot commented Oct 4, 2022

bedevere-bot commented Oct 5, 2022

mpage commented Oct 6, 2022

carljm left a comment

carljm Oct 7, 2022

carljm Oct 7, 2022

carljm Oct 7, 2022

mpage commented Oct 11, 2022

carljm commented Oct 11, 2022

gh-91053: Add an optional callback that is invoked whenever a function is modified #97834

gh-91053: Add an optional callback that is invoked whenever a function is modified #97834

Conversation

mpage commented Oct 4, 2022 • edited by bedevere-bot Loading

bedevere-bot commented Oct 4, 2022

cpython-cla-bot bot commented Oct 4, 2022 • edited Loading

bedevere-bot commented Oct 4, 2022

bedevere-bot commented Oct 5, 2022

mpage commented Oct 6, 2022

carljm left a comment

Choose a reason for hiding this comment

carljm Oct 7, 2022

Choose a reason for hiding this comment

carljm Oct 7, 2022

Choose a reason for hiding this comment

carljm Oct 7, 2022

Choose a reason for hiding this comment

mpage commented Oct 11, 2022

carljm commented Oct 11, 2022

mpage commented Oct 4, 2022 •

edited by bedevere-bot

Loading

cpython-cla-bot bot commented Oct 4, 2022 •

edited

Loading