gh-131798: Split `CHECK_FUNCTION_VERSION` into two guards in the JIT by Sacul0457 · Pull Request #145080 · python/cpython

Sacul0457

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realised this existing code I wrote might be wrong

The idea is that it's possible to modify a function version midway in a trace, so we need to recheck it or at least add a watcher at the first check.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sacul0457 would you be willing to help me fix this (please open a separate issue if so)? Basically you'd need to add a PyFunction_AddWatcher in the else case of this branch. (See what we do in _GUARD_TYPE_VERSION). It's just a callback that invalidates the executors (see type_watcher_callback).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, Ill open an issue and the PR in a bit

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I was just taking a look at this issue and I've got a few questions:

We add a PyFunction_AddWatcher only if the func_version was set properly, like what _GUARD_TYPE_VERSION version does?
If so, what PyFunction_WatchCallback do I add to PyFunction_AddWatcher or am I suppose to create one similar to type_watcher_callback

I'll continue to look into this in the meantime, thx! :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We add a PyFunction_AddWatcher only if the func_version was set properly, like what _GUARD_TYPE_VERSION version does?

Yup!

If so, what PyFunction_WatchCallback do I add to PyFunction_AddWatcher or am I suppose to create one similar to type_watcher_callback

Yes you need to create your own one with its own ID similar to type_watcher_callback.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function watcher needs to be more specific though: we want to invalidate for everything except for PyFunction_EVENT_DESTROY. As that doesn't actually invalidate any of our assumptions.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok ty!

Yes you need to create your own one with its own ID similar to type_watcher_callback.

Just one more thing on the callback (sorry!),
type_watcher_callback calls PyType_Unwatch, and so I'm guessing I also need to create some sort of PyFunc_Unwatch?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be PyFunction_ClearWatcher (check Objects/funcobject.c)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found it thx!