GeezerVison

Copy link

Copy Markdown

Member

carljm requested a review from erlend-aasland June 30, 2023 18:06

itamaro approved these changes Jul 1, 2023

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland approved these changes Jul 2, 2023

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

carljm deleted the fixfuncwatchertests branch July 3, 2023 14:25

…ers (pythonGH-106286)

(cherry picked from commit 5890621)

Co-authored-by: Carl Meyer <[email protected]>

…hers (GH-106286) (#106365)

gh-91053: make func watcher tests resilient to other func watchers (GH-106286)
(cherry picked from commit 5890621)

Co-authored-by: Carl Meyer <[email protected]>

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

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

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