Skip to content

gh-92228: disable the compiler's 'small exit block inlining' optimization for blocks that have a line number #94592

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

Merged
merged 7 commits into from
Jul 7, 2022

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Jul 6, 2022

Inlining of code that corresponds to source code lines, can make it hard to distinguish later between code which is only reachable from except handlers, and that which is reachable in normal control flow. This caused problems with the debugger's jump feature.

This PR turns off the inlining optimisation for code which has line numbers. We still inline things like the implicit "return None".

Automerge-Triggered-By: GH:tiran

@iritkatriel iritkatriel requested a review from markshannon as a code owner July 6, 2022 05:53
@iritkatriel iritkatriel marked this pull request as draft July 6, 2022 06:31
@iritkatriel iritkatriel changed the title gh-92228: disable the compiler's 'small exit block inlining' optimization gh-92228: disable the compiler's 'small exit block inlining' optimization for blocks that have a line number Jul 6, 2022
@iritkatriel iritkatriel marked this pull request as ready for review July 6, 2022 14:06
@iritkatriel iritkatriel requested a review from brandtbucher as a code owner July 6, 2022 14:06
@tiran
Copy link
Member

tiran commented Jul 6, 2022

Does the PR need yet another bytecode magic bump or can we piggy-back on the magic bump that was merged yesterday?

@iritkatriel
Copy link
Member Author

Does the PR need yet another bytecode magic bump or can we piggy-back on the magic bump that was merged yesterday?

I think it's safe to piggyback. In any case, we didn't change the set of opcodes so I think the old code would normally still work (unless you ask the compiler to jump over an except block to a small exit block).

@iritkatriel
Copy link
Member Author

Benchmark results show no difference overall:

+-------------------------+----------+------------------------+
| Benchmark               | baseline | gh-92228               |
+=========================+==========+========================+
| chameleon               | 6.37 ms  | 6.27 ms: 1.02x faster  |
+-------------------------+----------+------------------------+
| chaos                   | 64.9 ms  | 65.2 ms: 1.00x slower  |
+-------------------------+----------+------------------------+
| crypto_pyaes            | 72.0 ms  | 74.1 ms: 1.03x slower  |
+-------------------------+----------+------------------------+
| deltablue               | 3.23 ms  | 3.29 ms: 1.02x slower  |
+-------------------------+----------+------------------------+
| dulwich_log             | 60.9 ms  | 61.7 ms: 1.01x slower  |
+-------------------------+----------+------------------------+
| float                   | 70.0 ms  | 71.7 ms: 1.02x slower  |
+-------------------------+----------+------------------------+
| go                      | 135 ms   | 133 ms: 1.01x faster   |
+-------------------------+----------+------------------------+
| hexiom                  | 5.95 ms  | 5.98 ms: 1.01x slower  |
+-------------------------+----------+------------------------+
| json                    | 4.77 ms  | 4.72 ms: 1.01x faster  |
+-------------------------+----------+------------------------+
| json_dumps              | 12.0 ms  | 12.1 ms: 1.00x slower  |
+-------------------------+----------+------------------------+
| json_loads              | 24.4 us  | 24.5 us: 1.00x slower  |
+-------------------------+----------+------------------------+
| logging_format          | 6.29 us  | 6.34 us: 1.01x slower  |
+-------------------------+----------+------------------------+
| logging_silent          | 89.4 ns  | 90.6 ns: 1.01x slower  |
+-------------------------+----------+------------------------+
| logging_simple          | 5.68 us  | 5.77 us: 1.02x slower  |
+-------------------------+----------+------------------------+
| meteor_contest          | 100 ms   | 101 ms: 1.01x slower   |
+-------------------------+----------+------------------------+
| nbody                   | 92.6 ms  | 90.9 ms: 1.02x faster  |
+-------------------------+----------+------------------------+
| pathlib                 | 17.8 ms  | 18.1 ms: 1.01x slower  |
+-------------------------+----------+------------------------+
| pickle                  | 10.3 us  | 10.4 us: 1.00x slower  |
+-------------------------+----------+------------------------+
| pickle_dict             | 31.1 us  | 31.3 us: 1.01x slower  |
+-------------------------+----------+------------------------+
| pickle_list             | 4.30 us  | 4.13 us: 1.04x faster  |
+-------------------------+----------+------------------------+
| pickle_pure_python      | 285 us   | 289 us: 1.01x slower   |
+-------------------------+----------+------------------------+
| pidigits                | 199 ms   | 193 ms: 1.03x faster   |
+-------------------------+----------+------------------------+
| pycparser               | 1.07 sec | 1.11 sec: 1.03x slower |
+-------------------------+----------+------------------------+
| pyflate                 | 398 ms   | 389 ms: 1.02x faster   |
+-------------------------+----------+------------------------+
| python_startup          | 8.20 ms  | 8.26 ms: 1.01x slower  |
+-------------------------+----------+------------------------+
| python_startup_no_site  | 6.05 ms  | 6.10 ms: 1.01x slower  |
+-------------------------+----------+------------------------+
| raytrace                | 288 ms   | 281 ms: 1.02x faster   |
+-------------------------+----------+------------------------+
| regex_dna               | 200 ms   | 203 ms: 1.01x slower   |
+-------------------------+----------+------------------------+
| regex_effbot            | 3.41 ms  | 3.70 ms: 1.08x slower  |
+-------------------------+----------+------------------------+
| regex_v8                | 21.9 ms  | 21.8 ms: 1.01x faster  |
+-------------------------+----------+------------------------+
| scimark_fft             | 306 ms   | 302 ms: 1.02x faster   |
+-------------------------+----------+------------------------+
| scimark_sor             | 111 ms   | 109 ms: 1.02x faster   |
+-------------------------+----------+------------------------+
| scimark_sparse_mat_mult | 4.00 ms  | 3.90 ms: 1.03x faster  |
+-------------------------+----------+------------------------+
| spectral_norm           | 94.1 ms  | 91.3 ms: 1.03x faster  |
+-------------------------+----------+------------------------+
| sympy_integrate         | 20.1 ms  | 20.1 ms: 1.00x slower  |
+-------------------------+----------+------------------------+
| sympy_sum               | 160 ms   | 159 ms: 1.01x faster   |
+-------------------------+----------+------------------------+
| sympy_str               | 279 ms   | 276 ms: 1.01x faster   |
+-------------------------+----------+------------------------+
| telco                   | 6.55 ms  | 6.44 ms: 1.02x faster  |
+-------------------------+----------+------------------------+
| thrift                  | 737 us   | 727 us: 1.01x faster   |
+-------------------------+----------+------------------------+
| unpack_sequence         | 44.2 ns  | 44.8 ns: 1.01x slower  |
+-------------------------+----------+------------------------+
| unpickle                | 13.2 us  | 13.6 us: 1.03x slower  |
+-------------------------+----------+------------------------+
| unpickle_list           | 5.02 us  | 4.97 us: 1.01x faster  |
+-------------------------+----------+------------------------+
| unpickle_pure_python    | 198 us   | 200 us: 1.01x slower   |
+-------------------------+----------+------------------------+
| xml_etree_iterparse     | 106 ms   | 106 ms: 1.01x slower   |
+-------------------------+----------+------------------------+
| xml_etree_process       | 52.3 ms  | 52.7 ms: 1.01x slower  |
+-------------------------+----------+------------------------+
| Geometric mean          | (ref)    | 1.00x slower           |
+-------------------------+----------+------------------------+

Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

