-
Notifications
You must be signed in to change notification settings - Fork 19
jit-trace: seed keyword-only defaults from a namespace mapping, and ask the is-None scan about the operand the branch tests #1494
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| bridges_compiled=0 | ||
| descr_set_absent=0 | ||
| descr_set_ambiguous=0 | ||
| descr_set_stale_absent=0 | ||
| fbw_blackhole_adopted_multi_frame=0 | ||
| fbw_blackhole_adopted_single_frame=0 | ||
| fbw_escape_plain_fallback=0 | ||
| fbw_escape_plain_fallback_unclean=0 | ||
| fbw_midbody_latch_new_unjournaled=0 | ||
| fbw_rolled_back_with_effects=0 | ||
| fbw_store_journal_rollback_failed=0 | ||
| field_pos_attached_misplaced=0 | ||
| field_pos_spec_misplaced=0 | ||
| guard_failures=2 | ||
| internal_compile_panics=0 | ||
| loops_aborted=0 | ||
| loops_compiled=2 | ||
| retraces_compiled=0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| bridges_compiled=0 | ||
| descr_set_absent=0 | ||
| descr_set_ambiguous=0 | ||
| descr_set_stale_absent=0 | ||
| fbw_blackhole_adopted_multi_frame=0 | ||
| fbw_blackhole_adopted_single_frame=0 | ||
| fbw_escape_plain_fallback=0 | ||
| fbw_escape_plain_fallback_unclean=0 | ||
| fbw_midbody_latch_new_unjournaled=0 | ||
| fbw_rolled_back_with_effects=0 | ||
| fbw_store_journal_rollback_failed=0 | ||
| field_pos_attached_misplaced=0 | ||
| field_pos_spec_misplaced=0 | ||
| guard_failures=2 | ||
| internal_compile_panics=0 | ||
| loops_aborted=0 | ||
| loops_compiled=2 | ||
| retraces_compiled=0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| # `optional_none_arg_inline` with the default changed from `None` to an int, so | ||
| # the local the `is`-against-None branch tests is the one the callee's register | ||
| # banks hold unboxed. That is the case the scan exists for: a mid-body guard | ||
| # resume cannot source the operand's Ref form from those banks, the encoded | ||
| # liveness stream disagrees with the decoder and the caller frame is corrupted. | ||
| # `_read_from_buffer(self, size=-1)` was the shape that miscompiled. | ||
| # | ||
| # The gate is `loops_compiled` reading 2 -- the callee keeps a trace of its own. | ||
| # Dropping the scan folds it into the caller's and the count falls to 1, which | ||
| # is the movement this fixture is here to catch. | ||
| N = 200000 | ||
|
|
||
|
|
||
| def read_n(buf, size=-1): | ||
| if size is None or size < 0: | ||
| return buf | ||
| return size | ||
|
|
||
|
|
||
| def main(): | ||
| total = 0 | ||
| for i in range(N): | ||
| total += read_n(i) | ||
| total += read_n(1, 7) | ||
| print(total) | ||
|
|
||
|
|
||
| main() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| bridges_compiled=0 | ||
| descr_set_absent=0 | ||
| descr_set_ambiguous=0 | ||
| descr_set_stale_absent=0 | ||
| fbw_blackhole_adopted_multi_frame=0 | ||
| fbw_blackhole_adopted_single_frame=0 | ||
| fbw_escape_plain_fallback=0 | ||
| fbw_escape_plain_fallback_unclean=0 | ||
| fbw_midbody_latch_new_unjournaled=0 | ||
| fbw_rolled_back_with_effects=0 | ||
| fbw_store_journal_rollback_failed=0 | ||
| field_pos_attached_misplaced=0 | ||
| field_pos_spec_misplaced=0 | ||
| guard_failures=2 | ||
| internal_compile_panics=0 | ||
| loops_aborted=0 | ||
| loops_compiled=2 | ||
| retraces_compiled=0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| # pyre-check: selfcheck | ||
| # pyre-check: selfcheck-compiles=hot | ||
| # pyre-check: spec-folds=kwonly_defaults_inline | ||
| # Self-checking guard for the pins under an inlined callee's keyword-only | ||
| # defaults. | ||
| # | ||
| # Seeding those locals no longer probes `__kwdefaults__` per call. The mapping | ||
| # a definition builds carries a version, so the walker bakes each entry's cell, | ||
| # records a quasi-immutable marker on `Function.w_kw_defs` and one on the | ||
| # mapping's strategy version, drains both with a single `GuardNotInvalidated`, | ||
| # and reads the cell's field live. Three separate mechanisms therefore stand | ||
| # between a mutation and the next call, and a census can only say the fold | ||
| # fired -- not that the pins are honest. Each site below moves one of them and | ||
| # prints a wrong number if that one is missing. | ||
| # | ||
| # Sites: | ||
| # A overwrite an existing entry in place. A cell that already holds an | ||
| # object absorbs the store and bumps no version, so nothing invalidates: | ||
| # this is answered by the live field read alone, and a fold that baked the | ||
| # VALUE rather than the cell keeps returning the old one. | ||
| # B delete an entry, then add it back. Both move the strategy version, and | ||
| # the deleted window is observable on its own -- the call must raise | ||
| # rather than seed a stale default. | ||
| # C rebind the attribute. `f.__kwdefaults__ = d` stores `d` itself, so only | ||
| # the `Function.w_kw_defs` marker can catch this; the version marker is | ||
| # still watching the old mapping, which no longer answers anything. | ||
| # D keep calling after that rebind. A plain dict has no version to pin, so | ||
| # the resolve declines and the callee seeds through the ordinary path -- | ||
| # which must still see a later in-place mutation. | ||
| N = 20000 | ||
|
|
||
|
|
||
| def g(x, *, step=1, tag="a"): | ||
| return x + step, tag | ||
|
|
||
|
|
||
| def hot(n, want_step, want_tag): | ||
| """Run the loop hot and count answers that disagree with the mapping.""" | ||
| bad = 0 | ||
| for i in range(n): | ||
| value, tag = g(i) | ||
| if value != i + want_step or tag != want_tag: | ||
| bad += 1 | ||
| return bad | ||
|
|
||
|
|
||
| def main(): | ||
| # Warm first so the sites below mutate a compiled loop rather than an | ||
| # interpreted one. | ||
| if hot(N, 1, "a"): | ||
| print("FAIL warm-up disagreed with the declared defaults") | ||
| return 1 | ||
|
|
||
| g.__kwdefaults__["step"] = 5 | ||
| bad = hot(N, 5, "a") | ||
| if bad: | ||
| print(f"FAIL site A missed an in-place overwrite on {bad} of {N} calls") | ||
| return 1 | ||
|
|
||
| del g.__kwdefaults__["tag"] | ||
| missing = 0 | ||
| for i in range(N): | ||
| try: | ||
| g(i) | ||
| except TypeError: | ||
| missing += 1 | ||
| if missing != N: | ||
| print(f"FAIL site B seeded a deleted default on {N - missing} of {N} calls") | ||
| return 1 | ||
|
Comment on lines
+60
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: printf '%s\n' '--- fixture ---'
cat -n pyre/bench/synth/kwdefaults_invalidation.py
printf '%s\n' '--- applicable repository guidance ---'
find /tmp/coderabbit-repo-knowledge/youknowone-pyre-b09184ef -maxdepth 2 -type f -name '*.md' -printRepository: youknowone/pyre Length of output: 5130 🏁 Script executed: printf '%s\n' '--- repository conventions ---'
cat /tmp/coderabbit-repo-knowledge/youknowone-pyre-b09184ef/conventions/repo-wide.md
printf '%s\n' '--- bench-synth learnings ---'
cat /tmp/coderabbit-repo-knowledge/youknowone-pyre-b09184ef/learnings/pyre-bench-synth.md
printf '%s\n' '--- compile-directive and JIT-stat references ---'
rg -n --glob '!target/**' --glob '!node_modules/**' \
'selfcheck-compiles|spec-folds|jit.?stats|JIT.*stat|trace.*count|compiled.*count|statistics' \
pyre tests README.md .github 2>/dev/null | head -240Repository: youknowone/pyre Length of output: 28117 🏁 Script executed: printf '%s\n' '--- selfcheck compilation contract ---'
sed -n '1925,2015p' pyre/check.py
printf '%s\n' '--- selfcheck execution and merged-stat gate ---'
sed -n '1060,1125p' pyre/check.py
sed -n '1520,1575p' pyre/check.py
printf '%s\n' '--- fixture baselines and related examples ---'
git ls-files 'pyre/bench/*.jitstats' 'pyre/bench/**/*.jitstats' | grep -E 'synth|kwdefaults' | head -80
sed -n '1,90p' pyre/bench/synth/README.mdRepository: youknowone/pyre Length of output: 17095 🏁 Script executed: printf '%s\n' '--- exact fixture references ---'
rg -n -C 3 'kwdefaults_invalidation|selfcheck-compiles=|loop-census' \
pyre/check.py pyre/check_synthetic.py pyre/bench/synth \
--glob '*.py' --glob '*.md' --glob '*.jitstats' | head -260
printf '%s\n' '--- run_selfcheck implementation ---'
rg -n 'def run_selfcheck|synth_selfcheck_compiles|selfcheck_compiles|PYRE_LOOP_CENSUS' pyre/check.py
sed -n '3000,3185p' pyre/check.py
printf '%s\n' '--- current directive/baseline naming ---'
git ls-files | grep -E 'kwdefaults_invalidation|kwdefaults.*jitstats|jitstats.*kwdefaults' || trueRepository: youknowone/pyre Length of output: 31488 🏁 Script executed: printf '%s\n' '--- selfcheck runner and census matching ---'
sed -n '2120,2155p' pyre/check.py
sed -n '4225,4315p' pyre/check.py
printf '%s\n' '--- spec-fold contract ---'
sed -n '2040,2115p' pyre/check.py
rg -n -C 4 'kwonly_defaults_inline|spec-folds' pyre/pyre-jit-trace pyre/pyre-interpreter pyre/check.py pyre/bench/synth --glob '*.{rs,py}' | head -220Repository: youknowone/pyre Length of output: 28951 Require a compiled trace for the deleted-default call site.
🧰 Tools🪛 Ruff (0.16.2)[warning] 65-66: (PERF203) 🤖 Prompt for AI AgentsSources: Coding guidelines, Learnings |
||
|
|
||
| g.__kwdefaults__["tag"] = "z" | ||
| bad = hot(N, 5, "z") | ||
| if bad: | ||
| print(f"FAIL site B missed the re-added default on {bad} of {N} calls") | ||
| return 1 | ||
|
|
||
| g.__kwdefaults__ = {"step": 9, "tag": "q"} | ||
| bad = hot(N, 9, "q") | ||
| if bad: | ||
| print(f"FAIL site C missed the rebound mapping on {bad} of {N} calls") | ||
| return 1 | ||
|
|
||
| g.__kwdefaults__["step"] = 11 | ||
| bad = hot(N, 11, "q") | ||
| if bad: | ||
| print(f"FAIL site D missed a mutation after the rebind on {bad} of {N} calls") | ||
| return 1 | ||
|
|
||
| print("PASS kwdefaults invalidation") | ||
| return 0 | ||
|
|
||
|
|
||
| import sys | ||
|
|
||
| sys.exit(main()) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| bridges_compiled=0 | ||
| descr_set_absent=0 | ||
| descr_set_ambiguous=0 | ||
| descr_set_stale_absent=0 | ||
| fbw_blackhole_adopted_multi_frame=0 | ||
| fbw_blackhole_adopted_single_frame=0 | ||
| fbw_escape_plain_fallback=0 | ||
| fbw_escape_plain_fallback_unclean=0 | ||
| fbw_midbody_latch_new_unjournaled=0 | ||
| fbw_rolled_back_with_effects=0 | ||
| fbw_store_journal_rollback_failed=0 | ||
| field_pos_attached_misplaced=0 | ||
| field_pos_spec_misplaced=0 | ||
| guard_failures=1 | ||
| internal_compile_panics=0 | ||
| loops_aborted=0 | ||
| loops_compiled=1 | ||
| retraces_compiled=0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| bridges_compiled=0 | ||
| descr_set_absent=0 | ||
| descr_set_ambiguous=0 | ||
| descr_set_stale_absent=0 | ||
| fbw_blackhole_adopted_multi_frame=0 | ||
| fbw_blackhole_adopted_single_frame=0 | ||
| fbw_escape_plain_fallback=0 | ||
| fbw_escape_plain_fallback_unclean=0 | ||
| fbw_midbody_latch_new_unjournaled=0 | ||
| fbw_rolled_back_with_effects=0 | ||
| fbw_store_journal_rollback_failed=0 | ||
| field_pos_attached_misplaced=0 | ||
| field_pos_spec_misplaced=0 | ||
| guard_failures=1 | ||
| internal_compile_panics=0 | ||
| loops_aborted=0 | ||
| loops_compiled=1 | ||
| retraces_compiled=0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| # pyre-check: max-pypy-ratio=8 | ||
| # A hot call into a callee that branches on `arg is None` -- the shape every | ||
| # optional argument in the standard library takes. The tested local holds the | ||
| # default `None`, a Ref; the other parameter is an int. | ||
| # | ||
| # The gate is `loops_compiled` first: declined, the callee takes a trace of | ||
| # its own and the count reads 2; inlined, it folds into the caller's and reads | ||
| # 1. That census is host-independent, which the ratio at this size is not: | ||
| # the body is a single add, so pypy runs the whole loop well under | ||
| # `EXEC_TIME_FLOOR_S` and the comparison marks it -- only the ceiling applies. | ||
| # Sized up until pypy cleared the floor the fixture cost cpython a second, so | ||
| # the counter stays the gate here, as it is for `kwonly_default_callee_inline`. | ||
| # | ||
| # Its sibling `is_none_unboxed_operand_decline` is this fixture with the | ||
| # default changed from `None` to an int, and stays declined. The pair is what | ||
| # shows the `is`-against-None scan asking about the operand the branch tests | ||
| # rather than about the signature as a whole. | ||
| # | ||
| # `clamp` is called once more off the hot path so the printed total depends on | ||
| # the arm the trace guards away as well as the one it keeps: an identity test | ||
| # answered the wrong way changes the sum rather than the timing. | ||
| N = 200000 | ||
|
|
||
|
|
||
| def clamp(v, lo=None): | ||
| if lo is None: | ||
| return v | ||
| return lo | ||
|
|
||
|
|
||
| def main(): | ||
| total = 0 | ||
| for i in range(N): | ||
| total += clamp(i) | ||
| total += clamp(1, 7) | ||
| print(total) | ||
|
|
||
|
|
||
| main() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| bridges_compiled=0 | ||
| descr_set_absent=0 | ||
| descr_set_ambiguous=0 | ||
| descr_set_stale_absent=0 | ||
| fbw_blackhole_adopted_multi_frame=0 | ||
| fbw_blackhole_adopted_single_frame=0 | ||
| fbw_escape_plain_fallback=0 | ||
| fbw_escape_plain_fallback_unclean=0 | ||
| fbw_midbody_latch_new_unjournaled=0 | ||
| fbw_rolled_back_with_effects=0 | ||
| fbw_store_journal_rollback_failed=0 | ||
| field_pos_attached_misplaced=0 | ||
| field_pos_spec_misplaced=0 | ||
| guard_failures=1 | ||
| internal_compile_panics=0 | ||
| loops_aborted=0 | ||
| loops_compiled=1 | ||
| retraces_compiled=0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the invalidation mechanism description.
Lines 9-10 state that this specialization records a quasi-immutable marker on
Function.w_kw_defs. The PR keepsGuardValuebehavior for keyword-default rebinding.function_set_kwdefaultscan notifyQuasiImmutSlot::WKwDefswithout this trace path using that marker. Describe the guard that the specialization actually emits.🤖 Prompt for AI Agents