gh-91053: make func watcher tests resilient to other func watchers #106286
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
skipping news since this is a fix purely to CPython's own test suite; it has no effect on users of the func watchers API |
|
Thanks @carljm for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Jul 3, 2023
…ers (pythonGH-106286) (cherry picked from commit 5890621) Co-authored-by: Carl Meyer <[email protected]>
|
GH-106365 is a backport of this pull request to the 3.12 branch. |
carljm
added a commit
to carljm/cpython
that referenced
this pull request
Jul 3, 2023
* main: (167 commits) pythongh-91053: make func watcher tests resilient to other func watchers (python#106286) pythongh-104050: Add more type hints to Argument Clinic DSLParser() (python#106354) pythongh-106359: Fix corner case bugs in Argument Clinic converter parser (python#106361) pythongh-104146: Remove unused attr 'parameter_indent' from clinic.DLParser (python#106358) pythongh-106320: Remove private _PyErr C API functions (python#106356) pythongh-104050: Annotate Argument Clinic DSLParser attributes (python#106357) pythongh-106320: Create pycore_modsupport.h header file (python#106355) pythongh-106320: Move _PyUnicodeWriter to the internal C API (python#106342) pythongh-61215: New mock to wait for multi-threaded events to happen (python#16094) Document PYTHONSAFEPATH along side -P (python#106122) Replace the esoteric term 'datum' when describing dict comprehensions (python#106119) pythongh-104050: Add more type hints to Argument Clinic DSLParser() (python#106343) pythongh-106320: _testcapi avoids private _PyUnicode_EqualToASCIIString() (python#106341) pythongh-106320: Add pycore_complexobject.h header file (python#106339) pythongh-106078: Move DecimalException to _decimal state (python#106301) pythongh-106320: Use _PyInterpreterState_GET() (python#106336) pythongh-106320: Remove private _PyInterpreterState functions (python#106335) pythongh-104922: Doc: add note about PY_SSIZE_T_CLEAN (python#106314) pythongh-106217: Truncate the issue body size of `new-bugs-announce-notifier` (python#106329) pythongh-104922: remove PY_SSIZE_T_CLEAN (python#106315) ...
facebook-github-bot
pushed a commit
to facebookincubator/cinder
that referenced
this pull request
Jul 6, 2023
Summary: Register a function watcher for the JIT, and use its callback to handle func-modified and func-destroyed events. I'm leaving func-initialization to a separate PR; it will require more involved handling, because we'll also want to visit GC objects to find functions created before the JIT was initialized. I eliminated the call to `PyEntry_init` after a change to function defaults. This only happened if the function was not JIT-compiled. I think this dates back to Cinder 3.8 where we had many more (non-JIT) function entrypoint variants, depending on number of arguments, argument defaults, etc, and needed to ensure we set the right one. But in 3.10 we eliminated all those custom entry points, so I don't think this is needed anymore. I had to make a fix to the func watchers tests so they would work correctly when running with another func watcher active. I also submitted this fix upstream: python/cpython#106286 And I had to delete a C++ test that was passing only due to a series of accidents. The func-modified callbacks (both before and after this diff) are global and dispatch only to the global singleton `jit_ctx` in `pyjit.cpp`, so they can't be tested correctly by a unit test that never globally enables the JIT and only constructs its own private JIT context. The function-modified callback in this test was doing nothing, but the entrypoint of the function was getting re-set to `_PyFunction_Vectorcall` anyway due to `PyEntry_init` seeing the JIT as not enabled; this seems unlikely to be a realistic scenario the test was intended to check. There is already a Python-level test (`test_funcattrs.FunctionPropertiesTest.test_copying___code__`) that verifies that re-assigning `__code__` changes the behavior of the function; we run this test under the JIT, and it fails if we fail to deopt the function on `__code__` reassignment. So the behavior we care about is already tested. Reviewed By: alexmalyshev Differential Revision: D47156535 fbshipit-source-id: ba15f93800e23b33eb12262a201d24360df39a67
facebook-github-bot
pushed a commit
to facebookincubator/cinder
that referenced
this pull request
Aug 10, 2023
Summary: This reverts D47602760, and re-lands D47156535, D47165628, and D47306492, with changes to adapt to the new `cinder.cpp` compilation unit for Cinder-wide infra. Since Static Python relies on function watchers, function watchers go there rather than in the JIT. The revert wasn't due to any problems discovered in this code, it was just to minimize conflicts in the dict watchers revert. With this diff, all `#ifdef ENABLE_CINDERX` couplings have been removed from `funcobject.c`. This removes the `InitFunction` HIR opcode from the JIT, since that opcode existed to do Cinder entrypoint initialization for new functions, and this now happens automatically as part of `MakeFunction`, due to the function watcher. In order to properly initialize functions created before `Cinder_Init` is called, we use the new (upstream and backported) `PyUnstable_GC_VisitObjects` API to visit all GC objects and initialize the ones that are functions. I eliminated the call to `PyEntry_init` after a change to function defaults. This only happened if the function was not JIT-compiled. I think this dates back to Cinder 3.8 where we had many more (non-JIT) function entrypoint variants, depending on number of arguments, argument defaults, etc, and needed to ensure we set the right one. But in 3.10 we eliminated all those custom entry points, so I don't think this is needed anymore. I had to make a fix to the func watchers tests so they would work correctly when running with another func watcher active. I also submitted this fix upstream: python/cpython#106286 And I had to delete a C++ test that was passing only due to a series of accidents. The func-modified callbacks (both before and after this diff) are global and dispatch only to the global singleton `jit_ctx` in `pyjit.cpp`, so they can't be tested correctly by a unit test that never globally enables the JIT and only constructs its own private JIT context. The function-modified callback in this test was doing nothing, but the entrypoint of the function was getting re-set to `_PyFunction_Vectorcall` anyway due to `PyEntry_init` seeing the JIT as not enabled; this seems unlikely to be a realistic scenario the test was intended to check. There is already a Python-level test (`test_funcattrs.FunctionPropertiesTest.test_copying___code__`) that verifies that re-assigning `__code__` changes the behavior of the function; we run this test under the JIT, and it fails if we fail to deopt the function on `__code__` reassignment. So the behavior we care about is already tested. Reviewed By: alexmalyshev Differential Revision: D48128982 fbshipit-source-id: cd56b4c485f62e33580b4838882cd4c610653a4f
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to bridge between the C-only function watchers API and the func watcher tests written in Python, the support code in testcapimodule maintains a mapping between an array of up to two test func watchers (indices 0 and 1) and the real func watcher IDs (returned by
PyFunction_AddWatcher) corresponding to those test watcher indices.add_func_watcherandclear_func_watcherin testcapimodule disagreed about which of those ids should be exposed to the Python test code.add_func_watcherreturned the "test watcher index" (always 0 or 1), butclear_func_watcherexpected the real watcher ID.When the tests are run with no other func watchers active, this confusion doesn't matter, because the ids will always match up exactly; test watchers 0 and 1 will always have the real watcher IDs 0 and 1. But if the tests are run with another func watcher active (e.g. from a third-party JIT), the IDs will no longer match up (test watchers 0 and 1 will have real watcher IDs 1 and 2), and this breaks some tests.
Fix
add_func_watcherso that we always expose real watcher IDs (not test watcher indices) to the Python tests.(It's necessary to make the fix in this direction because of
test_clear_unassigned_watcher_id, which needs to pass an unassigned real watcher ID through toPyFunction_ClearWatcher.)Also rename
NUM_FUNC_WATCHERStoNUM_TEST_FUNC_WATCHERSto help clarify this distinction in the code, and update an error message to be clearer that there are no test watcher indices free; there may still be actual watcher slots free.