Tests are passing, benchmark shows no impact, C code changes look fine, test changes are plausible. I trust you know what you are doing, too. 👍

@iritkatriel iritkatriel added interpreter-core (Objects, Python, Grammar, and Parser dirs) needs backport to 3.11 only security fixes labels Jul 6, 2022
@miss-islington miss-islington merged commit bde06e1 into python:main Jul 7, 2022
@miss-islington
Copy link
Contributor

Thanks @iritkatriel for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @iritkatriel, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker bde06e1b8381f140b296a397ddd1deb1c784ff8e 3.11

@miss-islington miss-islington self-assigned this Jul 7, 2022
tiran pushed a commit to tiran/cpython that referenced this pull request Jul 7, 2022
…ing' optimization for blocks that have a line number (pythonGH-94592)

Inlining of code that corresponds to source code lines, can make it hard to distinguish later between code which is only reachable from except handlers, and that which is reachable in normal control flow. This caused problems with the debugger's jump feature.

This PR turns off the inlining optimisation for code which has line numbers. We still inline things like the implicit "return None"..
(cherry picked from commit bde06e1)

Co-authored-by: Irit Katriel <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 7, 2022
@bedevere-bot
Copy link

GH-94643 is a backport of this pull request to the 3.11 branch.

sidney pushed a commit to sidney/cpython that referenced this pull request Jul 7, 2022
…timization for blocks that have a line number (pythonGH-94592)

Inlining of code that corresponds to source code lines, can make it hard to distinguish later between code which is only reachable from except handlers, and that which is reachable in normal control flow. This caused problems with the debugger's jump feature.

This PR turns off the inlining optimisation for code which has line numbers. We still inline things like the implicit "return None".
miss-islington pushed a commit that referenced this pull request Jul 7, 2022
…ptimization for blocks that have a line number (GH-94592) (GH-94643)

Inlining of code that corresponds to source code lines, can make it hard to distinguish later between code which is only reachable from except handlers, and that which is reachable in normal control flow. This caused problems with the debugger's jump feature.

This PR turns off the inlining optimisation for code which has line numbers. We still inline things like the implicit "return None"..
(cherry picked from commit bde06e1)

Co-authored-by: Irit Katriel <[email protected]>
@esc
Copy link
Contributor

esc commented Jul 7, 2022

This PR turns off the inlining optimisation for code which has line numbers. We still inline things like the implicit "return None".

Quick question: what other "things" could possibly not have a line number besides the implicit return None?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants