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.

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-126892: Reset warmup counters when JIT compiling code #126893

Merged
merged 6 commits into from
Nov 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion Lib/test/support/strace_helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,10 @@ def _make_error(reason, details):
res, cmd_line = run_python_until_end(
"-c",
textwrap.dedent(code),
__run_using_command=[_strace_binary] + strace_flags)
__run_using_command=[_strace_binary] + strace_flags,
# Don't want to trace our JIT's own mmap and mprotect calls:
PYTHON_JIT="0",
Copy link
Contributor

@cmaloney cmaloney Nov 20, 2024

Choose a reason for hiding this comment

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

Guessing this showed up in the test_fileio test test_syscalls_read?

I have been keeping an eye on / working through a couple other issues where mmap(NULL) and mprotect show up, trying to figure out if just excluding those two from that test would be good/effective (memory allocation happening in reading files is fine / not what the test is focused on).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was wondering if this was susceptible to catching badly-timed arena allocations and such too. Might make sense to have an option to ignore the mmap/munmap/mprotect family of functions instead of this ad-hoc fix.

I'm just not familiar with how this is being used, and didn't want to change the meaning/behavior of the tests too dramatically.

)
except OSError as err:
return _make_error("Caught OSError", err)

Expand Down
50 changes: 22 additions & 28 deletions Lib/test/test_capi/test_opt.py
Original file line number Diff line number Diff line change
Expand Up @@ -1385,20 +1385,17 @@ class Foo:
guard_type_version_count = opnames.count("_GUARD_TYPE_VERSION")
self.assertEqual(guard_type_version_count, 1)

def test_guard_type_version_not_removed(self):
"""
Verify that the guard type version is not removed if we modify the class
"""
def test_guard_type_version_removed_invalidation(self):

def thing(a):
x = 0
for i in range(TIER2_THRESHOLD + 100):
for i in range(TIER2_THRESHOLD * 2 + 1):
x += a.attr
# for the first (TIER2_THRESHOLD + 90) iterations we set the attribute on this dummy function which shouldn't
# trigger the type watcher
# then after for the next 10 it should trigger it and stop optimizing
# Note that the code needs to be in this weird form so it's optimized inline without any control flow
setattr((Foo, Bar)[i < TIER2_THRESHOLD + 90], "attr", 2)
# The first TIER2_THRESHOLD iterations we set the attribute on
# this dummy class, which shouldn't trigger the type watcher.
# Note that the code needs to be in this weird form so it's
# optimized inline without any control flow:
setattr((Bar, Foo)[i == TIER2_THRESHOLD + 1], "attr", 2)
x += a.attr
return x

Expand All @@ -1410,24 +1407,21 @@ class Bar:

res, ex = self._run_with_optimizer(thing, Foo())
opnames = list(iter_opnames(ex))

self.assertIsNotNone(ex)
self.assertEqual(res, (TIER2_THRESHOLD * 2) + 219)
guard_type_version_count = opnames.count("_GUARD_TYPE_VERSION")
self.assertEqual(guard_type_version_count, 2)
self.assertEqual(res, TIER2_THRESHOLD * 6 + 1)
call = opnames.index("_CALL_BUILTIN_FAST")
load_attr_top = opnames.index("_LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES", 0, call)
load_attr_bottom = opnames.index("_LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES", call)
self.assertEqual(opnames[:load_attr_top].count("_GUARD_TYPE_VERSION"), 1)
self.assertEqual(opnames[call:load_attr_bottom].count("_CHECK_VALIDITY"), 1)


@unittest.expectedFailure
def test_guard_type_version_not_removed_escaping(self):
"""
Verify that the guard type version is not removed if have an escaping function
"""
def test_guard_type_version_removed_escaping(self):

def thing(a):
x = 0
for i in range(100):
for i in range(TIER2_THRESHOLD):
x += a.attr
# eval should be escaping and so should cause optimization to stop and preserve both type versions
# eval should be escaping
eval("None")
x += a.attr
return x
Expand All @@ -1437,12 +1431,12 @@ class Foo:
res, ex = self._run_with_optimizer(thing, Foo())
opnames = list(iter_opnames(ex))
self.assertIsNotNone(ex)
self.assertEqual(res, 200)
guard_type_version_count = opnames.count("_GUARD_TYPE_VERSION")
# Note: This will actually be 1 for noe
# https://github.com/python/cpython/pull/119365#discussion_r1626220129
self.assertEqual(guard_type_version_count, 2)

self.assertEqual(res, TIER2_THRESHOLD * 2)
call = opnames.index("_CALL_BUILTIN_FAST_WITH_KEYWORDS")
load_attr_top = opnames.index("_LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES", 0, call)
load_attr_bottom = opnames.index("_LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES", call)
self.assertEqual(opnames[:load_attr_top].count("_GUARD_TYPE_VERSION"), 1)
self.assertEqual(opnames[call:load_attr_bottom].count("_CHECK_VALIDITY"), 1)

def test_guard_type_version_executor_invalidated(self):
"""
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Require cold or invalidated code to "warm up" before being JIT compiled
again.
14 changes: 9 additions & 5 deletions Python/bytecodes.c
Original file line number Diff line number Diff line change
Expand Up @@ -2624,15 +2624,16 @@ dummy_func(
}
_PyExecutorObject *executor;
int optimized = _PyOptimizer_Optimize(frame, start, stack_pointer, &executor, 0);
ERROR_IF(optimized < 0, error);
if (optimized) {
if (optimized <= 0) {
this_instr[1].counter = restart_backoff_counter(counter);
ERROR_IF(optimized < 0, error);
}
else {
this_instr[1].counter = initial_jump_backoff_counter();
assert(tstate->previous_executor == NULL);
tstate->previous_executor = Py_None;
GOTO_TIER_TWO(executor);
}
else {
this_instr[1].counter = restart_backoff_counter(counter);
}
}
else {
ADVANCE_ADAPTIVE_COUNTER(this_instr[1].counter);
Expand Down Expand Up @@ -4875,6 +4876,9 @@ dummy_func(
tstate->previous_executor = (PyObject *)current_executor;
GOTO_TIER_ONE(target);
}
else {
exit->temperature = initial_temperature_backoff_counter();
}
}
exit->executor = executor;
}
Expand Down
3 changes: 3 additions & 0 deletions Python/executor_cases.c.h

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

13 changes: 8 additions & 5 deletions Python/generated_cases.c.h

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

Loading