Skip to content
Open
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
1 change: 1 addition & 0 deletions Include/internal/pycore_optimizer_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ typedef union {

typedef struct _Py_UOpsAbstractFrame {
bool globals_watched;
bool builtins_checked;
// The version number of the globals dicts, once checked. 0 if unchecked.
uint32_t globals_checked_version;
// Max stacklen
Expand Down
9 changes: 7 additions & 2 deletions Include/internal/pycore_uop_ids.h

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

21 changes: 21 additions & 0 deletions Include/internal/pycore_uop_metadata.h

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

114 changes: 114 additions & 0 deletions Lib/test/test_capi/test_opt.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import builtins
import contextlib
import dis
import itertools
Expand Down Expand Up @@ -5054,6 +5055,119 @@ def jitted(funcs):
with self.assertRaises(NameError):
jitted([f, f_with_bad_globals])

def test_jitted_code_sees_changed_copied_builtins(self):
# Trace-time check. The traced function's builtins is a copy of the
# canonical dict with the same keys version, so a version check
# cannot tell them apart. The optimizer must see that func_builtins
# is not interp->builtins and keep _LOAD_GLOBAL_BUILTINS, which reads
# the frame's own dict, rather than fold a constant from the
# canonical one. No runtime guard is involved.

def f(n):
return [len("hello") for _ in range(n)]

copied_builtins = vars(builtins).copy()
f = types.FunctionType(f.__code__, {"__builtins__": copied_builtins})

f(TIER2_THRESHOLD)
ex = get_first_executor(f)
self.assertIsNotNone(ex)
# Not folded: the load must still consult the frame's builtins.
self.assertIn("_LOAD_GLOBAL_BUILTINS", get_opnames(ex))

# Replacing an existing value does not change the keys version.
copied_builtins["len"] = lambda s: 42
self.assertEqual(f(8), [42] * 8)

def test_jitted_code_sees_changed_copied_globals(self):
# Copying a dict must not carry over the keys version of the source.
# The optimizer folds a global to a constant guarded only by the
# globals keys version plus a watcher on the traced dict. A copy is
# not watched, and replacing an existing value does not change the
# keys version, so a function whose globals are a copy of the traced
# dict would otherwise pass _GUARD_GLOBALS_VERSION and see the stale
# constant.
def f(n):
for _ in range(n):
x = COPIED_GLOBAL
return x

for copy in (dict.copy, dict):
with self.subTest(copy=copy):
original = {"COPIED_GLOBAL": 1}
# A fresh code object, so that each subtest traces anew.
f_original = types.FunctionType(f.__code__.replace(), original)
self.assertEqual(f_original(TIER2_THRESHOLD), 1)
ex = get_first_executor(f_original)
self.assertIsNotNone(ex)
uops = get_opnames(ex)
self.assertIn("_GUARD_GLOBALS_VERSION", uops)
# The global was folded to a constant.
self.assertNotIn("_LOAD_GLOBAL_MODULE", uops)

copied = copy(original)
copied["COPIED_GLOBAL"] = 2
# Share the code object, so that the same executor is entered.
f_copied = types.FunctionType(f_original.__code__, copied)
self.assertEqual(f_copied(TIER2_THRESHOLD), 2)
self.assertEqual(f_original(TIER2_THRESHOLD), 1)

def test_jitted_code_sees_different_builtins(self):
# Runtime check. The traced function's builtins IS the canonical
# dict, so folding len to a constant is correct at trace time.
# A second function sharing the code object then enters the same
# executor with other builtins, so only the runtime guard on the
# executing frame's builtins can catch it.
def f(n):
return [len("hello") for _ in range(n)]

namespace = {"__builtins__": builtins}
f_canonical = types.FunctionType(f.__code__, namespace)
copied_builtins = vars(builtins).copy()
namespace["__builtins__"] = copied_builtins
f_copied = types.FunctionType(f.__code__, namespace)


f_canonical(TIER2_THRESHOLD)
ex = get_first_executor(f_canonical)
self.assertIsNotNone(ex)
self.assertIn("_GUARD_BUILTINS_IS_CANONICAL", get_opnames(ex))

copied_builtins["len"] = lambda s: 42
# The executor's owner still sees the canonical len.
self.assertEqual(f_canonical(8), [5] * 8)
# A different function enters the same executor with other builtins.
self.assertEqual(f_copied(8), [42] * 8)

def test_builtins_guard_emitted_once_per_frame(self):
# A frame's builtins cannot change once the frame is pushed, so
# repeated builtin loads in one frame share a single guard, just as
# they already share a single _GUARD_GLOBALS_VERSION.

def warmup(n):
x = 0
for _ in range(n):
x += len("ab")
return x

def one_frame(n):
x = 0
for _ in range(n):
x += len("ab") + abs(-1) + ord("c")
return x

# The optimizer context is reused for every compilation, so compile an
# unrelated trace first: state that is not reset per frame leaks here.
warmup(TIER2_THRESHOLD)
self.assertIsNotNone(get_first_executor(warmup))

_, ex = self._run_with_optimizer(one_frame, TIER2_THRESHOLD)
self.assertIsNotNone(ex)
uop_names = get_opnames(ex)
self.assertNotIn("_LOAD_GLOBAL_BUILTINS", uop_names) # all folded
self.assertEqual(uop_names.count("_GUARD_BUILTINS_IS_CANONICAL"), 1)
self.assertEqual(uop_names.count("_GUARD_GLOBALS_VERSION"), 1)

def test_reference_tracking_across_call_doesnt_crash(self):

def f1():
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Fix the JIT optimizer folding a builtin name to a constant taken from the
interpreter's builtins dictionary even when the running function uses a
different ``__builtins__`` mapping, such as one created with
``vars(builtins).copy()``. The optimizer now only folds when the function's
builtins is the interpreter's, and the folded constant is guarded at runtime so
that another function sharing the same code object but a different builtins
mapping does not use it.
4 changes: 4 additions & 0 deletions Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -1041,6 +1041,10 @@ clone_combined_dict_keys(PyDictObject *orig)

memcpy(keys, orig->ma_keys, keys_size);

/* The keys version must be unique per keys object: the specializer
and the JIT optimizer rely on it to identify a dict's keys. */
keys->dk_version = 0;

/* After copying key/value pairs, we need to incref all
keys and values and they are about to be co-owned by a
new dict object. */
Expand Down
4 changes: 4 additions & 0 deletions Python/bytecodes.c
Original file line number Diff line number Diff line change
Expand Up @@ -2357,6 +2357,10 @@ dummy_func(
STAT_INC(LOAD_GLOBAL, hit);
}

tier2 op(_GUARD_BUILTINS_IS_CANONICAL, (--)) {
DEOPT_IF(BUILTINS() != tstate->interp->builtins);
}

macro(LOAD_GLOBAL_MODULE) =
unused/1 + // Skip over the counter
NOP + // For guard insertion in the JIT optimizer
Expand Down
70 changes: 70 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: 12 additions & 1 deletion Python/optimizer_bytecodes.c
Original file line number Diff line number Diff line change
Expand Up @@ -2519,13 +2519,24 @@ dummy_func(void) {
else if (interp->rare_events.builtin_dict >= _Py_MAX_ALLOWED_BUILTINS_MODIFICATIONS) {
/* Do nothing */
}
else if (ctx->frame->func == NULL ||
ctx->frame->func->func_builtins != builtins) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Empty clause. Can you put a /* Do nothing */ comment here for clarity

/* Do nothing */
}
else {
if (!ctx->builtins_watched) {
PyDict_Watch(BUILTINS_WATCHER_ID, builtins);
ctx->builtins_watched = true;
}
if (ctx->frame->globals_checked_version != 0 && ctx->frame->globals_watched) {
if (ctx->frame->globals_checked_version != 0 &&
ctx->frame->globals_watched)
{
cnst = convert_global_to_const(this_instr, builtins);
if (cnst != NULL && !ctx->frame->builtins_checked) {
ctx->frame->builtins_checked = true;
ADD_OP(_GUARD_BUILTINS_IS_CANONICAL, 0, 0);
ADD_OP(this_instr->opcode, 0, (uintptr_t)cnst);
Comment thread
markshannon marked this conversation as resolved.
}
}
}
if (cnst == NULL) {
Expand Down
16 changes: 15 additions & 1 deletion Python/optimizer_cases.c.h

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

Loading
Loading