Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-117086: Fix function version numbering #117093

Closed
wants to merge 2 commits into from
Closed

GH-117086: Fix function version numbering #117093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b9e8ce1
Fix function version numbering
markshannon Mar 20, 2024
8a0e362
Check builtins before issuing function version
markshannon Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Include/cpython/code.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ typedef struct {
int co_ncellvars; /* total number of cell variables */ \
int co_nfreevars; /* number of free variables */ \
uint32_t co_version; /* version number */ \
\
int32_t co_expected_number_of_defaults; \
PyObject *co_localsplusnames; /* tuple mapping offsets to names */ \
PyObject *co_localspluskinds; /* Bytes mapping to local kinds (one byte \
per variable) */ \
Expand All @@ -170,6 +170,7 @@ typedef struct {
uintptr_t _co_instrumentation_version; /* current instrumentation version */ \
_PyCoMonitoringData *_co_monitoring; /* Monitoring data */ \
int _co_firsttraceable; /* index of first traceable instruction */ \
uint64_t co_expected_globals_version; \
/* Scratch space for extra data relating to the code object. \
Type is a void* to keep the format private in codeobject.c to force \
people to go through the proper APIs. */ \
Expand Down
35 changes: 35 additions & 0 deletions Lib/test/test_optimizer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import unittest
import types
from test.support import import_helper
from textwrap import dedent


_testinternalcapi = import_helper.import_module("_testinternalcapi")
Expand Down Expand Up @@ -78,6 +79,40 @@ def func(x=0):
)


class TestOptimizations(unittest.TestCase):

def test_globals_to_consts(self):
Copy link
Member

Choose a reason for hiding this comment

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

Note that this test only fails when tier 2 is selected (e.g. -X uops, or when the JIT is enabled).

We have machinery in test_capi/test_opt.py to force the optimizer on. It feels pretty arbitrary whether this test belongs here or there. Can you explain the criteria for where a tier-2-related test should go?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you explain the criteria for where a tier-2-related test should go?

I don't think there are any. This test should pass for tier-1 and tier-2, as should all tests.

Copy link
Member

Choose a reason for hiding this comment

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

Okay. In my mind this test should be moved to test_capi/test_opt.py and should be wrapped in with temporary_optimizer(_testinternalcapi.new_uop_optimizer()):. Otherwise it doesn't test anything unless build with the JIT or run with -Xuops.

#GH-117051
src = """
def f():
global x
def inner(i):
a = 1
for _ in range(100):
a = x + i
return a
return inner

func = f()
"""

co = compile(dedent(src), __file__, "exec")

ns1 = {"x": 1000}
eval(co, ns1)
func1 = ns1["func"]
for i in range(1000):
x = func1(i)
self.assertEqual(x, 1999)

ns2 = {"x": 2000}
eval(co, ns2)
func2 = ns2["func"]
for i in range(1000):
x = func2(i)
self.assertEqual(x, 2999)


class TestOptimizerSymbols(unittest.TestCase):

def test_optimizer_symbols(self):
Expand Down
8 changes: 3 additions & 5 deletions Objects/codeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,11 +414,9 @@ init_code(PyCodeObject *co, struct _PyCodeConstructor *con)
co->co_framesize = nlocalsplus + con->stacksize + FRAME_SPECIALS_SIZE;
co->co_ncellvars = ncellvars;
co->co_nfreevars = nfreevars;
PyInterpreterState *interp = _PyInterpreterState_GET();
co->co_version = interp->func_state.next_version;
if (interp->func_state.next_version != 0) {
interp->func_state.next_version++;
}
co->co_version = 0;
co->co_expected_globals_version = 0;
co->co_expected_number_of_defaults = -1;
co->_co_monitoring = NULL;
co->_co_instrumentation_version = 0;
/* not set */
Expand Down
33 changes: 29 additions & 4 deletions Python/bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3829,14 +3829,32 @@ dummy_func(
PyFunctionObject *func_obj = (PyFunctionObject *)
PyFunction_New(codeobj, GLOBALS());

Py_DECREF(codeobj);
if (func_obj == NULL) {
GOTO_ERROR(error);
}

_PyFunction_SetVersion(
func_obj, ((PyCodeObject *)codeobj)->co_version);
PyCodeObject *code = (PyCodeObject *)codeobj;
if (func_obj->func_builtins != tstate->interp->builtins) {
_PyFunction_SetVersion(func_obj, 0);
}
else if (code->co_version == 0) {
code->co_version = tstate->interp->func_state.next_version;
if (tstate->interp->func_state.next_version != 0) {
tstate->interp->func_state.next_version++;
}
assert(code->co_expected_globals_version == 0);
if (PyDict_CheckExact(GLOBALS())) {
code->co_expected_globals_version = ((PyDictObject *)GLOBALS())->ma_version_tag;
}
_PyFunction_SetVersion(func_obj, code->co_version);
}
else {
if (PyDict_CheckExact(GLOBALS()) &&
code->co_expected_globals_version == ((PyDictObject *)GLOBALS())->ma_version_tag) {
_PyFunction_SetVersion(func_obj, code->co_version);
}
}
func = (PyObject *)func_obj;
Py_DECREF(codeobj);
Comment on lines +3835 to +3857
Copy link
Member

Choose a reason for hiding this comment

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

This feels like an awfully large block of code for the JIT. Maybe it should be moved to a helper function?

Also, my brain is too small to fully grasp what will happen if we have a local function that references a global and is repeatedly created (maybe a generator expression inside a hot loop), when the referenced global changes (or some other global, given how dict versions work IIRC). Is that going to completely disable tier 2 for that hot loop, or is it just going to use _LOAD_GLOBAL_MODULE instead of _LOAD_CONST_INLINE?

A variant of the new test may reveal what happens more easily than trying to reason it through...

Copy link
Member Author

Choose a reason for hiding this comment

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

This feels like an awfully large block of code for the JIT. Maybe it should be moved to a helper function?

Maybe, but that is the responsibility of the JIT builder. Let's not complicate the interpreter specification based on the current behavior of the JIT.

Is that going to completely disable tier 2 for that hot loop?

If the globals are changed, then we will get a different function version number.
That will disable some tier 2 optimizations. There is no reason why it should completely disable tier 2 optimization.

Copy link
Member

Choose a reason for hiding this comment

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

I would like to play with this to see what actually happens. As I said, my brain is too small to reason it through, and your response ("should") does not allay my worries.

}

inst(SET_FUNCTION_ATTRIBUTE, (attr, func -- func)) {
Expand All @@ -3860,6 +3878,13 @@ dummy_func(
assert(PyTuple_CheckExact(attr));
assert(func_obj->func_defaults == NULL);
func_obj->func_defaults = attr;
PyCodeObject *code = (PyCodeObject *)func_obj->func_code;
if (code->co_expected_number_of_defaults < 0) {
code->co_expected_number_of_defaults = (int32_t)PyTuple_GET_SIZE(attr);
}
else if (code->co_expected_number_of_defaults != PyTuple_GET_SIZE(attr)) {
func_obj->func_version = 0;
}
Comment on lines +3881 to +3887
Copy link
Member

Choose a reason for hiding this comment

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

This works, but couldn't we let the bytecode compiler give us the number? Again, I worry a bit about the size of the JIT code.

break;
default:
Py_UNREACHABLE();
Expand Down
32 changes: 29 additions & 3 deletions Python/executor_cases.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 29 additions & 3 deletions Python/generated_cases.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading